fix: pty kill waits for daemon exit; daemon shutdown doesn't resurrect removed metadata (#69) · compounding.tech/pty@6981c1c

+12 -1

src/cli.ts

··· 15 15 isGone, 16 16 cleanupAll, 17 17 cleanupSocket, 18 + waitForProcessExit, 18 19 validateName, 19 20 validateDisplayName, 20 21 acquireLock, ··· 2087 2088 2088 2089 try { 2089 2090 process.kill(session.pid, "SIGTERM"); 2090 - console.log(`Session "${name}" killed.`); 2091 2091 } catch { 2092 2092 console.error(`Failed to kill session "${name}".`); 2093 + cleanupSocket(name); 2094 + return; 2093 2095 } 2096 + 2097 + // Wait for the daemon to fully exit before returning. Its shutdown re-flushes 2098 + // exit metadata to disk (an atomic tmp-write + rename); if we returned while 2099 + // that was still in flight, a caller that immediately `pty rm`s the session 2100 + // could race the late write and leave a stray temp file behind. Bounded — the 2101 + // SIGTERM shutdown path settles in ~2s; if it somehow overruns we clean up and 2102 + // return anyway (the daemon finishes on its own). 2103 + await waitForProcessExit(session.pid, 3000); 2094 2104 cleanupSocket(name); 2105 + console.log(`Session "${name}" killed.`); 2095 2106 2096 2107 if (wasPermanent && session.metadata?.tags?.ptyfile) { 2097 2108 console.error(`Note: this session is managed by ${session.metadata.tags.ptyfile}`);

+6

src/server.ts

