From d2707bd115d0fd0a40a3bada32f7fe59ce521225 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 11:57:56 +0200 Subject: [PATCH 01/10] Say what the kill verified, not what it hoped `pty kill` signals the daemon and waits for that one pid. It then prints `Session "X" killed.` The child, and everything the child started, is never looked at. The word is a claim about a session made on evidence about a daemon. This takes a snapshot of the daemon's process tree before the signal, and re-checks it after the daemon exits. The success line now appears only when every process in the snapshot is gone. Otherwise the command prints `Session "X" daemon stopped.`, which is the part it verified, and names the survivors on standard error. The snapshot must come first. After the daemon exits its children reparent away, so the links that identify them are gone. The pre-kill snapshot is also taken at a calm moment, while the daemon takes its own during shutdown, so the command can see a process the daemon's teardown skipped. A pid is reported as surviving only when its start token still matches. A token that cannot be read on a process that has not exited is reported separately as undecided, rather than being folded into either answer. A zombie is not a survivor. It answers `kill(pid, 0)` and keeps its start token, so the check reads the process state through `hasProcessExitedForReap`. Two supporting changes: A daemon that could not kill a descendant now appends `session_descendants_survived` to the session event log, and names the pids in its stderr warning. The daemon's stderr has no reader. The Rust tool already writes this event; this closes the gap. On macOS, an empty `ps -o stat=` field no longer counts as a dead process. An empty field means the process is gone or `ps` did not answer, and under load `ps` is the thing that goes quiet. The new check depends on this predicate, so it had to stop reading silence as death. This sends no additional signals. --- CHANGELOG.md | 29 +++++++ docs/disk-layout.md | 1 + src/cli.ts | 16 +++- src/events.ts | 17 ++++ src/kill-report.ts | 85 ++++++++++++++++++++ src/server.ts | 8 +- src/sessions.ts | 23 +++++- tests/kill-report.test.ts | 164 ++++++++++++++++++++++++++++++++++++++ 8 files changed, 339 insertions(+), 4 deletions(-) create mode 100644 src/kill-report.ts create mode 100644 tests/kill-report.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a603cb..b8c41f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ ## Unreleased +### `pty kill` reports what it verified + +- `pty kill` prints `Session "X" killed.` only when the session's process tree + is gone. It takes a snapshot of the tree before it signals the daemon, and + re-checks that snapshot after the daemon exits. Before this, the command + asked about the daemon and reported about the session, so the success line + was a claim about processes it never looked at. +- When something outlives the kill, the command prints + `Session "X" daemon stopped.` on standard output, which is the part it + verified, and names the surviving PIDs on standard error. A PID is called a + survivor only when its process-start token still matches. A PID that has not + exited but whose token cannot be read is reported separately as undecided. +- A daemon that could not kill a descendant now appends + `session_descendants_survived` to the session event log, and its standard + error warning names the PIDs. The daemon's standard error has no reader, so + the log line is the copy a person can find. +- `pty kill` sends no additional signals and waits no longer than before. +- The exit status is unchanged. `pty kill` still exits 0 when the daemon stops, + even with survivors. +- On macOS, an empty `ps -o stat=` field no longer counts as a dead process. + An empty field means the process is gone or `ps` did not answer, and under + load `ps` is the thing that goes quiet, so the kernel is asked again. + ### Complete session termination - `pty kill` now stops the PTY child and its complete descendant tree. A @@ -28,6 +51,12 @@ ### Storage format +- New event type `session_descendants_survived`, carrying `data: { pids }`. + A daemon appends it when it signalled its child's process tree with TERM and + then KILL and found processes still alive. It records what the daemon could + not kill; a process that left the tree before the snapshot is not in it. + + - Supporting live daemons now advertise a `recovery` capability in session metadata. `pty recover --snapshot ` uses that captured capability to authenticate a signal-free listener/registry rebind after an diff --git a/docs/disk-layout.md b/docs/disk-layout.md index 4dd236c..3ca5a16 100644 --- a/docs/disk-layout.md +++ b/docs/disk-layout.md @@ -133,6 +133,7 @@ Envelope: `{ session: string; type: string; ts: string; ...payload }`. Event typ | `session_respawn` | — (`pty gc` respawned a `strategy=permanent` session) | | `session_abandoned` | `reason: "cwd-gone" \| "idle", idleDays?` — (`pty gc` reaped a live permanent session detected as abandoned) | | `session_flapping` | `counter, limit, window` — (`pty gc` flipped a permanent session to `strategy.status=flapping` after N consecutive fast-fail respawns; subsequent ticks skip it) | +| `session_descendants_survived` | `data: { pids }` — a daemon signalled its child's process tree with TERM and then KILL and these processes were still alive. A record of what it could not kill, not a list of everything that outlived the session: a process that left the tree before the snapshot is not in it | | `display_name_change` | `previous: string\|null, value: string\|null` | | `tags_change` | `previous, value` (full snapshots) | | `metadata_change` | `previous, value` containing only changed `displayName` and tag keys; absent tag values are `null` | diff --git a/src/cli.ts b/src/cli.ts index d135714..0986106 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -42,9 +42,12 @@ import { getPidPath, getMetadataPath, DEFAULT_SESSION_DIR, + hasProcessExitedForReap, type SessionInfo, type SessionMetadata, } from "./sessions.ts"; +import { snapshotDescendantProcesses } from "./process-tree.ts"; +import { aftermathOf, killOutcomeLines } from "./kill-report.ts"; import { spawnDaemon, resolveCommand } from "./spawn.ts"; import { acquireEventLock, appendEventSyncLocked, EventFollower, EventWriter, EventType, releaseEventLock, @@ -2637,6 +2640,12 @@ async function cmdKill(name: string): Promise { } catch {} } + // Take the tree BEFORE the signal. After the daemon exits its children are + // reparented to init or a subreaper, so the parent links that identify them + // as this session's processes are gone. This snapshot is the only chance to + // learn which processes the word "killed" would be a claim about. + const before = snapshotDescendantProcesses(session.pid); + try { process.kill(session.pid, "SIGTERM"); } catch { @@ -2662,7 +2671,12 @@ async function cmdKill(name: string): Promise { return; } cleanupSocket(name); - console.log(`Session "${name}" killed.`); + const outcome = killOutcomeLines( + name, + aftermathOf(before, readProcessStartToken, hasProcessExitedForReap), + ); + for (const line of outcome.out) console.log(line); + for (const line of outcome.err) console.error(line); if (wasPermanent && session.metadata?.tags?.ptyfile) { console.error(`Note: this session is managed by ${session.metadata.tags.ptyfile}`); diff --git a/src/events.ts b/src/events.ts index 0e05b1d..265d3e7 100644 --- a/src/events.ts +++ b/src/events.ts @@ -19,6 +19,7 @@ export const EventType = { SESSION_RESPAWN: "session_respawn", SESSION_ABANDONED: "session_abandoned", SESSION_FLAPPING: "session_flapping", + SESSION_DESCENDANTS_SURVIVED: "session_descendants_survived", } as const; export type EventType = (typeof EventType)[keyof typeof EventType]; @@ -85,6 +86,21 @@ export interface SessionExecEvent extends EventBase { command: string; } +/** Emitted by a daemon that signalled its child's process tree with TERM and + * then KILL and found processes still alive. The daemon also warns on its own + * standard error, which has had no reader since the command that launched it + * stopped listening — so this log line is the copy a person can find. + * + * A record, not a guarantee: the daemon reports what it could not kill, and + * cannot report a process that left its tree before the snapshot. */ +export interface SessionDescendantsSurvivedEvent extends EventBase { + type: "session_descendants_survived"; + /** The surviving pids, deepest descendant first. Nested under `data` to + * match the Rust tool byte for byte; both render through the unknown-type + * fallback, so the printed line is identical. */ + data: { pids: number[] }; +} + /** Emitted by `pty gc` whenever it respawns a `strategy=permanent` * session that's exited/vanished. Carries no payload beyond the * envelope — the restart is stateless, there is no attempt counter, @@ -185,6 +201,7 @@ export type EventRecord = | SessionRespawnEvent | SessionAbandonedEvent | SessionFlappingEvent + | SessionDescendantsSurvivedEvent | UserEvent | DisplayNameChangeEvent | TagsChangeEvent diff --git a/src/kill-report.ts b/src/kill-report.ts new file mode 100644 index 0000000..9e9b729 --- /dev/null +++ b/src/kill-report.ts @@ -0,0 +1,85 @@ +/** What `pty kill` may claim, and how it says it. + * + * The command signals the daemon and waits for that one PID. The child, and + * everything the child started, is a separate question. This module answers it + * from a snapshot taken before the signal, and turns the answer into the lines + * the command prints. + * + * Kept out of `cli.ts` because that module runs `main()` on import and cannot + * be loaded by a test. + */ + +import type { ProcessIdentity } from "./process-tree.ts"; + +/** What the pre-kill snapshot looks like once the daemon has gone. */ +export interface Aftermath { + /** The start token still matches, so this is the same process and it is + * still running. */ + survived: number[]; + /** The PID has not exited but its start token could not be read. We cannot + * tell whether it is the same process or a PID the kernel has reused. + * + * This case gets its own list rather than joining either side. Folding it + * into `survived` would invent a survivor; dropping it would repeat the + * defect this module exists to remove, which is a failure to measure + * reported as an answer. */ + unknown: number[]; +} + +export function allGone(after: Aftermath): boolean { + return after.survived.length === 0 && after.unknown.length === 0; +} + +/** Re-check a snapshot against the live process table. + * + * `exited` must be `hasProcessExitedForReap`, not `!isProcessAlive`. A zombie + * answers `kill(pid, 0)` and keeps a readable start token, so the two cheaper + * predicates both call it a survivor. It is a dead process waiting to be + * reaped, and reporting it as still running would be this command + * over-claiming again, only in the other direction. + */ +export function aftermathOf( + before: ProcessIdentity[], + readStartToken: (pid: number) => string | null, + exited: (pid: number) => boolean, +): Aftermath { + const after: Aftermath = { survived: [], unknown: [] }; + for (const identity of before) { + if (exited(identity.pid)) continue; + const token = readStartToken(identity.pid); + if (token === identity.processStartToken) after.survived.push(identity.pid); + // A different token is a PID the kernel handed to somebody else. + else if (token === null) after.unknown.push(identity.pid); + } + return after; +} + +/** Say what was verified, and nothing more. + * + * `killed` is a claim about the whole tree, so it appears only when every + * process in the snapshot is gone. Otherwise standard output carries the part + * that was verified — the daemon stopped — and standard error carries what + * survived it. The two never appear together, so a reader who greps for the + * success line cannot find it beside a warning that contradicts it. + */ +export function killOutcomeLines( + name: string, + after: Aftermath, +): { out: string[]; err: string[] } { + if (allGone(after)) return { out: [`Session "${name}" killed.`], err: [] }; + const err: string[] = []; + if (after.survived.length > 0) { + err.push( + `Session "${name}": ${after.survived.length} process(es) survived the kill ` + + `and are still running: ${after.survived.join(", ")}`, + ); + } + if (after.unknown.length > 0) { + err.push( + `Session "${name}": ${after.unknown.length} process(es) may still be running: ` + + `${after.unknown.join(", ")}. Their start tokens could not be read, so this ` + + "is not a conclusion.", + ); + } + return { out: [`Session "${name}" daemon stopped.`], err }; +} diff --git a/src/server.ts b/src/server.ts index 4f000e0..2b940a8 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1432,10 +1432,16 @@ export class PtyServer { } const survivingDescendants = await descendantsDone; if (survivingDescendants.length > 0) { + const pids = survivingDescendants.map((d) => d.pid); console.error( `pty daemon "${this.name}": ${survivingDescendants.length} child process(es) ` + - "did not exit after exact TERM and KILL signals", + `did not exit after exact TERM and KILL signals: ${pids.join(", ")}`, ); + // And somewhere a person can find it. The warning above goes to this + // daemon's standard error, which has had no reader since the command + // that launched it stopped listening — so the one moment it has + // something worth saying is the one moment nobody is there. + this.emitEvent(EventType.SESSION_DESCENDANTS_SURVIVED, { data: { pids } }); } if (this.exited) await this.saveExitMetadataUntilSettled(this.exitCode); try { await this.eventWriter.flush(); } catch {} diff --git a/src/sessions.ts b/src/sessions.ts index d258217..21961f1 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -806,7 +806,12 @@ type ReapObservedResult = signalled: boolean; }; -function hasProcessExitedForReap(pid: number): boolean { +/** Is `pid` gone for reaping purposes? A zombie counts as exited. + * + * Exported because `pty kill` needs the same question answered. `!isProcessAlive` + * is not a substitute: an unreaped process still answers `kill(pid, 0)` and still + * has a readable start token, so the cheap predicates call a corpse a survivor. */ +export function hasProcessExitedForReap(pid: number): boolean { if (!isProcessAlive(pid)) return true; try { if (process.platform === "linux") { @@ -818,12 +823,26 @@ function hasProcessExitedForReap(pid: number): boolean { encoding: "utf8", timeout: 1000, }).trim(); - return state === "" || state.startsWith("Z"); + return reapedFromPsState(state, () => isProcessAlive(pid)); } catch { return !isProcessAlive(pid); } } +/** Read a `ps -o stat=` field. `stillAlive` is asked only when the field is + * empty, and it is a FRESH answer rather than the one taken before `ps` ran. + * + * An empty field is two answers wearing one shape: the process is gone, or + * `ps` did not manage to say. Reading it as "gone" is a failure folded into an + * answer about what is there — so on an empty field we ask the kernel again + * instead of reading silence as death. Under load on macOS `ps` is exactly + * the thing that goes quiet. */ +export function reapedFromPsState(state: string, stillAlive: () => boolean): boolean { + if (state.startsWith("Z")) return true; + if (state === "") return !stillAlive(); + return false; +} + /** Signal only after proving ownership, then reacquire after daemon shutdown * so its final event/metadata flush cannot recreate artifacts after cleanup. */ async function reapObservedSession( diff --git a/tests/kill-report.test.ts b/tests/kill-report.test.ts new file mode 100644 index 0000000..71bb6b3 --- /dev/null +++ b/tests/kill-report.test.ts @@ -0,0 +1,164 @@ +// `pty kill` used to ask about the daemon and report about the session. These +// tests hold the replacement to the narrower claim: a process is called a +// survivor only when it was measured as one. + +import { describe, expect, it } from "vitest"; +import { spawn } from "node:child_process"; +import { + aftermathOf, + allGone, + killOutcomeLines, + type Aftermath, +} from "../src/kill-report.ts"; +import { hasProcessExitedForReap, reapedFromPsState } from "../src/sessions.ts"; +import { readProcessStartToken } from "../src/recovery.ts"; +import type { ProcessIdentity } from "../src/process-tree.ts"; + +const identity = (pid: number, token: string): ProcessIdentity => ({ + pid, + processStartToken: token, + depth: 1, +}); + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +describe("classifying a pre-kill snapshot", () => { + it("calls a matching start token a survivor", () => { + const after = aftermathOf([identity(10, "tok:10")], () => "tok:10", () => false); + expect(after.survived).toEqual([10]); + expect(after.unknown).toEqual([]); + expect(allGone(after)).toBe(false); + }); + + it("does not call a reused PID a survivor", () => { + const after = aftermathOf([identity(10, "tok:10")], () => "tok:other", () => false); + expect(allGone(after)).toBe(true); + }); + + it("does not report a process that exited", () => { + const after = aftermathOf([identity(10, "tok:10")], () => null, () => true); + expect(allGone(after)).toBe(true); + }); + + // The whole point of the third list: a PID we can see but cannot identify is + // reported as undecided, never silently as dead. + it("reports a live PID with an unreadable token as undecided", () => { + const after = aftermathOf([identity(10, "tok:10")], () => null, () => false); + expect(after.survived).toEqual([]); + expect(after.unknown).toEqual([10]); + expect(allGone(after)).toBe(false); + }); + + it("treats an empty snapshot as nothing to report", () => { + expect(allGone(aftermathOf([], () => null, () => false))).toBe(true); + }); +}); + +describe("classifying against the real process table", () => { + // The mocked cases prove the branching. This one proves the branching is + // about real processes. + it("sees a running process, then stops seeing it", async () => { + const child = spawn("sleep", ["30"], { stdio: "ignore" }); + const pid = child.pid!; + const token = readProcessStartToken(pid); + expect(token).not.toBeNull(); + const before = [identity(pid, token!)]; + + expect(aftermathOf(before, readProcessStartToken, hasProcessExitedForReap).survived) + .toEqual([pid]); + + child.kill("SIGKILL"); + await new Promise((r) => child.once("exit", r)); + await sleep(50); + + expect(allGone(aftermathOf(before, readProcessStartToken, hasProcessExitedForReap))) + .toBe(true); + }); + + // A zombie answers kill(pid, 0) and keeps a readable start token, so the two + // obvious predicates both call it alive. Reporting it as a surviving process + // would be a false alarm. The shell below backgrounds a short sleep and then + // stops itself, so it cannot reap the child. + it("does not call a real zombie a survivor", async () => { + const sh = spawn("sh", ["-c", "sleep 0.1 & echo $! ; kill -STOP $$"], { + stdio: ["ignore", "pipe", "ignore"], + }); + try { + const pid = await new Promise((resolve) => + sh.stdout!.once("data", (d) => resolve(Number(String(d).trim()))), + ); + const token = readProcessStartToken(pid); + expect(token).not.toBeNull(); + const before = [identity(pid, token!)]; + + // Wait for it to become a zombie rather than assuming the timing. + for (let i = 0; i < 100 && !hasProcessExitedForReap(pid); i++) await sleep(10); + + expect(hasProcessExitedForReap(pid)).toBe(true); + expect(readProcessStartToken(pid)).toBe(token); + expect(allGone(aftermathOf(before, readProcessStartToken, hasProcessExitedForReap))) + .toBe(true); + } finally { + sh.kill("SIGKILL"); + } + }); +}); + +describe("an empty ps state field", () => { + // An empty field is two answers wearing one shape. Reading it as "gone" is a + // failure folded into an answer about what is there. + it("asks the kernel again instead of reading silence as death", () => { + expect(reapedFromPsState("", () => true)).toBe(false); + expect(reapedFromPsState("", () => false)).toBe(true); + }); + + it("still reads an explicit zombie state as exited", () => { + expect(reapedFromPsState("Z", () => true)).toBe(true); + expect(reapedFromPsState("Z+", () => true)).toBe(true); + }); + + it("does not call a running process exited", () => { + expect(reapedFromPsState("S", () => true)).toBe(false); + expect(reapedFromPsState("S+", () => false)).toBe(false); + }); +}); + +describe("what the command prints", () => { + const clean: Aftermath = { survived: [], unknown: [] }; + + it("says killed only when the whole tree is gone", () => { + expect(killOutcomeLines("s", clean)).toEqual({ + out: ['Session "s" killed.'], + err: [], + }); + }); + + it("claims only the daemon when something survived", () => { + const lines = killOutcomeLines("s", { survived: [42, 43], unknown: [] }); + expect(lines.out).toEqual(['Session "s" daemon stopped.']); + expect(lines.err[0]).toContain("2 process(es) survived"); + expect(lines.err[0]).toContain("42, 43"); + }); + + it("names an undecided process without deciding", () => { + const lines = killOutcomeLines("s", { survived: [], unknown: [7] }); + expect(lines.out).toEqual(['Session "s" daemon stopped.']); + expect(lines.err[0]).toContain("may still be running"); + expect(lines.err[0]).toContain("is not a conclusion"); + }); + + // A reader who greps for the success line must not find it beside a warning + // that contradicts it. + it("never prints the success line next to a survivor report", () => { + for (const after of [ + clean, + { survived: [1], unknown: [] }, + { survived: [], unknown: [2] }, + { survived: [1], unknown: [2] }, + ]) { + const { out, err } = killOutcomeLines("s", after); + const claimedKilled = out.some((l) => l.includes("killed.")); + expect(claimedKilled && err.length > 0).toBe(false); + } + }); +}); From 45e6fa75f8173b78c7b84c51fa9c2b749e36e41a Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 12:26:26 +0200 Subject: [PATCH 02/10] Make the exit status agree with the words `pty kill` printed a survivor report and exited 0. A caller that reads only the status reached the opposite conclusion from one that reads the output, which leaves the honest line as decoration. It now exits non-zero when anything survived, and when a start token could not be read so the outcome is undecided. "I could not confirm the tree is empty" is not success. This is a compatibility break. A script that checks the status of `pty kill` will fail where it used to pass, because it was passing on a false success. The fix for such a caller is to stop treating an unverified kill as a completed one. Nathan decided the survivor case on 2026-09-03. Silber.cos decided the undecidable one. --- CHANGELOG.md | 8 ++++++-- src/cli.ts | 12 +++++++----- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8c41f2..8dc565f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,8 +19,12 @@ error warning names the PIDs. The daemon's standard error has no reader, so the log line is the copy a person can find. - `pty kill` sends no additional signals and waits no longer than before. -- The exit status is unchanged. `pty kill` still exits 0 when the daemon stops, - even with survivors. +- **Compatibility break.** `pty kill` now exits non-zero when anything survived, + and when a start token could not be read so the outcome is undecided. A script + that checks the status of `pty kill` will fail where it used to pass, because + it was passing on a false success. The fix for such a caller is to stop + treating an unverified kill as a completed one. A verified empty tree still + exits 0. - On macOS, an empty `ps -o stat=` field no longer counts as a dead process. An empty field means the process is gone or `ps` did not answer, and under load `ps` is the thing that goes quiet, so the kernel is asked again. diff --git a/src/cli.ts b/src/cli.ts index 0986106..42f73c2 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -47,7 +47,7 @@ import { type SessionMetadata, } from "./sessions.ts"; import { snapshotDescendantProcesses } from "./process-tree.ts"; -import { aftermathOf, killOutcomeLines } from "./kill-report.ts"; +import { aftermathOf, allGone, killOutcomeLines } from "./kill-report.ts"; import { spawnDaemon, resolveCommand } from "./spawn.ts"; import { acquireEventLock, appendEventSyncLocked, EventFollower, EventWriter, EventType, releaseEventLock, @@ -2671,12 +2671,14 @@ async function cmdKill(name: string): Promise { return; } cleanupSocket(name); - const outcome = killOutcomeLines( - name, - aftermathOf(before, readProcessStartToken, hasProcessExitedForReap), - ); + const after = aftermathOf(before, readProcessStartToken, hasProcessExitedForReap); + const outcome = killOutcomeLines(name, after); for (const line of outcome.out) console.log(line); for (const line of outcome.err) console.error(line); + // Anything left is a failure, and the status says so. `unknown` counts: + // "I could not confirm the tree is empty" is not success, and a caller that + // reads 0 as done would be wrong. + if (!allGone(after)) process.exitCode = 1; if (wasPermanent && session.metadata?.tags?.ptyfile) { console.error(`Note: this session is managed by ${session.metadata.tags.ptyfile}`); From 14c0f213e7a901a204133a6f38ebd9fd733752d9 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 12:33:13 +0200 Subject: [PATCH 03/10] Finish the kill instead of reporting that it did not The daemon tears down the child's tree on its way out, so the teardown races its own exit. Whatever it does not manage is nobody's work after that, and the command that outlives it does nothing about it. `pty kill` now re-reads the process table after the daemon has gone. If anything from its pre-kill snapshot is still alive, it signals the process groups the session left behind, waits, escalates to SIGKILL, reads the table again, and reports what is still there. It never reports the sending. Process groups rather than pids, because a group signal needs no identity. The snapshot drops a descendant whose start token cannot be read, and that process is then never signalled; groups are collected from the raw listing, so it is reached anyway. The blind spot is not solved, it is made irrelevant. A sweep also costs one `ps` in total, against one per descendant for tokens. The daemon's own group is never a target. The pty child calls setsid, so the daemon sits alone in its group and signalling it reaches the daemon only. The running process's own group is never a target either, so the command survives to print its result. A zombie is not a group member. `ps` lists it with its group, so counting it makes the sweep report a group it has already emptied. A test against a real process group found this; inspection did not. --- CHANGELOG.md | 20 +++++ src/cli.ts | 44 +++++++++- src/kill-report.ts | 20 ++++- src/process-groups.ts | 154 ++++++++++++++++++++++++++++++++++ tests/process-groups.test.ts | 155 +++++++++++++++++++++++++++++++++++ 5 files changed, 390 insertions(+), 3 deletions(-) create mode 100644 src/process-groups.ts create mode 100644 tests/process-groups.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dc565f..2a0d01e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,26 @@ ## Unreleased +### `pty kill` finishes the job + +- After the daemon has gone, `pty kill` re-reads the process table. If anything + from its pre-kill snapshot is still alive, it signals the process groups the + session left behind, waits, escalates to SIGKILL, reads the table again, and + reports what is still there. The daemon tears the tree down on its way out, so + its teardown races its own exit; the command outlives the daemon and can + finish the work. +- The sweep targets process groups rather than PIDs, because a group signal + needs no per-process identity. A descendant whose process-start token cannot + be read is dropped from the snapshot and never signalled individually; its + group is still swept. The sweep also costs one `ps` call in total, against one + per descendant for tokens. +- The daemon's own process group is never signalled. The PTY child calls + `setsid`, so the daemon is alone in its group. The group of the process + running `pty kill` is never signalled either, so the command survives to + report. Group id 1 and below are never signalled. +- A zombie no longer counts as a process-group member. `ps` lists it with its + group, so counting it made the sweep report a group it had already emptied. + ### `pty kill` reports what it verified - `pty kill` prints `Session "X" killed.` only when the session's process tree diff --git a/src/cli.ts b/src/cli.ts index 42f73c2..afbb967 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -48,6 +48,10 @@ import { } from "./sessions.ts"; import { snapshotDescendantProcesses } from "./process-tree.ts"; import { aftermathOf, allGone, killOutcomeLines } from "./kill-report.ts"; +import { + groupsInTree, listProcessesWithGroups, membersOfGroups, ownProcessGroup, + parseRows, signalGroup, sweepGroups, +} from "./process-groups.ts"; import { spawnDaemon, resolveCommand } from "./spawn.ts"; import { acquireEventLock, appendEventSyncLocked, EventFollower, EventWriter, EventType, releaseEventLock, @@ -298,6 +302,14 @@ Examples: Terminate a running session's daemon and exact descendant tree. Metadata is kept — restart or \`pty rm\` it later. +The daemon tears the tree down on its way out, which races its own exit. So once +the daemon has gone, \`pty kill\` re-reads the process table, signals any process +group the session left behind, and reports what is still there. It exits non-zero +unless it verified the tree is empty. + +A descendant that calls setsid leaves the session and the group, and neither the +tree walk nor the group sweep can reach it. + Examples: pty kill myserver`, @@ -2618,6 +2630,23 @@ function formatUptime(seconds: number | null): string { return `${d}d ${h % 24}h`; } +/** How long the escalation gives a group to answer SIGTERM before it stops + * asking. A coding agent was measured ignoring SIGTERM for ten seconds, so + * this grace is a courtesy, not a plan. */ +const ESCALATE_TERM_WAIT_MS = 2_000; +/** How long to wait after SIGKILL before reporting what is still there. */ +const ESCALATE_KILL_WAIT_MS = 1_000; + +/** TERM the groups, wait, KILL what is left, wait, then re-read the process + * table. Returns the pids still alive in those groups. */ +async function escalateOverGroups(groups: number[]): Promise { + return sweepGroups(groups, ownProcessGroup(), ESCALATE_TERM_WAIT_MS, ESCALATE_KILL_WAIT_MS, { + live: (targets) => membersOfGroups(targets, parseRows(listProcessesWithGroups())), + signal: signalGroup, + sleep: (ms) => new Promise((r) => setTimeout(r, ms)), + }); +} + async function cmdKill(name: string): Promise { const session = await getSession(name); @@ -2645,6 +2674,10 @@ async function cmdKill(name: string): Promise { // as this session's processes are gone. This snapshot is the only chance to // learn which processes the word "killed" would be a claim about. const before = snapshotDescendantProcesses(session.pid); + // Groups come from the raw listing, NOT from `before`. The snapshot drops a + // descendant whose start token cannot be read, and that process is then never + // signalled. A group needs no identity, so this reaches it anyway. + const groups = groupsInTree(session.pid, parseRows(listProcessesWithGroups())); try { process.kill(session.pid, "SIGTERM"); @@ -2671,8 +2704,15 @@ async function cmdKill(name: string): Promise { return; } cleanupSocket(name); - const after = aftermathOf(before, readProcessStartToken, hasProcessExitedForReap); - const outcome = killOutcomeLines(name, after); + let after = aftermathOf(before, readProcessStartToken, hasProcessExitedForReap); + let escalated: number[] | undefined; + if (!allGone(after)) { + escalated = await escalateOverGroups(groups); + // Re-measure. The report must describe the machine now, not the signals + // that were sent at it. + after = aftermathOf(before, readProcessStartToken, hasProcessExitedForReap); + } + const outcome = killOutcomeLines(name, after, escalated); for (const line of outcome.out) console.log(line); for (const line of outcome.err) console.error(line); // Anything left is a failure, and the status says so. `unknown` counts: diff --git a/src/kill-report.ts b/src/kill-report.ts index 9e9b729..e916b69 100644 --- a/src/kill-report.ts +++ b/src/kill-report.ts @@ -65,9 +65,27 @@ export function aftermathOf( export function killOutcomeLines( name: string, after: Aftermath, + /** Pids still alive after the escalation swept the session's process groups, + * or undefined when no escalation ran. An empty array means it ran and + * cleared everything. */ + escalated?: number[], ): { out: string[]; err: string[] } { - if (allGone(after)) return { out: [`Session "${name}" killed.`], err: [] }; + if (allGone(after)) { + // Say when the escalation was needed. A silent success would hide that the + // daemon's teardown left something behind, which is the fact somebody + // debugging this wants. + const line = escalated + ? `Session "${name}" killed (the escalation stopped the remainder).` + : `Session "${name}" killed.`; + return { out: [line], err: [] }; + } const err: string[] = []; + if (escalated && escalated.length > 0) { + err.push( + `Session "${name}": ${escalated.length} process(es) survived SIGKILL to ` + + `their process group: ${escalated.join(", ")}`, + ); + } if (after.survived.length > 0) { err.push( `Session "${name}": ${after.survived.length} process(es) survived the kill ` + diff --git a/src/process-groups.ts b/src/process-groups.ts new file mode 100644 index 0000000..152285f --- /dev/null +++ b/src/process-groups.ts @@ -0,0 +1,154 @@ +/** Finishing a kill that the daemon could not finish. + * + * The daemon tears the child's tree down on its way out, so the teardown races + * its own exit. The command outlives the daemon, which puts it in the only + * position from which the job can be completed. + * + * Groups rather than pids, because a group signal needs no per-process + * identity. `snapshotDescendantProcesses` drops a descendant whose start token + * cannot be read and never signals it; a group reaches it anyway. The sweep is + * also cheaper — reading start tokens costs one `ps` per descendant on macOS, + * and a sweep costs one `ps` in total. + */ + +import { execFileSync } from "node:child_process"; + +/** One row of `ps -axo pid=,ppid=,pgid=,stat=`. */ +export interface ProcessRow { + pid: number; + ppid: number; + pgid: number; + /** Process state. `ps` lists a zombie with its process group, so without + * this the sweep counts a corpse as a member and reports a group it has + * already emptied. Measured on Linux 2026-09-03: ` Z`. */ + state: string; +} + +export function isZombie(row: ProcessRow): boolean { + return row.state.startsWith("Z"); +} + +export function listProcessesWithGroups(): string { + try { + return execFileSync("ps", ["-axo", "pid=,ppid=,pgid=,stat="], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + timeout: 2_000, + }); + } catch { + return ""; + } +} + +export function parseRows(listing: string): ProcessRow[] { + const rows: ProcessRow[] = []; + for (const line of listing.split("\n")) { + const fields = line.trim().split(/\s+/); + if (fields.length < 3 || fields.length > 4) continue; + const [pid, ppid, pgid] = fields.map(Number); + if (!Number.isInteger(pid) || !Number.isInteger(ppid) || !Number.isInteger(pgid)) continue; + rows.push({ pid, ppid, pgid, state: fields[3] ?? "" }); + } + return rows; +} + +/** Every distinct process group inside `rootPid`'s tree. + * + * The root's own group is excluded. A pty child calls `setsid`, so the daemon + * sits alone in its group and signalling it would reach the daemon and nothing + * else. Measured on Linux, both tools, 2026-09-03. + * + * Deliberately not filtered by start token: that is the whole point. + */ +export function groupsInTree(rootPid: number, rows: ProcessRow[]): number[] { + const children = new Map(); + const pgidOf = new Map(); + for (const r of rows) { + children.set(r.ppid, [...(children.get(r.ppid) ?? []), r.pid]); + pgidOf.set(r.pid, r.pgid); + } + const rootGroup = pgidOf.get(rootPid); + const groups: number[] = []; + const seen = new Set([rootPid]); + const queue = [...(children.get(rootPid) ?? [])]; + while (queue.length > 0) { + const pid = queue.shift()!; + if (seen.has(pid)) continue; + seen.add(pid); + const g = pgidOf.get(pid); + if (g !== undefined && g !== rootGroup && g > 1 && !groups.includes(g)) groups.push(g); + for (const c of children.get(pid) ?? []) queue.push(c); + } + return groups.sort((a, b) => a - b); +} + +/** The live pids that still belong to any of `groups`. A zombie is excluded: + * `ps` still lists it with its group, and counting it would make the sweep + * report a group it has already emptied. */ +export function membersOfGroups(groups: number[], rows: ProcessRow[]): number[] { + return rows + .filter((r) => !isZombie(r) && groups.includes(r.pgid)) + .map((r) => r.pid) + .sort((a, b) => a - b); +} + +export function signalGroup(pgid: number, signal: NodeJS.Signals): void { + if (pgid <= 1) return; + // A negative pid is the documented way to signal a process group. + try { process.kill(-pgid, signal); } catch {} +} + +export function ownProcessGroup(): number { + // `pgid` of self. Node has no getpgrp binding, so ask ps about our own pid. + try { + const out = execFileSync("ps", ["-o", "pgid=", "-p", String(process.pid)], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + timeout: 1_000, + }).trim(); + const n = Number(out); + return Number.isInteger(n) ? n : -1; + } catch { + return -1; + } +} + +export interface SweepDeps { + live: (groups: number[]) => number[]; + signal: (pgid: number, signal: NodeJS.Signals) => void; + sleep: (ms: number) => Promise; +} + +/** TERM every group, wait, KILL what is left, wait, then say what is STILL + * there. The caller reports the return value; it never reports the sending. + * + * `ownGroup` is skipped so the command survives to print its own result. + */ +export async function sweepGroups( + groups: number[], + ownGroup: number, + termWaitMs: number, + killWaitMs: number, + deps: SweepDeps, +): Promise { + const targets = groups.filter((g) => g > 1 && g !== ownGroup); + if (targets.length === 0) return deps.live(groups); + + for (const g of targets) deps.signal(g, "SIGTERM"); + let remaining = await waitFor(targets, termWaitMs, deps); + if (remaining.length === 0) return []; + + for (const g of targets) deps.signal(g, "SIGKILL"); + remaining = await waitFor(targets, killWaitMs, deps); + return remaining; +} + +async function waitFor(targets: number[], budgetMs: number, deps: SweepDeps): Promise { + const deadline = Date.now() + budgetMs; + let remaining = deps.live(targets); + while (remaining.length > 0 && Date.now() < deadline) { + await deps.sleep(25); + remaining = deps.live(targets); + } + return remaining; +} diff --git a/tests/process-groups.test.ts b/tests/process-groups.test.ts new file mode 100644 index 0000000..d538e1a --- /dev/null +++ b/tests/process-groups.test.ts @@ -0,0 +1,155 @@ +// The escalation that finishes a kill the daemon could not finish. These tests +// check the machine after the signals, never the signals themselves. + +import { describe, expect, it } from "vitest"; +import { spawn } from "node:child_process"; +import { + groupsInTree, + listProcessesWithGroups, + membersOfGroups, + ownProcessGroup, + parseRows, + signalGroup, + sweepGroups, + type ProcessRow, +} from "../src/process-groups.ts"; +import { snapshotDescendantProcesses } from "../src/process-tree.ts"; + +// daemon 100 (its own group), pty child 200 (setsid: its own group and +// session), 300 under the child, and 400 in a background group of its own. +// 900 is unrelated. This is the shape measured on Linux for both tools on +// 2026-09-03. +const ROWS = ["100 1 100 Ss", "200 100 200 Ss", "300 200 200 S", "400 300 400 S", "900 1 900 S"].join("\n"); + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); +const live = (groups: number[]) => membersOfGroups(groups, parseRows(listProcessesWithGroups())); + +describe("choosing which process groups to sweep", () => { + it("never targets the daemon's own group", () => { + const groups = groupsInTree(100, parseRows(ROWS)); + // The pty child calls setsid, so the daemon sits alone in its group and + // signalling it would reach the daemon and nothing else. + expect(groups).not.toContain(100); + expect(groups).toEqual([200, 400]); + }); + + it("never targets an unrelated group", () => { + expect(groupsInTree(100, parseRows(ROWS))).not.toContain(900); + }); + + // The reason to sweep groups at all. + it("still targets the group of a descendant whose start token cannot be read", () => { + const snapshot = snapshotDescendantProcesses(100, { + listProcesses: () => ROWS.split("\n").map((l) => l.split(/\s+/).slice(0, 2).join(" ")).join("\n"), + readStartToken: (pid) => (pid === 400 ? null : `tok:${pid}`), + }); + expect(snapshot.some((i) => i.pid === 400)).toBe(false); + expect(groupsInTree(100, parseRows(ROWS))).toContain(400); + }); + + it("reads members back by group", () => { + const rows = parseRows(ROWS); + expect(membersOfGroups([200], rows)).toEqual([200, 300]); + expect(membersOfGroups([], rows)).toEqual([]); + }); + + // `ps` lists a zombie with its process group. Counting it would make the + // sweep report a group it has already emptied, and then signal it again. + it("does not count a zombie as a group member", () => { + const rows: ProcessRow[] = parseRows("100 1 100 Ss\n200 100 200 Sl\n300 200 200 Z"); + expect(membersOfGroups([200], rows)).toEqual([200]); + }); + + it("ignores a malformed listing row rather than guessing", () => { + expect(parseRows("1 2\nx y z\n7 8 9 S\n")).toEqual([{ pid: 7, ppid: 8, pgid: 9, state: "S" }]); + }); +}); + +describe("the sweep", () => { + function fake(alive: number[][]) { + const sent: Array<[number, string]> = []; + let step = 0; + return { + sent, + deps: { + live: () => alive[step++] ?? [], + signal: (g: number, s: NodeJS.Signals) => { sent.push([g, s]); }, + sleep: async () => {}, + }, + }; + } + + it("does not kill a group that answers TERM", async () => { + const f = fake([[]]); + expect(await sweepGroups([200], 5, 0, 0, f.deps)).toEqual([]); + expect(f.sent).toEqual([[200, "SIGTERM"]]); + }); + + // A coding agent was measured ignoring SIGTERM for ten seconds. One TERM and + // hope is already known not to work here. + it("kills a group that ignores TERM", async () => { + const f = fake([[300], []]); + expect(await sweepGroups([200], 5, 0, 0, f.deps)).toEqual([]); + expect(f.sent).toEqual([[200, "SIGTERM"], [200, "SIGKILL"]]); + }); + + it("returns what outlives SIGKILL rather than swallowing it", async () => { + const f = fake([[300], [300]]); + expect(await sweepGroups([200], 5, 0, 0, f.deps)).toEqual([300]); + }); + + it("never signals its own group", async () => { + const f = fake([[]]); + await sweepGroups([200], 200, 0, 0, f.deps); + expect(f.sent).toEqual([]); + }); + + it("never signals group 1 or below", async () => { + const f = fake([[]]); + await sweepGroups([0, 1, -1], 999, 0, 0, f.deps); + expect(f.sent).toEqual([]); + }); +}); + +describe("the sweep against real processes", () => { + // The fake tests prove the ordering. This one proves the ordering is about + // real processes: it builds a real group whose members ignore SIGTERM, runs + // the real sweep, and checks the process table afterwards. + it("kills a real group that ignores SIGTERM and verifies it is gone", async () => { + const child = spawn("setsid", ["sh", "-c", "trap '' TERM; sleep 60 & sleep 60"], { + stdio: "ignore", + }); + const leader = child.pid!; + try { + const deadline = Date.now() + 5_000; + while (live([leader]).length < 2 && Date.now() < deadline) await sleep(25); + expect(live([leader]).length).toBeGreaterThanOrEqual(2); + + // SIGTERM alone must not be enough, or this proves nothing. + signalGroup(leader, "SIGTERM"); + await sleep(400); + expect(live([leader]).length).toBeGreaterThan(0); + + const stillThere = await sweepGroups([leader], ownProcessGroup(), 500, 2_000, { + live, + signal: signalGroup, + sleep, + }); + expect(stillThere).toEqual([]); + expect(live([leader])).toEqual([]); + } finally { + // The leader alone is not the cleanup: if this test fails, its whole + // group is still running and would leak into the next run. + signalGroup(leader, "SIGKILL"); + } + }, 20_000); + + // The command must not signal the group it is running in, or it dies before + // it can report. + it("leaves its own process group alone", async () => { + const own = ownProcessGroup(); + expect(own).toBeGreaterThan(1); + const stillThere = await sweepGroups([own], own, 0, 0, { live, signal: signalGroup, sleep }); + expect(stillThere).toContain(process.pid); + }); +}); From c7063c63d92f8811b8d2f754c1ad238ef1dccf81 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 12:42:57 +0200 Subject: [PATCH 04/10] Do not call a tree empty while the escalation left something running The success line and the exit status read only the pre-kill snapshot. That snapshot drops a descendant whose start token could not be read, and it never contained a process spawned after it was taken. So a process the sweep found and could not kill can be absent from it entirely, and the command would print `killed` and exit 0 over a process that had just survived SIGKILL. That is the defect this command exists to stop making, reintroduced by the escalation that was supposed to end it. Both halves are now required: the snapshot must be clear AND the sweep must have left nothing behind. Found by reading the diff rather than by a failing test, so the test came after; it fails without the fix. --- src/cli.ts | 8 +++----- src/kill-report.ts | 15 ++++++++++++++- tests/kill-report.test.ts | 17 +++++++++++++++++ tests/process-groups.test.ts | 3 ++- 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index afbb967..50f91a6 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -47,7 +47,7 @@ import { type SessionMetadata, } from "./sessions.ts"; import { snapshotDescendantProcesses } from "./process-tree.ts"; -import { aftermathOf, allGone, killOutcomeLines } from "./kill-report.ts"; +import { aftermathOf, allGone, killOutcomeLines, verifiedEmpty } from "./kill-report.ts"; import { groupsInTree, listProcessesWithGroups, membersOfGroups, ownProcessGroup, parseRows, signalGroup, sweepGroups, @@ -2715,10 +2715,8 @@ async function cmdKill(name: string): Promise { const outcome = killOutcomeLines(name, after, escalated); for (const line of outcome.out) console.log(line); for (const line of outcome.err) console.error(line); - // Anything left is a failure, and the status says so. `unknown` counts: - // "I could not confirm the tree is empty" is not success, and a caller that - // reads 0 as done would be wrong. - if (!allGone(after)) process.exitCode = 1; + // Anything left is a failure, and the status says so. + if (!verifiedEmpty(after, escalated)) process.exitCode = 1; if (wasPermanent && session.metadata?.tags?.ptyfile) { console.error(`Note: this session is managed by ${session.metadata.tags.ptyfile}`); diff --git a/src/kill-report.ts b/src/kill-report.ts index e916b69..c5bf3d5 100644 --- a/src/kill-report.ts +++ b/src/kill-report.ts @@ -30,6 +30,19 @@ export function allGone(after: Aftermath): boolean { return after.survived.length === 0 && after.unknown.length === 0; } +/** Did the command verify that nothing is left? + * + * **Both halves are required.** `Aftermath` only describes the processes that + * were in the pre-kill snapshot, and the snapshot drops anything whose start + * token could not be read. A process the sweep found and could not kill may + * therefore be absent from `after` entirely. Reading `after` alone would print + * the success line over a process that just survived SIGKILL, which is the + * defect this command exists to stop making. + */ +export function verifiedEmpty(after: Aftermath, escalated?: number[]): boolean { + return allGone(after) && (escalated === undefined || escalated.length === 0); +} + /** Re-check a snapshot against the live process table. * * `exited` must be `hasProcessExitedForReap`, not `!isProcessAlive`. A zombie @@ -70,7 +83,7 @@ export function killOutcomeLines( * cleared everything. */ escalated?: number[], ): { out: string[]; err: string[] } { - if (allGone(after)) { + if (verifiedEmpty(after, escalated)) { // Say when the escalation was needed. A silent success would hide that the // daemon's teardown left something behind, which is the fact somebody // debugging this wants. diff --git a/tests/kill-report.test.ts b/tests/kill-report.test.ts index 71bb6b3..24c3801 100644 --- a/tests/kill-report.test.ts +++ b/tests/kill-report.test.ts @@ -8,6 +8,7 @@ import { aftermathOf, allGone, killOutcomeLines, + verifiedEmpty, type Aftermath, } from "../src/kill-report.ts"; import { hasProcessExitedForReap, reapedFromPsState } from "../src/sessions.ts"; @@ -147,6 +148,22 @@ describe("what the command prints", () => { expect(lines.err[0]).toContain("is not a conclusion"); }); + // A process the sweep could not kill need not appear in Aftermath at all: the + // snapshot drops anything whose start token could not be read, and a process + // spawned after the snapshot was never in it. Reading the snapshot alone would + // print the success line over a process that just survived SIGKILL. + it("never calls a tree empty while the escalation left something running", () => { + expect(allGone(clean)).toBe(true); + expect(verifiedEmpty(clean, [4321])).toBe(false); + expect(verifiedEmpty(clean, [])).toBe(true); + expect(verifiedEmpty(clean, undefined)).toBe(true); + expect(verifiedEmpty({ survived: [1], unknown: [] }, [])).toBe(false); + + const lines = killOutcomeLines("s", clean, [4321]); + expect(lines.out.some((l) => l.includes("killed"))).toBe(false); + expect(lines.err.join(" ")).toContain("survived SIGKILL"); + }); + // A reader who greps for the success line must not find it beside a warning // that contradicts it. it("never prints the success line next to a survivor report", () => { diff --git a/tests/process-groups.test.ts b/tests/process-groups.test.ts index d538e1a..9003bd8 100644 --- a/tests/process-groups.test.ts +++ b/tests/process-groups.test.ts @@ -21,7 +21,8 @@ import { snapshotDescendantProcesses } from "../src/process-tree.ts"; // 2026-09-03. const ROWS = ["100 1 100 Ss", "200 100 200 Ss", "300 200 200 S", "400 300 400 S", "900 1 900 S"].join("\n"); -const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); +const sleep = (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, ms)); const live = (groups: number[]) => membersOfGroups(groups, parseRows(listProcessesWithGroups())); describe("choosing which process groups to sweep", () => { From f694899c13097e720a503b93e7f0ce80d22d5bb5 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 13:33:09 +0200 Subject: [PATCH 05/10] Read the process table in one place, and stop spawning ps on Linux Every caller that needed a fact about a process ran its own `ps` and treated the output as fact. A subprocess can be slow, truncated or silent, and all three look exactly like "the process is gone". Node cannot make the syscalls the Rust tool uses, so this does what Node can. On Linux there is now no subprocess at all: `/proc` carries ppid, pgid, state and starttime, which is every fact the callers ask for. On macOS `ps` stays, but the table is read once per operation rather than once per process per poll. In the teardown loop that is the difference between 240 spawns inside a 1500 ms deadline and 60. Silence is a third answer everywhere. Every query separates the fact from "the table was read and this process is not in it" from "I could not find out", with no default and no conversion that turns the last into the middle by accident. Treating silence as death requires calling `orAbsentWhenUnknown`, which greps in one command. That makes the mistake visible rather than impossible. A listing that does not contain the process that read it was truncated, not empty. `ps` always lists itself. `recovery.processStartToken` is untouched and still comes from `ps -o lstart=`. Its exact text is a contract with the Rust tool through a shared registry, so the parser takes the tail verbatim rather than re-joining split fields, which would have rewritten `Wed Sep 3` as `Wed Sep 3`. The in-memory identity is a separate branded type so the two can never be compared. Production `ps` call sites: six to three, none in a per-process poll loop. --- CHANGELOG.md | 20 +++ src/cli.ts | 15 +- src/kill-report.ts | 6 +- src/proc-table.ts | 298 +++++++++++++++++++++++++++++++++++ src/process-groups.ts | 86 +++------- src/process-tree.ts | 91 ++++++----- src/recovery.ts | 15 ++ src/server.ts | 4 + src/sessions.ts | 23 ++- tests/kill-report.test.ts | 31 ++-- tests/proc-table.test.ts | 132 ++++++++++++++++ tests/process-groups.test.ts | 37 ++--- tests/process-tree.test.ts | 68 +++++--- 13 files changed, 645 insertions(+), 181 deletions(-) create mode 100644 src/proc-table.ts create mode 100644 tests/proc-table.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a0d01e..c2ce7fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,26 @@ ## Unreleased +### One reader for the process table + +- Process facts now come from one module. On Linux it reads `/proc` and spawns + nothing at all. On macOS `ps` remains, but it is read once per operation + rather than once per process per poll — the difference between 240 spawns + inside a 1500 ms deadline and 60. +- Every query separates three answers: the fact, "the table was read and this + process is not in it", and "I could not find out". A `ps` that is slow, + truncated or silent now produces the third rather than the second. There is + no default and no conversion that turns silence into absence by accident. +- A listing that does not contain the process that read it is treated as + truncated rather than as an empty machine. `ps` always lists at least itself. +- `recovery.processStartToken` is unchanged and still comes from + `ps -o lstart=`. Its exact text, including the two spaces before a + single-digit day, is a contract with the Rust tool through a shared registry. + The in-memory identity used by the teardown is a separate branded type so the + two cannot be compared by accident. +- Production `ps` call sites: six down to three, and none of them inside a + per-process poll loop. + ### `pty kill` finishes the job - After the daemon has gone, `pty kill` re-reads the process table. If anything diff --git a/src/cli.ts b/src/cli.ts index 50f91a6..03b194b 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -49,9 +49,9 @@ import { import { snapshotDescendantProcesses } from "./process-tree.ts"; import { aftermathOf, allGone, killOutcomeLines, verifiedEmpty } from "./kill-report.ts"; import { - groupsInTree, listProcessesWithGroups, membersOfGroups, ownProcessGroup, - parseRows, signalGroup, sweepGroups, + groupsInTree, membersOfGroups, ownProcessGroup, signalGroup, sweepGroups, } from "./process-groups.ts"; +import { openSource, valueOf } from "./proc-table.ts"; import { spawnDaemon, resolveCommand } from "./spawn.ts"; import { acquireEventLock, appendEventSyncLocked, EventFollower, EventWriter, EventType, releaseEventLock, @@ -2641,7 +2641,7 @@ const ESCALATE_KILL_WAIT_MS = 1_000; * table. Returns the pids still alive in those groups. */ async function escalateOverGroups(groups: number[]): Promise { return sweepGroups(groups, ownProcessGroup(), ESCALATE_TERM_WAIT_MS, ESCALATE_KILL_WAIT_MS, { - live: (targets) => membersOfGroups(targets, parseRows(listProcessesWithGroups())), + live: (targets) => membersOfGroups(targets, openSource()), signal: signalGroup, sleep: (ms) => new Promise((r) => setTimeout(r, ms)), }); @@ -2677,7 +2677,7 @@ async function cmdKill(name: string): Promise { // Groups come from the raw listing, NOT from `before`. The snapshot drops a // descendant whose start token cannot be read, and that process is then never // signalled. A group needs no identity, so this reaches it anyway. - const groups = groupsInTree(session.pid, parseRows(listProcessesWithGroups())); + const groups = groupsInTree(session.pid, openSource()); try { process.kill(session.pid, "SIGTERM"); @@ -2710,7 +2710,12 @@ async function cmdKill(name: string): Promise { escalated = await escalateOverGroups(groups); // Re-measure. The report must describe the machine now, not the signals // that were sent at it. - after = aftermathOf(before, readProcessStartToken, hasProcessExitedForReap); + const again = openSource(); + after = aftermathOf( + before, + (pid) => valueOf(again.identity(pid)), + hasProcessExitedForReap, + ); } const outcome = killOutcomeLines(name, after, escalated); for (const line of outcome.out) console.log(line); diff --git a/src/kill-report.ts b/src/kill-report.ts index c5bf3d5..39253cb 100644 --- a/src/kill-report.ts +++ b/src/kill-report.ts @@ -53,14 +53,14 @@ export function verifiedEmpty(after: Aftermath, escalated?: number[]): boolean { */ export function aftermathOf( before: ProcessIdentity[], - readStartToken: (pid: number) => string | null, + readIdentity: (pid: number) => string | null, exited: (pid: number) => boolean, ): Aftermath { const after: Aftermath = { survived: [], unknown: [] }; for (const identity of before) { if (exited(identity.pid)) continue; - const token = readStartToken(identity.pid); - if (token === identity.processStartToken) after.survived.push(identity.pid); + const token = readIdentity(identity.pid); + if (token === identity.identity) after.survived.push(identity.pid); // A different token is a PID the kernel handed to somebody else. else if (token === null) after.unknown.push(identity.pid); } diff --git a/src/proc-table.ts b/src/proc-table.ts new file mode 100644 index 0000000..c701982 --- /dev/null +++ b/src/proc-table.ts @@ -0,0 +1,298 @@ +/** One reader for the process table. + * + * Three wrong answers came from the same shape: a caller ran its own `ps` and + * treated whatever came back as fact. `ps` is a subprocess. It can be slow, + * truncated, or silent, and each of those looked exactly like "the process is + * gone". + * + * Node cannot make the syscalls the Rust tool uses, so this does what Node can: + * + * **On Linux there is no subprocess at all.** `/proc` carries ppid, pgid, state + * and starttime, which is every fact the callers ask for. + * + * **On macOS `ps` stays, but it is read once per operation** rather than once + * per process per poll. That is the difference between 240 spawns inside a + * 1500 ms deadline and 60. + * + * **And silence is its own answer everywhere.** Every query returns an + * {@link Answer}, which separates "the table says this process is not there" + * from "I could not find out". There is deliberately no default and no + * conversion that lets the second quietly become the first. + */ + +import { execFileSync } from "node:child_process"; +import * as fs from "node:fs"; + +/** How long `ps` gets before the table is declared unreadable. Under + * contention `ps` is exactly the thing that goes quiet. */ +const PS_TIMEOUT_MS = 2_000; + +/** Why a fact is not available. None of these mean the process is gone. */ +export type Unknown = + /** The table could not be read: `ps` failed, timed out, returned nothing, or + * returned something that did not contain this very process. */ + | "table-unreadable" + /** The table has the process, but this column was empty. */ + | "field-empty"; + +/** What the process table said about one thing. + * + * **`unknown` is not `not-present`.** Folding them together is the defect this + * type exists to prevent, so there is no default and no direct unwrap. A + * caller that genuinely wants silence to mean death calls + * {@link orAbsentWhenUnknown}, which is named so it shows up in a review and + * in a grep. */ +export type Answer = + | { readonly kind: "known"; readonly value: T } + | { readonly kind: "not-present" } + | { readonly kind: "unknown"; readonly reason: Unknown }; + +export const known = (value: T): Answer => ({ kind: "known", value }); +export const notPresent = (): Answer => ({ kind: "not-present" }); +export const unknown = (reason: Unknown): Answer => ({ kind: "unknown", reason }); + +/** The value if the table knew it. Silence and absence both yield null, so + * this is for callers that have decided the difference does not matter. */ +export function valueOf(a: Answer): T | null { + return a.kind === "known" ? a.value : null; +} + +/** Is the process definitely gone? Only `not-present` says so. An unreadable + * table never does. */ +export function isDefinitelyAbsent(a: Answer): boolean { + return a.kind === "not-present"; +} + +/** Deliberately treat silence as absence. Sometimes that is right. It is never + * the right default, which is why it has a long name. */ +export function orAbsentWhenUnknown(a: Answer): Answer { + return a.kind === "unknown" ? notPresent() : a; +} + +/** A process identity that is only ever compared with another one taken from + * the same run. + * + * **This is deliberately not the same type as the registry's + * `recovery.processStartToken`, and it must never be compared with it.** That + * token is written into session metadata, read by the Rust tool from the same + * registry, and its exact text — including the two spaces `ps` puts before a + * single-digit day — is a contract between the two. This one is private to a + * single command's lifetime. The brand is what stops them meeting. */ +export type LiveIdentity = string & { readonly __liveIdentity: unique symbol }; +export const liveIdentity = (value: string): LiveIdentity => value as LiveIdentity; + +/** One process, as the table saw it. */ +export interface Row { + pid: number; + ppid: number; + pgid: number; + /** `ps` state letters or the `/proc` state character. Empty if unknown. */ + state: string; + rssKb: number | null; + cpuPercent: number | null; + identity: LiveIdentity | null; +} + +export const isZombie = (row: Row): boolean => row.state.startsWith("Z"); + +/** Where process facts come from. + * + * Two implementations, chosen by which is cheaper on the platform. On Linux a + * single-process read is a small file read, so asking per process beats + * re-reading the machine. On macOS every read is a subprocess, so one snapshot + * serves the whole iteration. */ +export interface ProcessSource { + row(pid: number): Answer; + identity(pid: number): Answer; + /** Alive and not a corpse awaiting reaping. */ + isRunning(pid: number): Answer; + /** Every row, for callers that need the whole tree. */ + rows(): Answer; +} + +const onLinux = process.platform === "linux"; + +/** Open a source for one iteration of work. + * + * On macOS this spawns `ps` once. On Linux it spawns nothing and reads + * lazily. Call it once per iteration, not once per question. */ +export function openSource(): ProcessSource { + return onLinux ? new DirectSource() : new SnapshotSource(readPsTable()); +} + +/** Linux: answer each question from `/proc` as it is asked. */ +class DirectSource implements ProcessSource { + row(pid: number): Answer { + if (!Number.isSafeInteger(pid) || pid <= 0) return notPresent(); + let stat: string; + try { + stat = fs.readFileSync(`/proc/${pid}/stat`, "utf8"); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code === "ENOENT" || code === "ESRCH") return notPresent(); + // Permission denied, or /proc not mounted. We did not find out. + return unknown("table-unreadable"); + } + const row = parseProcStat(pid, stat); + return row ? known(row) : unknown("field-empty"); + } + + identity(pid: number): Answer { + return mapAnswer(this.row(pid), (r) => r.identity); + } + + isRunning(pid: number): Answer { + const r = this.row(pid); + return r.kind === "known" ? known(!isZombie(r.value)) : (r as Answer); + } + + rows(): Answer { + let names: string[]; + try { + names = fs.readdirSync("/proc"); + } catch { + return unknown("table-unreadable"); + } + const out: Row[] = []; + for (const name of names) { + if (!/^\d+$/.test(name)) continue; + const pid = Number(name); + try { + const row = parseProcStat(pid, fs.readFileSync(`/proc/${pid}/stat`, "utf8")); + if (row) out.push(row); + } catch { + // Exited between the readdir and the read. That is a real absence. + } + } + if (!out.some((r) => r.pid === process.pid)) return unknown("table-unreadable"); + return known(out); + } +} + +/** macOS and elsewhere: one `ps` listing, answered from memory. */ +class SnapshotSource implements ProcessSource { + private readonly byPid: Map | null; + + constructor(rows: Row[] | null) { + this.byPid = rows === null ? null : new Map(rows.map((r) => [r.pid, r])); + } + + row(pid: number): Answer { + if (this.byPid === null) return unknown("table-unreadable"); + const r = this.byPid.get(pid); + return r ? known(r) : notPresent(); + } + + identity(pid: number): Answer { + return mapAnswer(this.row(pid), (r) => r.identity); + } + + isRunning(pid: number): Answer { + const r = this.row(pid); + return r.kind === "known" ? known(!isZombie(r.value)) : (r as Answer); + } + + rows(): Answer { + return this.byPid === null ? unknown("table-unreadable") : known([...this.byPid.values()]); + } +} + +function mapAnswer(a: Answer, f: (v: T) => U | null): Answer { + if (a.kind !== "known") return a as Answer; + const v = f(a.value); + return v === null ? unknown("field-empty") : known(v); +} + +/** `ps -axo pid=,ppid=,pgid=,state=,rss=,pcpu=,lstart=`, or null if the table + * could not be read. + * + * **An empty or self-omitting listing is an unreadable table, not an empty + * machine.** `ps` always lists at least the process that ran it, so a listing + * without our own pid was truncated or never produced. That one comparison is + * what turns a silent `ps` into "unknown" instead of "everything is dead". */ +export function parsePsListing(listing: string, mustContain = process.pid): Row[] | null { + const rows: Row[] = []; + for (const line of listing.split("\n")) { + const row = parsePsRow(line); + if (row) rows.push(row); + } + return rows.some((r) => r.pid === mustContain) ? rows : null; +} + +function readPsTable(): Row[] | null { + let listing: string; + try { + listing = execFileSync("ps", ["-axo", "pid=,ppid=,pgid=,state=,rss=,pcpu=,lstart="], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + timeout: PS_TIMEOUT_MS, + maxBuffer: 8 * 1024 * 1024, + }); + } catch { + return null; + } + return parsePsListing(listing); +} + +function parsePsRow(line: string): Row | null { + // The tail is taken as raw text rather than re-joined from split fields. + // `ps -o lstart=` pads a single-digit day with two spaces, and re-joining + // would quietly rewrite `Wed Sep 3` as `Wed Sep 3`. + const m = line.match(/^\s*(\d+)\s+(\d+)\s+(\d+)(?:\s+(\S+))?(?:\s+(\S+))?(?:\s+(\S+))?(?:\s+(.*))?$/); + if (!m) return null; + const lstart = (m[7] ?? "").trim(); + return { + pid: Number(m[1]), + ppid: Number(m[2]), + pgid: Number(m[3]), + state: m[4] ?? "", + rssKb: m[5] !== undefined && /^\d+$/.test(m[5]) ? Number(m[5]) : null, + cpuPercent: m[6] !== undefined && !Number.isNaN(Number(m[6])) ? Number(m[6]) : null, + identity: lstart ? liveIdentity(`darwin:${lstart}`) : null, + }; +} + +/** `/proc//stat`. Field 2 is the comm in parentheses and may contain + * spaces and brackets, so everything is read relative to the LAST `)`. */ +export function parseProcStat(pid: number, stat: string): Row | null { + const close = stat.lastIndexOf(")"); + if (close < 0) return null; + const f = stat.slice(close + 1).trim().split(/\s+/); + // f[0] is field 3 (state), so field N is f[N - 3]. + if (f.length < 20) return null; + const ppid = Number(f[1]); + const pgid = Number(f[2]); + const startTime = f[19]; + if (!Number.isSafeInteger(ppid) || !Number.isSafeInteger(pgid) || !startTime) return null; + return { + pid, + ppid, + pgid, + state: f[0] ?? "", + rssKb: null, + cpuPercent: null, + identity: liveIdentity(`linux:${startTime}`), + }; +} + +/** A source built from `pid ppid pgid [state] [identity]` lines, for tests that + * care about tree shape rather than about reading a real machine. A literal + * `-` in the identity column means the table had the process but could not + * name it. */ +export function sourceFromShape(spec: string): ProcessSource { + const rows: Row[] = []; + for (const line of spec.split("\n")) { + const f = line.trim().split(/\s+/).filter(Boolean); + if (f.length < 3) continue; + rows.push({ + pid: Number(f[0]), + ppid: Number(f[1]), + pgid: Number(f[2]), + state: f[3] ?? "S", + rssKb: null, + cpuPercent: null, + identity: f[4] === "-" ? null : liveIdentity(f[4] ?? `tok:${f[0]}`), + }); + } + return new SnapshotSource(rows); +} diff --git a/src/process-groups.ts b/src/process-groups.ts index 152285f..4ef2527 100644 --- a/src/process-groups.ts +++ b/src/process-groups.ts @@ -13,44 +13,17 @@ import { execFileSync } from "node:child_process"; -/** One row of `ps -axo pid=,ppid=,pgid=,stat=`. */ -export interface ProcessRow { - pid: number; - ppid: number; - pgid: number; - /** Process state. `ps` lists a zombie with its process group, so without - * this the sweep counts a corpse as a member and reports a group it has - * already emptied. Measured on Linux 2026-09-03: ` Z`. */ - state: string; -} - -export function isZombie(row: ProcessRow): boolean { - return row.state.startsWith("Z"); -} - -export function listProcessesWithGroups(): string { - try { - return execFileSync("ps", ["-axo", "pid=,ppid=,pgid=,stat="], { - encoding: "utf8", - stdio: ["ignore", "pipe", "ignore"], - timeout: 2_000, - }); - } catch { - return ""; - } -} +import { + isZombie, + openSource, + valueOf, + type ProcessSource, + type Row, +} from "./proc-table.ts"; +import { walkTree } from "./process-tree.ts"; -export function parseRows(listing: string): ProcessRow[] { - const rows: ProcessRow[] = []; - for (const line of listing.split("\n")) { - const fields = line.trim().split(/\s+/); - if (fields.length < 3 || fields.length > 4) continue; - const [pid, ppid, pgid] = fields.map(Number); - if (!Number.isInteger(pid) || !Number.isInteger(ppid) || !Number.isInteger(pgid)) continue; - rows.push({ pid, ppid, pgid, state: fields[3] ?? "" }); - } - return rows; -} +export type { Row as ProcessRow } from "./proc-table.ts"; +export { openSource } from "./proc-table.ts"; /** Every distinct process group inside `rootPid`'s tree. * @@ -60,24 +33,14 @@ export function parseRows(listing: string): ProcessRow[] { * * Deliberately not filtered by start token: that is the whole point. */ -export function groupsInTree(rootPid: number, rows: ProcessRow[]): number[] { - const children = new Map(); - const pgidOf = new Map(); - for (const r of rows) { - children.set(r.ppid, [...(children.get(r.ppid) ?? []), r.pid]); - pgidOf.set(r.pid, r.pgid); - } +export function groupsInTree(rootPid: number, source: ProcessSource): number[] { + const rows = valueOf(source.rows()) ?? ([] as Row[]); + const pgidOf = new Map(rows.map((r) => [r.pid, r.pgid])); const rootGroup = pgidOf.get(rootPid); const groups: number[] = []; - const seen = new Set([rootPid]); - const queue = [...(children.get(rootPid) ?? [])]; - while (queue.length > 0) { - const pid = queue.shift()!; - if (seen.has(pid)) continue; - seen.add(pid); + for (const { pid } of walkTree(rootPid, source)) { const g = pgidOf.get(pid); if (g !== undefined && g !== rootGroup && g > 1 && !groups.includes(g)) groups.push(g); - for (const c of children.get(pid) ?? []) queue.push(c); } return groups.sort((a, b) => a - b); } @@ -85,7 +48,8 @@ export function groupsInTree(rootPid: number, rows: ProcessRow[]): number[] { /** The live pids that still belong to any of `groups`. A zombie is excluded: * `ps` still lists it with its group, and counting it would make the sweep * report a group it has already emptied. */ -export function membersOfGroups(groups: number[], rows: ProcessRow[]): number[] { +export function membersOfGroups(groups: number[], source: ProcessSource): number[] { + const rows = valueOf(source.rows()) ?? ([] as Row[]); return rows .filter((r) => !isZombie(r) && groups.includes(r.pgid)) .map((r) => r.pid) @@ -98,19 +62,11 @@ export function signalGroup(pgid: number, signal: NodeJS.Signals): void { try { process.kill(-pgid, signal); } catch {} } -export function ownProcessGroup(): number { - // `pgid` of self. Node has no getpgrp binding, so ask ps about our own pid. - try { - const out = execFileSync("ps", ["-o", "pgid=", "-p", String(process.pid)], { - encoding: "utf8", - stdio: ["ignore", "pipe", "ignore"], - timeout: 1_000, - }).trim(); - const n = Number(out); - return Number.isInteger(n) ? n : -1; - } catch { - return -1; - } +export function ownProcessGroup(source: ProcessSource = openSource()): number { + // Node has no `getpgrp` binding, so this comes out of the table like every + // other process fact. On Linux that is a `/proc` read and no subprocess. + const row = valueOf(source.row(process.pid)); + return row ? row.pgid : -1; } export interface SweepDeps { diff --git a/src/process-tree.ts b/src/process-tree.ts index 73402c1..c6d8e73 100644 --- a/src/process-tree.ts +++ b/src/process-tree.ts @@ -1,27 +1,27 @@ -import { execFileSync } from "node:child_process"; -import { readProcessStartToken } from "./recovery.ts"; +import { + openSource, + valueOf, + type LiveIdentity, + type ProcessSource, + type Row, +} from "./proc-table.ts"; export interface ProcessIdentity { pid: number; - processStartToken: string; + /** Proof of identity for the length of one command. Not the registry's + * `recovery.processStartToken`: see `proc-table.ts`. */ + identity: LiveIdentity; depth: number; } interface ProcessTreeDeps { - listProcesses?: () => string; - readStartToken?: (pid: number) => string | null; + /** Where process facts come from. Defaults to one source per iteration, + * which on Linux reads `/proc` and on macOS is one `ps` call. */ + source?: () => ProcessSource; signal?: (pid: number, signal: NodeJS.Signals) => void; sleep?: (ms: number) => Promise; } -function listProcesses(): string { - return execFileSync("ps", ["-axo", "pid=,ppid="], { - encoding: "utf8", - stdio: ["ignore", "pipe", "ignore"], - timeout: 2_000, - }); -} - /** Take one parent-chain snapshot before the PTY leader can exit and lose its * descendants to init or a subreaper. Every PID is bound to its process start * identity so later signals cannot target a reused PID. */ @@ -29,42 +29,45 @@ export function snapshotDescendantProcesses( rootPid: number, deps: ProcessTreeDeps = {}, ): ProcessIdentity[] { - const output = (deps.listProcesses ?? listProcesses)(); - const readStartToken = deps.readStartToken ?? readProcessStartToken; - const children = new Map(); - for (const line of output.split("\n")) { - const match = line.trim().match(/^(\d+)\s+(\d+)$/); - if (!match) continue; - const pid = Number(match[1]); - const ppid = Number(match[2]); - const siblings = children.get(ppid) ?? []; - siblings.push(pid); - children.set(ppid, siblings); + const source = (deps.source ?? openSource)(); + const descendants: ProcessIdentity[] = []; + for (const { pid, depth } of walkTree(rootPid, source)) { + const identity = valueOf(source.identity(pid)); + if (identity !== null) descendants.push({ pid, identity, depth }); } + return descendants.sort((a, b) => b.depth - a.depth || b.pid - a.pid); +} - const descendants: ProcessIdentity[] = []; +/** Walk a tree from `rootPid`, breadth first, recording depth. */ +export function walkTree( + rootPid: number, + source: ProcessSource, +): Array<{ pid: number; depth: number }> { + const rows = valueOf(source.rows()) ?? ([] as Row[]); + const children = new Map(); + for (const r of rows) children.set(r.ppid, [...(children.get(r.ppid) ?? []), r.pid]); + const out: Array<{ pid: number; depth: number }> = []; const seen = new Set([rootPid]); const queue = (children.get(rootPid) ?? []).map((pid) => ({ pid, depth: 1 })); while (queue.length > 0) { const current = queue.shift()!; if (seen.has(current.pid)) continue; seen.add(current.pid); - const processStartToken = readStartToken(current.pid); - if (processStartToken !== null) { - descendants.push({ ...current, processStartToken }); - } + out.push(current); for (const pid of children.get(current.pid) ?? []) { queue.push({ pid, depth: current.depth + 1 }); } } - return descendants.sort((a, b) => b.depth - a.depth || b.pid - a.pid); + return out; } -function isSameProcess( - identity: ProcessIdentity, - readStartToken: (pid: number) => string | null, -): boolean { - return readStartToken(identity.pid) === identity.processStartToken; +/** Is this still the same process? + * + * **An unreadable source answers false, and that is deliberate**: it says + * "do not signal", never "it is gone". Every caller here wants the safe + * direction for a signal. */ +function isSameProcess(identity: ProcessIdentity, source: ProcessSource): boolean { + return valueOf(source.identity(identity.pid)) === identity.identity; } /** Signal only identities that still match their snapshot. A token mismatch @@ -74,11 +77,11 @@ export function signalProcessIdentities( signal: NodeJS.Signals, deps: ProcessTreeDeps = {}, ): number[] { - const readStartToken = deps.readStartToken ?? readProcessStartToken; + const source = (deps.source ?? openSource)(); const sendSignal = deps.signal ?? ((pid, value) => process.kill(pid, value)); const signalled: number[] = []; for (const identity of identities) { - if (!isSameProcess(identity, readStartToken)) continue; + if (!isSameProcess(identity, source)) continue; try { sendSignal(identity.pid, signal); signalled.push(identity.pid); @@ -92,13 +95,21 @@ async function waitForIdentitiesToExit( timeoutMs: number, deps: ProcessTreeDeps, ): Promise { - const readStartToken = deps.readStartToken ?? readProcessStartToken; + const openIteration = deps.source ?? openSource; const sleep = deps.sleep ?? ((ms) => new Promise((resolve) => setTimeout(resolve, ms))); const deadline = Date.now() + timeoutMs; - let survivors = identities.filter((identity) => isSameProcess(identity, readStartToken)); + // **One source per iteration, not one question per process.** On macOS every + // question used to be a `ps` spawn: at 25 ms polling inside a 1500 ms budget, + // four descendants cost 240 spawns, and at the 10.9 ms a spawn was measured to + // take that is 2.6 seconds of spawning inside a 1.5 second deadline. The loop + // could not meet its own deadline on an idle machine. It is now one `ps` per + // iteration there, and no subprocess at all on Linux. + let source = openIteration(); + let survivors = identities.filter((identity) => isSameProcess(identity, source)); while (survivors.length > 0 && Date.now() < deadline) { await sleep(25); - survivors = survivors.filter((identity) => isSameProcess(identity, readStartToken)); + source = openIteration(); + survivors = survivors.filter((identity) => isSameProcess(identity, source)); } return survivors; } diff --git a/src/recovery.ts b/src/recovery.ts index ed396bb..b2013cc 100644 --- a/src/recovery.ts +++ b/src/recovery.ts @@ -144,6 +144,21 @@ export function readProcessStartToken(pid: number): string | null { return startTime ? `linux:${startTime}` : null; } if (process.platform === "darwin") { + // **THIS `ps` STAYS, AND IT IS NOT AN OVERSIGHT.** + // + // Everything else moved to `proc-table.ts`, which reads `/proc` on Linux + // and calls `ps` once per operation elsewhere. This one cannot, because + // the text it produces is written into session metadata as + // `recovery.processStartToken` and the Rust tool reads it back from the + // same registry. `ps -o lstart=` output — including the two spaces it + // puts before a single-digit day — is therefore a contract between two + // programs, not an implementation detail. + // + // It is safe where it is: one call per session lookup, never inside a + // poll loop, and a failure here already means "cannot confirm" rather + // than "gone". `LiveIdentity` in `proc-table.ts` is a separate branded + // type so the cheap identity used by the teardown can never be compared + // with this one. const started = execFileSync( "ps", ["-o", "lstart=", "-p", String(pid)], diff --git a/src/server.ts b/src/server.ts index 2b940a8..572639e 100644 --- a/src/server.ts +++ b/src/server.ts @@ -216,6 +216,10 @@ export interface ProcessResources { /** Query CPU and memory usage for a process via ps. Returns null on failure. */ function queryProcessResources(pid: number): ProcessResources | null { try { + // The only per-pid `ps` left in the daemon, and only off Linux. Resident + // set and CPU are not in `/proc//stat` in the form this wants, and a + // stats query is one call for one session rather than one per descendant + // inside a loop. const output = execFileSync("ps", ["-o", "rss=,pcpu=", "-p", String(pid)], { encoding: "utf-8", timeout: 1000, diff --git a/src/sessions.ts b/src/sessions.ts index 21961f1..adb7bc5 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -5,6 +5,7 @@ import * as os from "node:os"; import * as net from "node:net"; import { createHash } from "node:crypto"; import { execFileSync } from "node:child_process"; +import { openSource } from "./proc-table.ts"; import { assertPrivateRecoveryPaths, atomicWritePrivate, @@ -813,20 +814,14 @@ type ReapObservedResult = * has a readable start token, so the cheap predicates call a corpse a survivor. */ export function hasProcessExitedForReap(pid: number): boolean { if (!isProcessAlive(pid)) return true; - try { - if (process.platform === "linux") { - const stat = fs.readFileSync(`/proc/${pid}/stat`, "utf8"); - const stateOffset = stat.lastIndexOf(") ") + 2; - return stateOffset >= 2 && stat[stateOffset] === "Z"; - } - const state = execFileSync("ps", ["-o", "stat=", "-p", String(pid)], { - encoding: "utf8", - timeout: 1000, - }).trim(); - return reapedFromPsState(state, () => isProcessAlive(pid)); - } catch { - return !isProcessAlive(pid); - } + // One `/proc` read on Linux, one `ps` call on macOS — and never one per + // process inside a poll loop, which is what this used to be. + const answer = openSource().isRunning(pid); + if (answer.kind === "known") return !answer.value; + if (answer.kind === "not-present") return true; + // We did not find out. Ask the kernel once more rather than reading our own + // silence as a death. + return !isProcessAlive(pid); } /** Read a `ps -o stat=` field. `stillAlive` is asked only when the field is diff --git a/tests/kill-report.test.ts b/tests/kill-report.test.ts index 24c3801..e31fdd8 100644 --- a/tests/kill-report.test.ts +++ b/tests/kill-report.test.ts @@ -12,12 +12,13 @@ import { type Aftermath, } from "../src/kill-report.ts"; import { hasProcessExitedForReap, reapedFromPsState } from "../src/sessions.ts"; -import { readProcessStartToken } from "../src/recovery.ts"; +import { openSource, valueOf } from "../src/proc-table.ts"; import type { ProcessIdentity } from "../src/process-tree.ts"; +import { liveIdentity } from "../src/proc-table.ts"; const identity = (pid: number, token: string): ProcessIdentity => ({ pid, - processStartToken: token, + identity: liveIdentity(token), depth: 1, }); @@ -61,19 +62,24 @@ describe("classifying against the real process table", () => { it("sees a running process, then stops seeing it", async () => { const child = spawn("sleep", ["30"], { stdio: "ignore" }); const pid = child.pid!; - const token = readProcessStartToken(pid); + const token = valueOf(openSource().identity(pid)); expect(token).not.toBeNull(); const before = [identity(pid, token!)]; - expect(aftermathOf(before, readProcessStartToken, hasProcessExitedForReap).survived) - .toEqual([pid]); + expect( + aftermathOf(before, (p) => valueOf(openSource().identity(p)), hasProcessExitedForReap) + .survived, + ).toEqual([pid]); child.kill("SIGKILL"); await new Promise((r) => child.once("exit", r)); await sleep(50); - expect(allGone(aftermathOf(before, readProcessStartToken, hasProcessExitedForReap))) - .toBe(true); + expect( + allGone( + aftermathOf(before, (p) => valueOf(openSource().identity(p)), hasProcessExitedForReap), + ), + ).toBe(true); }); // A zombie answers kill(pid, 0) and keeps a readable start token, so the two @@ -88,7 +94,7 @@ describe("classifying against the real process table", () => { const pid = await new Promise((resolve) => sh.stdout!.once("data", (d) => resolve(Number(String(d).trim()))), ); - const token = readProcessStartToken(pid); + const token = valueOf(openSource().identity(pid)); expect(token).not.toBeNull(); const before = [identity(pid, token!)]; @@ -96,9 +102,12 @@ describe("classifying against the real process table", () => { for (let i = 0; i < 100 && !hasProcessExitedForReap(pid); i++) await sleep(10); expect(hasProcessExitedForReap(pid)).toBe(true); - expect(readProcessStartToken(pid)).toBe(token); - expect(allGone(aftermathOf(before, readProcessStartToken, hasProcessExitedForReap))) - .toBe(true); + expect(valueOf(openSource().identity(pid))).toBe(token); + expect( + allGone( + aftermathOf(before, (p) => valueOf(openSource().identity(p)), hasProcessExitedForReap), + ), + ).toBe(true); } finally { sh.kill("SIGKILL"); } diff --git a/tests/proc-table.test.ts b/tests/proc-table.test.ts new file mode 100644 index 0000000..7787416 --- /dev/null +++ b/tests/proc-table.test.ts @@ -0,0 +1,132 @@ +// One reader for the process table. These tests are about the three answers +// and about a `ps` that misbehaves, because that is what produced the wrong +// answers this module exists to stop. + +import { describe, expect, it } from "vitest"; +import { + isDefinitelyAbsent, + isZombie, + openSource, + orAbsentWhenUnknown, + parseProcStat, + parsePsListing, + sourceFromShape, + unknown, + valueOf, +} from "../src/proc-table.ts"; +import { spawn } from "node:child_process"; + +const sleep = (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, ms)); + +const me = process.pid; + +describe("the truncation guard", () => { + // `ps` always lists at least the process that ran it. A listing without our + // own pid was truncated or never produced, and reading it as "the machine + // has no processes" is the defect this whole module exists for. + it("treats a listing without our own pid as unreadable, not empty", () => { + expect(parsePsListing("4242 1 4242 S 100 0.0 Wed Sep 3 11:00:00 2026\n")).toBeNull(); + }); + + it("treats an empty listing as unreadable, not empty", () => { + expect(parsePsListing("")).toBeNull(); + expect(parsePsListing(" \n\n")).toBeNull(); + }); + + it("accepts a listing that contains us", () => { + const rows = parsePsListing(`${me} 1 ${me} S 100 0.5 Wed Sep 3 11:00:00 2026\n`); + expect(rows).not.toBeNull(); + expect(rows![0]).toMatchObject({ pid: me, ppid: 1, pgid: me, rssKb: 100, cpuPercent: 0.5 }); + }); + + // `ps -o lstart=` pads a single-digit day with two spaces, and that text is + // the registry's on-disk token. Re-joining split fields would rewrite it. + it("keeps the lstart text exactly as ps printed it", () => { + const rows = parsePsListing(`${me} 1 ${me} S 100 0.5 Wed Sep 3 11:00:00 2026\n`); + expect(rows![0].identity).toBe("darwin:Wed Sep 3 11:00:00 2026"); + }); +}); + +describe("three answers, never two", () => { + const source = sourceFromShape(`${me} 1 ${me} Ss`); + + it("separates a missing process from an unreadable table", () => { + expect(isDefinitelyAbsent(source.isRunning(999_999))).toBe(true); + expect(isDefinitelyAbsent(unknown("table-unreadable"))).toBe(false); + }); + + it("makes treating silence as death something you have to ask for by name", () => { + const silent = unknown("table-unreadable"); + expect(isDefinitelyAbsent(silent)).toBe(false); + expect(isDefinitelyAbsent(orAbsentWhenUnknown(silent))).toBe(true); + }); + + // An empty column is its own answer too: the process is there, `ps` just did + // not say. This is the defect that shipped in `hasProcessExitedForReap`. + it("does not turn an unnamed process into an absent one", () => { + const unnamed = sourceFromShape(`${me} 1 ${me} Ss -`); + const answer = unnamed.identity(me); + expect(answer.kind).toBe("unknown"); + expect(isDefinitelyAbsent(answer)).toBe(false); + }); +}); + +describe("parsing /proc//stat", () => { + // Field 2 is the comm in parentheses and may contain spaces and brackets. + it("reads relative to the last close paren", () => { + const row = parseProcStat(7, `7 (a b) c) S 3 5 ${Array.from({ length: 40 }, (_, i) => i).join(" ")}`); + expect(row).not.toBeNull(); + expect(row!.ppid).toBe(3); + expect(row!.pgid).toBe(5); + expect(row!.state).toBe("S"); + }); + + it("returns null rather than guessing at a short line", () => { + expect(parseProcStat(7, "7 (x) S 1 2")).toBeNull(); + expect(parseProcStat(7, "no parens here")).toBeNull(); + }); +}); + +describe("against the real machine", () => { + it("knows this very process", () => { + const source = openSource(); + const row = valueOf(source.row(me)); + expect(row).not.toBeNull(); + expect(row!.pid).toBe(me); + expect(row!.ppid).toBeGreaterThan(0); + expect(valueOf(source.isRunning(me))).toBe(true); + expect(valueOf(source.identity(me))).toBeTruthy(); + }); + + it("calls a pid that cannot exist definitely absent", () => { + expect(isDefinitelyAbsent(openSource().isRunning(0x7fffffff))).toBe(true); + }); + + // A zombie has a row and answers kill(pid, 0). The table must still say it + // is not running, and must not say it is absent. + it("reports a real zombie as present but not running", async () => { + const sh = spawn("sh", ["-c", "sleep 0.1 & echo $! ; kill -STOP $$"], { + stdio: ["ignore", "pipe", "ignore"], + }); + try { + const pid = await new Promise((resolve) => + sh.stdout!.once("data", (d) => resolve(Number(String(d).trim()))), + ); + let source = openSource(); + for (let i = 0; i < 200; i++) { + source = openSource(); + const row = valueOf(source.row(pid)); + if (row && isZombie(row)) break; + await sleep(10); + } + const row = valueOf(source.row(pid)); + expect(row, "the child never appeared as a zombie").not.toBeNull(); + expect(isZombie(row!)).toBe(true); + expect(valueOf(source.isRunning(pid))).toBe(false); + expect(isDefinitelyAbsent(source.isRunning(pid))).toBe(false); + } finally { + sh.kill("SIGKILL"); + } + }, 20_000); +}); diff --git a/tests/process-groups.test.ts b/tests/process-groups.test.ts index 9003bd8..7d0eec5 100644 --- a/tests/process-groups.test.ts +++ b/tests/process-groups.test.ts @@ -5,29 +5,28 @@ import { describe, expect, it } from "vitest"; import { spawn } from "node:child_process"; import { groupsInTree, - listProcessesWithGroups, membersOfGroups, ownProcessGroup, - parseRows, signalGroup, sweepGroups, - type ProcessRow, } from "../src/process-groups.ts"; +import { openSource, sourceFromShape } from "../src/proc-table.ts"; import { snapshotDescendantProcesses } from "../src/process-tree.ts"; // daemon 100 (its own group), pty child 200 (setsid: its own group and // session), 300 under the child, and 400 in a background group of its own. // 900 is unrelated. This is the shape measured on Linux for both tools on // 2026-09-03. -const ROWS = ["100 1 100 Ss", "200 100 200 Ss", "300 200 200 S", "400 300 400 S", "900 1 900 S"].join("\n"); +const SHAPE = ["100 1 100 Ss", "200 100 200 Ss", "300 200 200 S", "400 300 400 S", "900 1 900 S"].join("\n"); +const shaped = () => sourceFromShape(SHAPE); const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); -const live = (groups: number[]) => membersOfGroups(groups, parseRows(listProcessesWithGroups())); +const live = (groups: number[]) => membersOfGroups(groups, openSource()); describe("choosing which process groups to sweep", () => { it("never targets the daemon's own group", () => { - const groups = groupsInTree(100, parseRows(ROWS)); + const groups = groupsInTree(100, shaped()); // The pty child calls setsid, so the daemon sits alone in its group and // signalling it would reach the daemon and nothing else. expect(groups).not.toContain(100); @@ -35,35 +34,33 @@ describe("choosing which process groups to sweep", () => { }); it("never targets an unrelated group", () => { - expect(groupsInTree(100, parseRows(ROWS))).not.toContain(900); + expect(groupsInTree(100, shaped())).not.toContain(900); }); // The reason to sweep groups at all. it("still targets the group of a descendant whose start token cannot be read", () => { - const snapshot = snapshotDescendantProcesses(100, { - listProcesses: () => ROWS.split("\n").map((l) => l.split(/\s+/).slice(0, 2).join(" ")).join("\n"), - readStartToken: (pid) => (pid === 400 ? null : `tok:${pid}`), - }); + // 400 is the one the table cannot name. + const unnamed = sourceFromShape( + ["100 1 100 Ss", "200 100 200 Ss", "300 200 200 S", "400 300 400 S -", "900 1 900 S"].join("\n"), + ); + const snapshot = snapshotDescendantProcesses(100, { source: () => unnamed }); expect(snapshot.some((i) => i.pid === 400)).toBe(false); - expect(groupsInTree(100, parseRows(ROWS))).toContain(400); + expect(groupsInTree(100, unnamed)).toContain(400); }); it("reads members back by group", () => { - const rows = parseRows(ROWS); - expect(membersOfGroups([200], rows)).toEqual([200, 300]); - expect(membersOfGroups([], rows)).toEqual([]); + expect(membersOfGroups([200], shaped())).toEqual([200, 300]); + expect(membersOfGroups([], shaped())).toEqual([]); }); // `ps` lists a zombie with its process group. Counting it would make the // sweep report a group it has already emptied, and then signal it again. it("does not count a zombie as a group member", () => { - const rows: ProcessRow[] = parseRows("100 1 100 Ss\n200 100 200 Sl\n300 200 200 Z"); - expect(membersOfGroups([200], rows)).toEqual([200]); + const withCorpse = sourceFromShape("100 1 100 Ss\n200 100 200 Sl\n300 200 200 Z"); + expect(membersOfGroups([200], withCorpse)).toEqual([200]); }); - it("ignores a malformed listing row rather than guessing", () => { - expect(parseRows("1 2\nx y z\n7 8 9 S\n")).toEqual([{ pid: 7, ppid: 8, pgid: 9, state: "S" }]); - }); + }); describe("the sweep", () => { diff --git a/tests/process-tree.test.ts b/tests/process-tree.test.ts index 3d0b568..01aa5b5 100644 --- a/tests/process-tree.test.ts +++ b/tests/process-tree.test.ts @@ -5,41 +5,63 @@ import { terminateProcessIdentities, type ProcessIdentity, } from "../src/process-tree.ts"; +import { + liveIdentity, + sourceFromShape, + type ProcessSource, +} from "../src/proc-table.ts"; + +/** A source whose identities can change under the test, so a reused PID can be + * simulated without touching a real machine. */ +function mutableSource(live: Map, shape: string): () => ProcessSource { + const base = sourceFromShape(shape); + return () => ({ + rows: () => base.rows(), + row: (pid) => base.row(pid), + isRunning: (pid) => base.isRunning(pid), + identity: (pid) => { + const v = live.get(pid); + return v === undefined + ? { kind: "not-present" as const } + : { kind: "known" as const, value: liveIdentity(v) }; + }, + }); +} describe("exact descendant process shutdown", () => { it("snapshots only descendants and records depth plus process-start identity", () => { - const tokens = new Map([ - [11, "start-11"], - [12, "start-12"], - [13, "start-13"], - ]); + // 10 is the root; 14 hangs off an unrelated parent and must not appear. const snapshot = snapshotDescendantProcesses(10, { - listProcesses: () => [ - "10 1", - "11 10", - "12 11", - "13 10", - "14 99", - ].join("\n"), - readStartToken: (pid) => tokens.get(pid) ?? null, + source: () => + sourceFromShape( + [ + "10 1 10 Ss start-10", + "11 10 10 S start-11", + "12 11 10 S start-12", + "13 10 10 S start-13", + "14 99 99 S start-14", + ].join("\n"), + ), }); expect(snapshot).toEqual([ - { pid: 12, processStartToken: "start-12", depth: 2 }, - { pid: 13, processStartToken: "start-13", depth: 1 }, - { pid: 11, processStartToken: "start-11", depth: 1 }, + { pid: 12, identity: liveIdentity("start-12"), depth: 2 }, + { pid: 13, identity: liveIdentity("start-13"), depth: 1 }, + { pid: 11, identity: liveIdentity("start-11"), depth: 1 }, ]); }); it("never signals a PID whose process-start identity changed", () => { const identities: ProcessIdentity[] = [ - { pid: 20, processStartToken: "original-20", depth: 1 }, - { pid: 21, processStartToken: "original-21", depth: 1 }, + { pid: 20, identity: liveIdentity("original-20"), depth: 1 }, + { pid: 21, identity: liveIdentity("original-21"), depth: 1 }, ]; const signals: Array<[number, NodeJS.Signals]> = []; + // 20 has been reused by something else; 21 is still itself. + const live = new Map([[20, "reused-20"], [21, "original-21"]]); const signalled = signalProcessIdentities(identities, "SIGTERM", { - readStartToken: (pid) => pid === 20 ? "reused-20" : "original-21", + source: mutableSource(live, "20 1 20\n21 1 21"), signal: (pid, signal) => { signals.push([pid, signal]); }, }); @@ -49,17 +71,17 @@ describe("exact descendant process shutdown", () => { it("uses exact TERM then exact KILL without a process-group signal", async () => { const identities: ProcessIdentity[] = [ - { pid: 30, processStartToken: "start-30", depth: 2 }, - { pid: 31, processStartToken: "start-31", depth: 1 }, + { pid: 30, identity: liveIdentity("start-30"), depth: 2 }, + { pid: 31, identity: liveIdentity("start-31"), depth: 1 }, ]; - const live = new Map(identities.map((identity) => [identity.pid, identity.processStartToken])); + const live = new Map([[30, "start-30"], [31, "start-31"]]); const signals: Array<[number, NodeJS.Signals]> = []; const survivors = await terminateProcessIdentities( identities, { termWaitMs: 0, killWaitMs: 1 }, { - readStartToken: (pid) => live.get(pid) ?? null, + source: mutableSource(live, "30 1 30\n31 1 31"), signal: (pid, signal) => { signals.push([pid, signal]); if (signal === "SIGKILL") live.delete(pid); From 0c78c29b383e85562cefb4626ee8b076d7be8962 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 13:52:41 +0200 Subject: [PATCH 06/10] Do not count a corpse as a surviving descendant An unreaped descendant keeps its `/proc` row and its identity on Linux, so matching on identity alone counted it as alive. The teardown would wait out its whole TERM budget for a process that had already died, and then report it as having survived a SIGKILL. That is the kill over-claiming again, in the other direction. macOS never had this, because `ps` stops listing a process the moment it exits. The two platforms disagreeing is what exposed it. Two test fixes from a real Mac run, reported by Silber.pty: macOS has the `setsid` system call but no `setsid` executable. The real process-group test spawned the binary, so on the one platform where process groups are the whole escalation story, the test could not run at all. It now uses `detached: true`, which is the same thing without the command. The zombie test asserted the Linux mechanism rather than the conclusion. On Linux the corpse keeps a row with state Z; on macOS it is dropped from the listing at once. The test now asserts what every caller depends on, which is the same on both. A single-pid query no longer reads the whole table. `hasProcessExitedForReap` sits inside poll loops that run every 25 ms, and asking about one pid should not pay for every process on the machine. --- src/proc-table.ts | 39 ++++++++++++++++++++++++++++++++++++ src/process-tree.ts | 14 +++++++++++-- src/sessions.ts | 14 +++++++------ tests/proc-table.test.ts | 37 +++++++++++++++++++++------------- tests/process-groups.test.ts | 9 ++++++++- tests/process-tree.test.ts | 27 ++++++++++++++++++++----- 6 files changed, 112 insertions(+), 28 deletions(-) diff --git a/src/proc-table.ts b/src/proc-table.ts index c701982..88b4551 100644 --- a/src/proc-table.ts +++ b/src/proc-table.ts @@ -120,6 +120,45 @@ export function openSource(): ProcessSource { return onLinux ? new DirectSource() : new SnapshotSource(readPsTable()); } +/** One process, without reading the whole table. + * + * A poll loop asking about a single pid should not pay for every process on + * the machine. On Linux this is one small file read and no subprocess. + * + * **On macOS it is still one `ps` per call, and that is Node's floor.** The + * Rust tool makes a syscall here; Node cannot without a native module. What + * this avoids is the larger whole-table listing, not the spawn. + */ +export function processOf(pid: number): Answer { + if (!Number.isSafeInteger(pid) || pid <= 0) return notPresent(); + if (onLinux) return new DirectSource().row(pid); + let out: string; + try { + out = execFileSync( + "ps", + ["-o", "pid=,ppid=,pgid=,state=,rss=,pcpu=,lstart=", "-p", String(pid)], + { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"], timeout: PS_TIMEOUT_MS }, + ); + } catch { + // `ps` exits non-zero when the pid is not there, which is indistinguishable + // from `ps` failing. Ask the kernel, which does distinguish them. + return processExists(pid) ? unknown("table-unreadable") : notPresent(); + } + const row = parsePsRow(out); + if (row) return known(row); + // It ran and said nothing about this pid. + return processExists(pid) ? unknown("field-empty") : notPresent(); +} + +function processExists(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (err) { + return (err as NodeJS.ErrnoException).code === "EPERM"; + } +} + /** Linux: answer each question from `/proc` as it is asked. */ class DirectSource implements ProcessSource { row(pid: number): Answer { diff --git a/src/process-tree.ts b/src/process-tree.ts index c6d8e73..39e5916 100644 --- a/src/process-tree.ts +++ b/src/process-tree.ts @@ -1,4 +1,5 @@ import { + isZombie, openSource, valueOf, type LiveIdentity, @@ -61,13 +62,22 @@ export function walkTree( return out; } -/** Is this still the same process? +/** Is this still the same process, and still running? + * + * **A corpse is not a survivor.** On Linux an unreaped descendant keeps its + * `/proc` row and its identity, so matching on identity alone counted it as + * alive: the teardown would wait out its whole TERM budget for a process that + * had already died, then report it as having survived a SIGKILL. That is the + * kill over-claiming again, in the other direction. macOS never had this, + * because `ps` stops listing a process the moment it exits. * * **An unreadable source answers false, and that is deliberate**: it says * "do not signal", never "it is gone". Every caller here wants the safe * direction for a signal. */ function isSameProcess(identity: ProcessIdentity, source: ProcessSource): boolean { - return valueOf(source.identity(identity.pid)) === identity.identity; + const row = valueOf(source.row(identity.pid)); + if (row === null || isZombie(row)) return false; + return row.identity === identity.identity; } /** Signal only identities that still match their snapshot. A token mismatch diff --git a/src/sessions.ts b/src/sessions.ts index adb7bc5..991a7b3 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -5,7 +5,7 @@ import * as os from "node:os"; import * as net from "node:net"; import { createHash } from "node:crypto"; import { execFileSync } from "node:child_process"; -import { openSource } from "./proc-table.ts"; +import { isZombie, processOf } from "./proc-table.ts"; import { assertPrivateRecoveryPaths, atomicWritePrivate, @@ -814,11 +814,13 @@ type ReapObservedResult = * has a readable start token, so the cheap predicates call a corpse a survivor. */ export function hasProcessExitedForReap(pid: number): boolean { if (!isProcessAlive(pid)) return true; - // One `/proc` read on Linux, one `ps` call on macOS — and never one per - // process inside a poll loop, which is what this used to be. - const answer = openSource().isRunning(pid); - if (answer.kind === "known") return !answer.value; - if (answer.kind === "not-present") return true; + // One `/proc` read on Linux and no subprocess. On macOS this is still one + // `ps` per call, which is Node's floor without a native module — but it asks + // about ONE pid rather than reading the whole table, because this sits inside + // poll loops that run every 25 ms. + const row = processOf(pid); + if (row.kind === "known") return isZombie(row.value); + if (row.kind === "not-present") return true; // We did not find out. Ask the kernel once more rather than reading our own // silence as a death. return !isProcessAlive(pid); diff --git a/tests/proc-table.test.ts b/tests/proc-table.test.ts index 7787416..db5e054 100644 --- a/tests/proc-table.test.ts +++ b/tests/proc-table.test.ts @@ -3,9 +3,9 @@ // answers this module exists to stop. import { describe, expect, it } from "vitest"; +import { hasProcessExitedForReap } from "../src/sessions.ts"; import { isDefinitelyAbsent, - isZombie, openSource, orAbsentWhenUnknown, parseProcStat, @@ -103,9 +103,11 @@ describe("against the real machine", () => { expect(isDefinitelyAbsent(openSource().isRunning(0x7fffffff))).toBe(true); }); - // A zombie has a row and answers kill(pid, 0). The table must still say it - // is not running, and must not say it is absent. - it("reports a real zombie as present but not running", async () => { + // An unreaped child must never read as running. **The two platforms reach + // that answer differently, and this asserts the answer.** On Linux the corpse + // keeps a row with state Z. On macOS `ps` stops listing it the moment it + // exits. Measured on a real Mac by Silber.pty on 2026-09-03. + it("never reports an unreaped child as running", async () => { const sh = spawn("sh", ["-c", "sleep 0.1 & echo $! ; kill -STOP $$"], { stdio: ["ignore", "pipe", "ignore"], }); @@ -113,18 +115,25 @@ describe("against the real machine", () => { const pid = await new Promise((resolve) => sh.stdout!.once("data", (d) => resolve(Number(String(d).trim()))), ); - let source = openSource(); - for (let i = 0; i < 200; i++) { - source = openSource(); - const row = valueOf(source.row(pid)); - if (row && isZombie(row)) break; + let settled: string | null = null; + for (let i = 0; i < 500; i++) { + const source = openSource(); + const answer = source.isRunning(pid); + // Linux: still listed, but a corpse. + if (answer.kind === "known" && answer.value === false) { + settled = "listed as not running"; + break; + } + // macOS: gone from the listing entirely. + if (answer.kind === "not-present") { + settled = "no longer listed"; + break; + } await sleep(10); } - const row = valueOf(source.row(pid)); - expect(row, "the child never appeared as a zombie").not.toBeNull(); - expect(isZombie(row!)).toBe(true); - expect(valueOf(source.isRunning(pid))).toBe(false); - expect(isDefinitelyAbsent(source.isRunning(pid))).toBe(false); + expect(settled, "an exited child still read as running").not.toBeNull(); + // Whichever route, the conclusion callers depend on is the same. + expect(hasProcessExitedForReap(pid)).toBe(true); } finally { sh.kill("SIGKILL"); } diff --git a/tests/process-groups.test.ts b/tests/process-groups.test.ts index 7d0eec5..e0e8f38 100644 --- a/tests/process-groups.test.ts +++ b/tests/process-groups.test.ts @@ -114,8 +114,15 @@ describe("the sweep against real processes", () => { // real processes: it builds a real group whose members ignore SIGTERM, runs // the real sweep, and checks the process table afterwards. it("kills a real group that ignores SIGTERM and verifies it is gone", async () => { - const child = spawn("setsid", ["sh", "-c", "trap '' TERM; sleep 60 & sleep 60"], { + // `detached: true` gives the child its own process group. + // + // **macOS has the `setsid` system call but no `setsid` executable.** An + // earlier version of this test spawned the binary, so on the one platform + // where process groups are the whole escalation story, the test could not + // run at all. Reported from a real Mac by Silber.pty on 2026-09-03. + const child = spawn("sh", ["-c", "trap '' TERM; sleep 60 & sleep 60"], { stdio: "ignore", + detached: true, }); const leader = child.pid!; try { diff --git a/tests/process-tree.test.ts b/tests/process-tree.test.ts index 01aa5b5..85c773c 100644 --- a/tests/process-tree.test.ts +++ b/tests/process-tree.test.ts @@ -8,6 +8,7 @@ import { import { liveIdentity, sourceFromShape, + valueOf, type ProcessSource, } from "../src/proc-table.ts"; @@ -15,15 +16,31 @@ import { * simulated without touching a real machine. */ function mutableSource(live: Map, shape: string): () => ProcessSource { const base = sourceFromShape(shape); + const rowOf = (pid: number) => { + const v = live.get(pid); + if (v === undefined) return null; + const row = valueOf(base.row(pid)); + return row ? { ...row, identity: liveIdentity(v) } : null; + }; return () => ({ rows: () => base.rows(), - row: (pid) => base.row(pid), - isRunning: (pid) => base.isRunning(pid), + row: (pid) => { + const r = rowOf(pid); + return r === null + ? { kind: "not-present" as const } + : { kind: "known" as const, value: r }; + }, + isRunning: (pid) => { + const r = rowOf(pid); + return r === null + ? { kind: "not-present" as const } + : { kind: "known" as const, value: true }; + }, identity: (pid) => { - const v = live.get(pid); - return v === undefined + const r = rowOf(pid); + return r === null || r.identity === null ? { kind: "not-present" as const } - : { kind: "known" as const, value: liveIdentity(v) }; + : { kind: "known" as const, value: r.identity }; }, }); } From 19762b9127752e8d53a524962669908159cef82a Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 14:10:24 +0200 Subject: [PATCH 07/10] Parse what ps writes, not what the parser expected The single-process read handed the subprocess's raw stdout to the single-line parser. `ps` ends its output with a newline and that parser's `$` does not match before one, so on macOS a live process read as "field-empty" and a zombie read as NOT EXITED. The teardown would then have waited out its whole budget for a corpse. That is the corpse defect a third time, reintroduced by a second parsing path that Linux never exercised, because on Linux this read goes to `/proc`. There is one parser now. The single-process read goes through `parsePsListing`, which splits lines first and checks that the row for the pid it asked about is actually present, so both jobs are done by the code that was already tested. The other two `ps` call sites were checked for the same seam. `server.ts` and `recovery.ts` both trim before parsing; only the path added by this branch did not. Nothing caught it because every test built its input the way the parser expected. They all agreed with each other and none of them agreed with `ps`. So there is now a test that runs the real command with the real arguments and feeds the parser exactly what the subprocess wrote, newline and all. Reverting to the old behaviour fails it. Found on a real Mac by Silber.pty. --- src/proc-table.ts | 14 +++++++++++- tests/proc-table.test.ts | 46 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/proc-table.ts b/src/proc-table.ts index 88b4551..56da86f 100644 --- a/src/proc-table.ts +++ b/src/proc-table.ts @@ -144,7 +144,19 @@ export function processOf(pid: number): Answer { // from `ps` failing. Ask the kernel, which does distinguish them. return processExists(pid) ? unknown("table-unreadable") : notPresent(); } - const row = parsePsRow(out); + // **One parser, not two.** An earlier version handed the subprocess's raw + // stdout to the single-line parser, which rejected the trailing newline every + // `ps` prints — so a live process read as "field-empty" and a zombie read as + // NOT EXITED, which would make the teardown wait out its whole budget for a + // corpse. That is the corpse defect again, reintroduced on macOS by a second + // parsing path that Linux never exercised. Found on a real Mac by + // `Silber.pty` on 2026-09-03. + // + // `parsePsListing` splits lines first and checks that the row for the pid we + // asked about is actually there, so both jobs are done by the code that was + // already tested. + const rows = parsePsListing(out, pid); + const row = rows?.find((r) => r.pid === pid); if (row) return known(row); // It ran and said nothing about this pid. return processExists(pid) ? unknown("field-empty") : notPresent(); diff --git a/tests/proc-table.test.ts b/tests/proc-table.test.ts index db5e054..76d50b2 100644 --- a/tests/proc-table.test.ts +++ b/tests/proc-table.test.ts @@ -14,7 +14,7 @@ import { unknown, valueOf, } from "../src/proc-table.ts"; -import { spawn } from "node:child_process"; +import { execFileSync, spawn } from "node:child_process"; const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); @@ -48,6 +48,50 @@ describe("the truncation guard", () => { }); }); +describe("the shape a subprocess actually returns", () => { + // `ps` prints a trailing newline. An earlier version passed raw stdout to the + // single-line parser, whose `$` does not match before one, so a live process + // read as "field-empty" and a zombie read as not exited. There is now one + // parser and these pin its input shape. + const row = "94908 94907 94576 Z 0 0.0 Thu Sep 3 14:05:44 2026 "; + + it("parses a row that ends in a newline", () => { + const rows = parsePsListing(`${row}\n`, 94908); + expect(rows).not.toBeNull(); + expect(rows![0]).toMatchObject({ pid: 94908, ppid: 94907, pgid: 94576, state: "Z" }); + }); + + it("parses a row with no trailing newline", () => { + expect(parsePsListing(row, 94908)).not.toBeNull(); + }); + + it("keeps the lstart text through a trailing newline", () => { + const rows = parsePsListing(`${row}\n`, 94908); + expect(rows![0].identity).toBe("darwin:Thu Sep 3 14:05:44 2026"); + }); + + // **The seam this bug lived in.** Every other test builds its input the way + // the parser expects, so all of them agreed with each other and none of them + // agreed with `ps`. This one runs the real command with the real arguments + // and feeds the parser exactly what the subprocess wrote, newline and all. + it("parses what the ps subprocess actually writes", () => { + const out = execFileSync( + "ps", + ["-o", "pid=,ppid=,pgid=,state=,rss=,pcpu=,lstart=", "-p", String(me)], + { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] }, + ); + expect(out.endsWith("\n"), "precondition: ps ends its output with a newline").toBe(true); + + const rows = parsePsListing(out, me); + expect(rows, `parser rejected real ps output: ${JSON.stringify(out)}`).not.toBeNull(); + const row = rows!.find((r) => r.pid === me); + expect(row).toBeDefined(); + expect(row!.ppid).toBeGreaterThan(0); + expect(row!.pgid).toBeGreaterThan(0); + expect(row!.identity).toBeTruthy(); + }); +}); + describe("three answers, never two", () => { const source = sourceFromShape(`${me} 1 ${me} Ss`); From 39883e969b5edf6b02569b9c8e5987987983d2fd Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 14:43:16 +0200 Subject: [PATCH 08/10] Say "is already running" at once instead of after thirty seconds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `pty run` that loses a creation race waited out the whole start budget and then reported a generic publication timeout. `wait_for_publication` compares the published metadata against its OWN pid, so for a loser that check is false for the rest of the budget. Its only other way out of the loop is noticing its own daemon die. When that is slow — a loaded machine, a daemon still starting up — the loop spends the entire thirty second default and reports a timeout, when the true answer was on disk in the first iteration. Measured on a Mac by Silber.pty on 2026-09-03: 30.06 s against a 30 s budget, saying "Timed out waiting for daemon publication" instead of "is already running". The loop now checks whether the name is published by a live process that is not us, and stops with the sentence `pty run` already prints when it sees a running session before it spawns. Losing the race later should not produce a different explanation of the same situation. All three conditions matter and each is tested. Published, or a name whose metadata is still being written would be refused. A different pid, or a successful spawn would refuse itself. A live one, or stale metadata from a dead daemon would make the name permanently unusable. The safety property was never in question. The test that found this asserts exactly one winner before it checks the loser message, and that assertion passed every time. What was wrong is what the loser said, and how long it took. --- src/spawn.ts | 41 +++++++++++++++++++++++++++ tests/spawn-already-published.test.ts | 38 +++++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 tests/spawn-already-published.test.ts diff --git a/src/spawn.ts b/src/spawn.ts index 0f77ac9..d32b25d 100644 --- a/src/spawn.ts +++ b/src/spawn.ts @@ -6,6 +6,7 @@ import { fileURLToPath } from "node:url"; import { acquireLock, getEventsPath, getSocketPath, readMetadata, releaseLock, validateDisplayName, + isProcessAlive, } from "./sessions.ts"; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -228,6 +229,27 @@ async function spawnViaNode(options: SpawnDaemonOptions, serverModule: string): metadata.daemonPid === child.pid && hasPublishedSessionStart(options.name, metadata.createdAt); if (startPublished) break; + // **Read the fact that is already there before waiting for one that is + // not.** If somebody else has published this name, this attempt can never + // win: the check above compares against our own pid and stays false for + // the rest of the budget. The only other way out of this loop is noticing + // our own daemon die, so when that is slow — a loaded machine, a daemon + // still starting up — the loop spends the whole start timeout and then + // reports a timeout, when the true answer was on disk in the first pass. + // + // Measured on a Mac by Silber.pty on 2026-09-03: the losing `pty run` + // took 30.06 s against a 30 s budget and said "Timed out waiting for + // daemon publication" instead of "is already running". + if (publishedElsewhere(metadata?.daemonPid ?? null, child.pid ?? -1, isProcessAlive, () => + metadata !== null && hasPublishedSessionStart(options.name, metadata.createdAt), + )) { + // Deliberately the same sentence `pty run` prints when it sees a + // running session before it spawns. Losing the race later should not + // produce a different explanation of the same situation. + throw new Error( + `Session "${options.name}" is already running. Use "pty attach ${options.name}" to connect.`, + ); + } checkEarlyExit(); if (Date.now() - startedAt >= timeoutMs) { throw new Error(`Timed out waiting for daemon publication for session "${options.name}".`); @@ -242,6 +264,25 @@ async function spawnViaNode(options: SpawnDaemonOptions, serverModule: string): } } +/** Has this session been published by a live process that is not us? + * + * All three conditions matter. **Published**, or we would refuse a name whose + * metadata is still being written. **By a different pid**, or we would refuse + * our own success. **By a live one**, or stale metadata from a daemon that died + * would make the name permanently unusable. + * + * Kept separate from the registry so all four ways of answering "no" can be + * tested rather than raced for. */ +export function publishedElsewhere( + owner: number | null, + mine: number, + alive: (pid: number) => boolean, + published: () => boolean, +): boolean { + if (owner === null) return false; + return owner !== mine && alive(owner) && published(); +} + function hasPublishedSessionStart(name: string, createdAt: string): boolean { try { return fs.readFileSync(getEventsPath(name), "utf8") diff --git a/tests/spawn-already-published.test.ts b/tests/spawn-already-published.test.ts new file mode 100644 index 0000000..4c59fc1 --- /dev/null +++ b/tests/spawn-already-published.test.ts @@ -0,0 +1,38 @@ +// A `pty run` that loses a creation race waited out the whole 30 second start +// budget and then reported a generic publication timeout, when the true answer +// was on disk in the first pass. +// +// The safety property was never in question: exactly one process wins. What was +// wrong is what the loser said, and how long it took to say it. + +import { describe, expect, it } from "vitest"; +import { publishedElsewhere } from "../src/spawn.ts"; + +describe("deciding that somebody else owns the name", () => { + const live = () => true; + const dead = () => false; + const yes = () => true; + const no = () => false; + + // Every way of answering "no", so the one way of answering "yes" means + // something. Raced for, only the last of these would ever be exercised. + it("needs a live, different, published owner", () => { + expect(publishedElsewhere(200, 100, live, yes)).toBe(true); + }); + + it("does not count our own pid as somebody else", () => { + expect(publishedElsewhere(100, 100, live, yes)).toBe(false); + }); + + it("does not count a dead owner, which would make the name unusable forever", () => { + expect(publishedElsewhere(200, 100, dead, yes)).toBe(false); + }); + + it("does not count metadata that is still being written", () => { + expect(publishedElsewhere(200, 100, live, no)).toBe(false); + }); + + it("does not count a missing owner", () => { + expect(publishedElsewhere(null, 100, live, yes)).toBe(false); + }); +}); From c7eaaa15117d79eaaa630abb66136ced532d5c15 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 14:52:41 +0200 Subject: [PATCH 09/10] Do not call a zombie daemon a live owner The new check refuses a session name when it is published by a live process that is not us. It asked `pid_alive` / `isProcessAlive`, and a zombie answers `kill(pid, 0)`. So an unreaped daemon would have counted as live and the name would have been refused for as long as the corpse went unreaped. That is the exact failure the liveness condition exists to prevent, arrived at by the check that was supposed to prevent it. An unreaped daemon is the precise case that matters here: dead, not reaped, still in the process list. Both now ask `has_process_exited_for_reap` / `hasProcessExitedForReap`, which reads the process state and counts a zombie as gone. Tested against a real corpse in both languages, with the predicate the production path actually passes. Reverting to the cheap predicate fails both. Node also exports `hasProcessExitedForReap`, which was private. Silber.cos asked what this check does with a zombie daemon on macOS. The answer was worse than the question: it was wrong on both platforms. --- src/sessions.ts | 8 +++++--- src/spawn.ts | 14 +++++++++++-- tests/spawn-already-published.test.ts | 29 +++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 991a7b3..8da082d 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -809,9 +809,11 @@ type ReapObservedResult = /** Is `pid` gone for reaping purposes? A zombie counts as exited. * - * Exported because `pty kill` needs the same question answered. `!isProcessAlive` - * is not a substitute: an unreaped process still answers `kill(pid, 0)` and still - * has a readable start token, so the cheap predicates call a corpse a survivor. */ + * Exported because `pty kill` and the spawner both need the same question + * answered. `isProcessAlive` is not a substitute: an unreaped process still + * answers `kill(pid, 0)` and still has a readable start token, so the cheap + * predicates call a corpse alive — a survivor to the one, a live owner to the + * other. */ export function hasProcessExitedForReap(pid: number): boolean { if (!isProcessAlive(pid)) return true; // One `/proc` read on Linux and no subprocess. On macOS this is still one diff --git a/src/spawn.ts b/src/spawn.ts index d32b25d..c045598 100644 --- a/src/spawn.ts +++ b/src/spawn.ts @@ -6,7 +6,7 @@ import { fileURLToPath } from "node:url"; import { acquireLock, getEventsPath, getSocketPath, readMetadata, releaseLock, validateDisplayName, - isProcessAlive, + hasProcessExitedForReap, } from "./sessions.ts"; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -240,7 +240,12 @@ async function spawnViaNode(options: SpawnDaemonOptions, serverModule: string): // Measured on a Mac by Silber.pty on 2026-09-03: the losing `pty run` // took 30.06 s against a 30 s budget and said "Timed out waiting for // daemon publication" instead of "is already running". - if (publishedElsewhere(metadata?.daemonPid ?? null, child.pid ?? -1, isProcessAlive, () => + // **NOT `isProcessAlive`.** A zombie answers `kill(pid, 0)`, so the cheap + // predicate calls a corpse live — and a corpse recorded as the owner would + // make this session name refuse every future `pty run`, which is precisely + // the failure this check exists to avoid. + const ownerLive = (pid: number) => !hasProcessExitedForReap(pid); + if (publishedElsewhere(metadata?.daemonPid ?? null, child.pid ?? -1, ownerLive, () => metadata !== null && hasPublishedSessionStart(options.name, metadata.createdAt), )) { // Deliberately the same sentence `pty run` prints when it sees a @@ -271,6 +276,11 @@ async function spawnViaNode(options: SpawnDaemonOptions, serverModule: string): * our own success. **By a live one**, or stale metadata from a daemon that died * would make the name permanently unusable. * + * "Live" means `hasProcessExitedForReap`, never `isProcessAlive`. **A zombie + * answers `kill(pid, 0)`**, measured on Linux 2026-09-03, so the cheap + * predicate calls a corpse live. An unreaped daemon is the precise case this + * has to get right. + * * Kept separate from the registry so all four ways of answering "no" can be * tested rather than raced for. */ export function publishedElsewhere( diff --git a/tests/spawn-already-published.test.ts b/tests/spawn-already-published.test.ts index 4c59fc1..7ad0cef 100644 --- a/tests/spawn-already-published.test.ts +++ b/tests/spawn-already-published.test.ts @@ -6,7 +6,12 @@ // wrong is what the loser said, and how long it took to say it. import { describe, expect, it } from "vitest"; +import { spawn } from "node:child_process"; import { publishedElsewhere } from "../src/spawn.ts"; +import { hasProcessExitedForReap, isProcessAlive } from "../src/sessions.ts"; + +const sleep = (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, ms)); describe("deciding that somebody else owns the name", () => { const live = () => true; @@ -35,4 +40,28 @@ describe("deciding that somebody else owns the name", () => { it("does not count a missing owner", () => { expect(publishedElsewhere(null, 100, live, yes)).toBe(false); }); + + // The predicate the production path actually passes, against a real corpse. + // `isProcessAlive` says true for a zombie, which would refuse the name for as + // long as the corpse went unreaped. An unreaped daemon is the precise case. + it("does not count a zombie daemon as a live owner", async () => { + const sh = spawn("sh", ["-c", "sleep 0.1 & echo $! ; kill -STOP $$"], { + stdio: ["ignore", "pipe", "ignore"], + }); + try { + const corpse = await new Promise((resolve) => + sh.stdout!.once("data", (d) => resolve(Number(String(d).trim()))), + ); + for (let i = 0; i < 500 && !hasProcessExitedForReap(corpse); i++) await sleep(10); + + expect(isProcessAlive(corpse), "precondition: a zombie answers kill(pid, 0)").toBe(true); + const ownerLive = (pid: number) => !hasProcessExitedForReap(pid); + expect( + publishedElsewhere(corpse, 100, ownerLive, yes), + "a zombie daemon must not make the session name unusable", + ).toBe(false); + } finally { + sh.kill("SIGKILL"); + } + }, 20_000); }); From 1e0debfcdacc8a5252d93720ec793e636c89ae92 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 3 Sep 2026 15:06:58 +0200 Subject: [PATCH 10/10] Do not call a name in use when nothing alive is using it `pty run --id X` refused when a session record named X existed on disk, without asking whether anything was answering for it. A record whose owner is a zombie has no socket, so the name was unusable until something reaped the corpse. That is the same failure the liveness check in the spawner exists to prevent, one command earlier and still shipping. The Rust tool already asks `session_exists(name) && client::is_alive(name)` here. This adds the missing half: `isSessionAlive`, a faithful port of `client::is_alive`, which is a socket connect. Measured on a Mac by Silber.pty on 2026-09-03 with a real zombie owner: Rust created a replacement in 186 ms, Node exited 1 with "is already in use". Verified end to end on Linux with a real zombie-owned record and no socket: Node now creates the session, and reverting the liveness condition refuses it again. --- src/cli.ts | 16 ++++++++++++++-- src/client.ts | 23 +++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 03b194b..bd87609 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -4,7 +4,9 @@ import * as path from "node:path"; import * as readline from "node:readline/promises"; import { spawnSync, execFileSync } from "node:child_process"; import { randomBytes } from "node:crypto"; -import { attach, peek, send, queryStats, resolveSeqDelayMs, validateAttachStreamFdV1, type StatsResult } from "./client.ts"; +import { attach, peek, send, queryStats, resolveSeqDelayMs, validateAttachStreamFdV1, type StatsResult, + isSessionAlive, +} from "./client.ts"; import { printVersion } from "./version.ts"; import { parseSeqValue } from "./keys.ts"; import { @@ -950,7 +952,17 @@ async function main(): Promise { console.error(e.message); process.exit(1); } - if (existingNames.has(explicitId) && !attachExisting) { + // **"In use" has to mean in use by something alive.** A name that + // merely exists on disk is not taken: a record whose owner is a zombie + // has no socket to answer, and refusing it would make that session name + // unusable until something reaped the corpse — the exact failure the + // liveness check in `spawn.ts` exists to prevent, one command earlier. + // + // The Rust tool asks `session_exists(name) && client::is_alive(name)` + // here. This is the same question. Measured on a Mac by Silber.pty on + // 2026-09-03: with a zombie owner, Rust created a replacement in 186 ms + // and Node exited 1. + if (existingNames.has(explicitId) && !attachExisting && await isSessionAlive(explicitId)) { console.error(`Session id "${explicitId}" is already in use.`); process.exit(1); } diff --git a/src/client.ts b/src/client.ts index 3718c11..b1d6308 100644 --- a/src/client.ts +++ b/src/client.ts @@ -17,6 +17,29 @@ import { getSocketPath } from "./sessions.ts"; import { stripAnsi } from "./tui/colors.ts"; import { BRACKETED_PASTE_START, BRACKETED_PASTE_END } from "./paste.ts"; +/** Can anything answer on this session's socket? + * + * The Rust tool's `client::is_alive`, which is `connect(name).is_ok()`. A name + * that merely EXISTS on disk is not a name that is in use: a record whose + * owner is a zombie has no socket to answer, and refusing it would make that + * session name unusable until something reaped the corpse. + */ +export function isSessionAlive(name: string, timeoutMs = 250): Promise { + return new Promise((resolve) => { + let settled = false; + const done = (alive: boolean) => { + if (settled) return; + settled = true; + try { socket.destroy(); } catch {} + resolve(alive); + }; + const socket = net.createConnection(getSocketPath(name)); + socket.setTimeout(timeoutMs, () => done(false)); + socket.once("connect", () => done(true)); + socket.once("error", () => done(false)); + }); +} + const DETACH_KEY = 0x1c; // Ctrl+\ (legacy encoding) const DETACH_KEY_KITTY = "\x1b[92;5u"; // Ctrl+\ (Kitty keyboard protocol)