diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a603cb..c2ce7fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,73 @@ ## 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 + 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 + 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. +- **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. + ### Complete session termination - `pty kill` now stops the PTY child and its complete descendant tree. A @@ -28,6 +95,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..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 { @@ -42,9 +44,16 @@ import { getPidPath, getMetadataPath, DEFAULT_SESSION_DIR, + hasProcessExitedForReap, type SessionInfo, type SessionMetadata, } from "./sessions.ts"; +import { snapshotDescendantProcesses } from "./process-tree.ts"; +import { aftermathOf, allGone, killOutcomeLines, verifiedEmpty } from "./kill-report.ts"; +import { + 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, @@ -295,6 +304,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`, @@ -935,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); } @@ -2615,6 +2642,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, openSource()), + signal: signalGroup, + sleep: (ms) => new Promise((r) => setTimeout(r, ms)), + }); +} + async function cmdKill(name: string): Promise { const session = await getSession(name); @@ -2637,6 +2681,16 @@ 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); + // 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, openSource()); + try { process.kill(session.pid, "SIGTERM"); } catch { @@ -2662,7 +2716,24 @@ async function cmdKill(name: string): Promise { return; } cleanupSocket(name); - console.log(`Session "${name}" killed.`); + 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. + 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); + for (const line of outcome.err) console.error(line); + // 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/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) 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..39253cb --- /dev/null +++ b/src/kill-report.ts @@ -0,0 +1,116 @@ +/** 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; +} + +/** 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 + * 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[], + 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 = 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); + } + 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, + /** 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 (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. + 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 ` + + `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/proc-table.ts b/src/proc-table.ts new file mode 100644 index 0000000..56da86f --- /dev/null +++ b/src/proc-table.ts @@ -0,0 +1,349 @@ +/** 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()); +} + +/** 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(); + } + // **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(); +} + +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 { + 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 new file mode 100644 index 0000000..4ef2527 --- /dev/null +++ b/src/process-groups.ts @@ -0,0 +1,110 @@ +/** 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"; + +import { + isZombie, + openSource, + valueOf, + type ProcessSource, + type Row, +} from "./proc-table.ts"; +import { walkTree } from "./process-tree.ts"; + +export type { Row as ProcessRow } from "./proc-table.ts"; +export { openSource } from "./proc-table.ts"; + +/** 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, 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[] = []; + 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); + } + 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[], source: ProcessSource): number[] { + const rows = valueOf(source.rows()) ?? ([] as Row[]); + 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(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 { + 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/src/process-tree.ts b/src/process-tree.ts index 73402c1..39e5916 100644 --- a/src/process-tree.ts +++ b/src/process-tree.ts @@ -1,27 +1,28 @@ -import { execFileSync } from "node:child_process"; -import { readProcessStartToken } from "./recovery.ts"; +import { + isZombie, + 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 +30,54 @@ 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, 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 { + 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 @@ -74,11 +87,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 +105,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 4f000e0..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, @@ -1432,10 +1436,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..8da082d 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 { isZombie, processOf } from "./proc-table.ts"; import { assertPrivateRecoveryPaths, atomicWritePrivate, @@ -806,22 +807,39 @@ type ReapObservedResult = signalled: boolean; }; -function hasProcessExitedForReap(pid: number): boolean { +/** Is `pid` gone for reaping purposes? A zombie counts as exited. + * + * 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; - 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 state === "" || state.startsWith("Z"); - } catch { - return !isProcessAlive(pid); - } + // 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); +} + +/** 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 diff --git a/src/spawn.ts b/src/spawn.ts index 0f77ac9..c045598 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, + hasProcessExitedForReap, } from "./sessions.ts"; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -228,6 +229,32 @@ 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". + // **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 + // 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 +269,30 @@ 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. + * + * "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( + 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/kill-report.test.ts b/tests/kill-report.test.ts new file mode 100644 index 0000000..e31fdd8 --- /dev/null +++ b/tests/kill-report.test.ts @@ -0,0 +1,190 @@ +// `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, + verifiedEmpty, + type Aftermath, +} from "../src/kill-report.ts"; +import { hasProcessExitedForReap, reapedFromPsState } from "../src/sessions.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, + identity: liveIdentity(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 = valueOf(openSource().identity(pid)); + expect(token).not.toBeNull(); + const before = [identity(pid, token!)]; + + 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, (p) => valueOf(openSource().identity(p)), 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 = valueOf(openSource().identity(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(valueOf(openSource().identity(pid))).toBe(token); + expect( + allGone( + aftermathOf(before, (p) => valueOf(openSource().identity(p)), 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 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", () => { + 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); + } + }); +}); diff --git a/tests/proc-table.test.ts b/tests/proc-table.test.ts new file mode 100644 index 0000000..76d50b2 --- /dev/null +++ b/tests/proc-table.test.ts @@ -0,0 +1,185 @@ +// 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 { hasProcessExitedForReap } from "../src/sessions.ts"; +import { + isDefinitelyAbsent, + openSource, + orAbsentWhenUnknown, + parseProcStat, + parsePsListing, + sourceFromShape, + unknown, + valueOf, +} from "../src/proc-table.ts"; +import { execFileSync, 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("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`); + + 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); + }); + + // 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"], + }); + try { + const pid = await new Promise((resolve) => + sh.stdout!.once("data", (d) => resolve(Number(String(d).trim()))), + ); + 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); + } + 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"); + } + }, 20_000); +}); diff --git a/tests/process-groups.test.ts b/tests/process-groups.test.ts new file mode 100644 index 0000000..e0e8f38 --- /dev/null +++ b/tests/process-groups.test.ts @@ -0,0 +1,160 @@ +// 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, + membersOfGroups, + ownProcessGroup, + signalGroup, + sweepGroups, +} 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 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, openSource()); + +describe("choosing which process groups to sweep", () => { + it("never targets the daemon's own group", () => { + 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); + expect(groups).toEqual([200, 400]); + }); + + it("never targets an unrelated group", () => { + 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", () => { + // 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, unnamed)).toContain(400); + }); + + it("reads members back by group", () => { + 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 withCorpse = sourceFromShape("100 1 100 Ss\n200 100 200 Sl\n300 200 200 Z"); + expect(membersOfGroups([200], withCorpse)).toEqual([200]); + }); + + +}); + +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 () => { + // `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 { + 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); + }); +}); diff --git a/tests/process-tree.test.ts b/tests/process-tree.test.ts index 3d0b568..85c773c 100644 --- a/tests/process-tree.test.ts +++ b/tests/process-tree.test.ts @@ -5,41 +5,80 @@ import { terminateProcessIdentities, type ProcessIdentity, } from "../src/process-tree.ts"; +import { + liveIdentity, + sourceFromShape, + valueOf, + 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); + 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) => { + 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 r = rowOf(pid); + return r === null || r.identity === null + ? { kind: "not-present" as const } + : { kind: "known" as const, value: r.identity }; + }, + }); +} 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 +88,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); diff --git a/tests/spawn-already-published.test.ts b/tests/spawn-already-published.test.ts new file mode 100644 index 0000000..7ad0cef --- /dev/null +++ b/tests/spawn-already-published.test.ts @@ -0,0 +1,67 @@ +// 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 { 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; + 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); + }); + + // 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); +});