··· 20 20 import { 21 21 getSocketPath, 22 22 getPidPath, 23 + getMetadataPath, 23 24 ensureSessionDir, 24 25 cleanup, 25 26 cleanupAll, ··· 879 880 } 880 881 881 882 private saveExitMetadata(exitCode: number): void { 883 + // Don't resurrect a session whose metadata was already removed. On teardown 884 + // the daemon re-flushes exit metadata during its (possibly watchdog-delayed) 885 + // shutdown; if a caller already `pty rm`'d the session, re-creating its 886 + // `.json` here would leave a stray registry file (and its atomic tmp) behind. 887 + if (!fs.existsSync(getMetadataPath(this.name))) return; 882 888 const existing = readMetadata(this.name); 883 889 writeMetadata(this.name, { 884 890 command: this.options.command,

+14 -1

src/sessions.ts

··· 1047 1047 } 1048 1048 } 1049 1049 1050 - function isProcessAlive(pid: number): boolean { 1050 + export function isProcessAlive(pid: number): boolean { 1051 1051 try { 1052 1052 process.kill(pid, 0); 1053 1053 return true; 1054 1054 } catch { 1055 1055 return false; 1056 1056 } 1057 + } 1058 + 1059 + /** Poll until `pid` is gone (or `timeoutMs` elapses). Returns true if the 1060 + * process exited within the budget, false if it was still alive at timeout. 1061 + * Used by `pty kill` to wait for the daemon's shutdown (which re-flushes exit 1062 + * metadata) to finish before returning, so a following `pty rm` can't race it. */ 1063 + export async function waitForProcessExit(pid: number, timeoutMs: number): Promise<boolean> { 1064 + const start = Date.now(); 1065 + while (Date.now() - start < timeoutMs) { 1066 + if (!isProcessAlive(pid)) return true; 1067 + await new Promise((r) => setTimeout(r, 50)); 1068 + } 1069 + return !isProcessAlive(pid); 1057 1070 } 1058 1071 1059 1072 function isSocketReachable(socketPath: string): Promise<boolean> {

+90

tests/kill-wait.test.ts

··· 1 + import { describe, it, expect, afterAll } from "vitest"; 2 + import * as fs from "node:fs"; 3 + import * as os from "node:os"; 4 + import * as path from "node:path"; 5 + import { fileURLToPath } from "node:url"; 6 + import { spawnSync } from "node:child_process"; 7 + 8 + const __dirname = path.dirname(fileURLToPath(import.meta.url)); 9 + const nodeBin = process.execPath; 10 + const cliPath = path.join(__dirname, "..", "dist", "cli.js"); 11 + const testRoot = fs.mkdtempSync(path.join(os.tmpdir(), "pty-killw-")); 12 + const bgPids: number[] = []; 13 + afterAll(() => { 14 + for (const pid of bgPids) { try { process.kill(pid, "SIGTERM"); } catch {} } 15 + fs.rmSync(testRoot, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 }); 16 + }); 17 + 18 + function isAlive(pid: number): boolean { 19 + try { process.kill(pid, 0); return true; } catch { return false; } 20 + } 21 + 22 + function runCli(dir: string, args: string[]) { 23 + return spawnSync(nodeBin, [cliPath, ...args], { 24 + env: { ...process.env, PTY_SESSION_DIR: dir, PTY_ROOT_LEGACY_SILENT: "1" }, 25 + encoding: "utf8", timeout: 15000, 26 + }); 27 + } 28 + 29 + /** Create a real detached session (the production path) and return its pid. */ 30 + function createSession(dir: string, name: string): number { 31 + const r = runCli(dir, ["run", "-d", "--id", name, "--", "cat"]); 32 + expect(r.status).toBe(0); 33 + const pid = Number(fs.readFileSync(path.join(dir, `${name}.pid`), "utf8").trim()); 34 + bgPids.push(pid); 35 + return pid; 36 + } 37 + 38 + describe("pty kill waits for the daemon to fully exit", () => { 39 + it("the daemon process is gone by the time `pty kill` returns", () => { 40 + const dir = fs.mkdtempSync(path.join(testRoot, "d-")); 41 + const pid = createSession(dir, "kw"); 42 + expect(isAlive(pid)).toBe(true); 43 + 44 + const r = runCli(dir, ["kill", "kw"]); 45 + expect(r.status).toBe(0); 46 + expect(r.stdout).toContain("killed"); 47 + // The point of the fix: kill blocked until the daemon (and its shutdown 48 + // metadata re-flush) finished, so the pid is already dead here — not racing 49 + // a caller that immediately `pty rm`s the session. 50 + expect(isAlive(pid)).toBe(false); 51 + }, 20000); 52 + 53 + it("a follow-up `pty rm` after kill leaves no stray files (no late-flush race)", () => { 54 + const dir = fs.mkdtempSync(path.join(testRoot, "d-")); 55 + createSession(dir, "kw2"); 56 + 57 + expect(runCli(dir, ["kill", "kw2"]).status).toBe(0); 58 + expect(runCli(dir, ["rm", "kw2"]).status).toBe(0); 59 + 60 + // Daemon fully exited before kill returned, so rm removed the final state 61 + // and nothing (metadata or a `.tmp.*`) can reappear. 62 + const leftovers = fs.readdirSync(dir).filter((f) => f.startsWith("kw2")); 63 + expect(leftovers).toEqual([]); 64 + }, 20000); 65 + 66 + it("daemon shutdown does NOT resurrect metadata that was already removed", async () => { 67 + // The (ii) fix for the watchdog residue: if the session's metadata was 68 + // removed (e.g. `pty rm`) while the daemon is still shutting down, the 69 + // daemon's late exit-metadata re-flush must not re-create the `.json`. 70 + const dir = fs.mkdtempSync(path.join(testRoot, "d-")); 71 + const pid = createSession(dir, "kw3"); 72 + const metaPath = path.join(dir, "kw3.json"); 73 + expect(fs.existsSync(metaPath)).toBe(true); 74 + 75 + // Remove the metadata out from under the running daemon, then trigger its 76 + // shutdown (its exit-metadata write fires during shutdown). 77 + fs.unlinkSync(metaPath); 78 + process.kill(pid, "SIGTERM"); 79 + const start = Date.now(); 80 + while (Date.now() - start < 6000 && isAlive(pid)) { 81 + await new Promise((r) => setTimeout(r, 50)); 82 + } 83 + await new Promise((r) => setTimeout(r, 400)); // grace for any late write 84 + 85 + expect(isAlive(pid)).toBe(false); 86 + // Must stay gone — no resurrected .json, no stray .json.tmp.*. 87 + expect(fs.existsSync(metaPath)).toBe(false); 88 + expect(fs.readdirSync(dir).filter((f) => f.startsWith("kw3.json"))).toEqual([]); 89 + }, 20000); 90 + });

+11 -10

tests/seq-delay.test.ts

··· 72 72 } 73 73 74 74 describe("pty send --seq delay is applied end-to-end", () => { 75 - // 4 items = 3 inter-item gaps → default ≈ 900ms of spacing, plenty above the 76 - // node-startup noise floor once we subtract the straight-stream baseline. 77 - const ITEMS = ["--seq", "a", "--seq", "b", "--seq", "c", "--seq", "d"]; 75 + // 9 items = 8 inter-item gaps. At the 0.3s default that's ~2.4s of real 76 + // inserted delay — a large enough signal that startup / scheduling noise 77 + // under a saturated parallel test run can't close the gap vs the straight 78 + // stream. (The exact per-value delay is proven deterministically by the 79 + // resolveSeqDelayMs unit tests above; this just proves it's wired.) 80 + const ITEMS = ["a", "b", "c", "d", "e", "f", "g", "h", "i"].flatMap((v) => ["--seq", v]); 78 81 79 82 it("default spaces items but --with-delay 0 does not (0 = straight stream)", async () => { 80 83 const dir = fs.mkdtempSync(path.join(testRoot, "d-")); ··· 84 87 const straight = timeSend(dir, [name, "--with-delay", "0", ...ITEMS]); 85 88 const dflt = timeSend(dir, [name, ...ITEMS]); 86 89 87 - // The default adds ~900ms (3 × 0.3s) over the straight stream. Generous 88 - // margin (≥ 600ms) keeps it robust under load. 89 - expect(dflt - straight).toBeGreaterThan(600); 90 - // And the straight stream itself is quick (no 0.9s of inserted delay). 91 - expect(straight).toBeLessThan(dflt - 500); 90 + // The default adds ~2.4s (8 × 0.3s) over the straight stream. Assert well 91 + // below that (≥ 1.2s) so a saturated CI run can't false-fail. 92 + expect(dflt - straight).toBeGreaterThan(1200); 92 93 }, 30000); 93 94 94 95 it("--with-delay N scales the spacing", async () => { ··· 99 100 const straight = timeSend(dir, [name, "--with-delay", "0", ...ITEMS]); 100 101 const slow = timeSend(dir, [name, "--with-delay", "0.2", ...ITEMS]); 101 102 102 - // 3 gaps × 0.2s ≈ 600ms over the straight baseline. 103 - expect(slow - straight).toBeGreaterThan(400); 103 + // 8 gaps × 0.2s ≈ 1.6s over the straight baseline; assert ≥ 0.9s. 104 + expect(slow - straight).toBeGreaterThan(900); 104 105 }, 30000); 105 106 });

Configure Feed

Configure Feed