Skip to content

Commit 8c366fa

Browse files
committed
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.
1 parent 500eab2 commit 8c366fa

8 files changed

Lines changed: 339 additions & 4 deletions

File tree

CHANGELOG.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,29 @@
22

33
## Unreleased
44

5+
### `pty kill` reports what it verified
6+
7+
- `pty kill` prints `Session "X" killed.` only when the session's process tree
8+
is gone. It takes a snapshot of the tree before it signals the daemon, and
9+
re-checks that snapshot after the daemon exits. Before this, the command
10+
asked about the daemon and reported about the session, so the success line
11+
was a claim about processes it never looked at.
12+
- When something outlives the kill, the command prints
13+
`Session "X" daemon stopped.` on standard output, which is the part it
14+
verified, and names the surviving PIDs on standard error. A PID is called a
15+
survivor only when its process-start token still matches. A PID that has not
16+
exited but whose token cannot be read is reported separately as undecided.
17+
- A daemon that could not kill a descendant now appends
18+
`session_descendants_survived` to the session event log, and its standard
19+
error warning names the PIDs. The daemon's standard error has no reader, so
20+
the log line is the copy a person can find.
21+
- `pty kill` sends no additional signals and waits no longer than before.
22+
- The exit status is unchanged. `pty kill` still exits 0 when the daemon stops,
23+
even with survivors.
24+
- On macOS, an empty `ps -o stat=` field no longer counts as a dead process.
25+
An empty field means the process is gone or `ps` did not answer, and under
26+
load `ps` is the thing that goes quiet, so the kernel is asked again.
27+
528
### Complete session termination
629

730
- `pty kill` now stops the PTY child and its complete descendant tree. A
@@ -28,6 +51,12 @@
2851

2952
### Storage format
3053

54+
- New event type `session_descendants_survived`, carrying `data: { pids }`.
55+
A daemon appends it when it signalled its child's process tree with TERM and
56+
then KILL and found processes still alive. It records what the daemon could
57+
not kill; a process that left the tree before the snapshot is not in it.
58+
59+
3160
- Supporting live daemons now advertise a `recovery` capability in session
3261
metadata. `pty recover <name> --snapshot <file>` uses that captured
3362
capability to authenticate a signal-free listener/registry rebind after an

docs/disk-layout.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ Envelope: `{ session: string; type: string; ts: string; ...payload }`. Event typ
127127
| `session_respawn` | — (`pty gc` respawned a `strategy=permanent` session) |
128128
| `session_abandoned` | `reason: "cwd-gone" \| "idle", idleDays?` — (`pty gc` reaped a live permanent session detected as abandoned) |
129129
| `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) |
130+
| `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 |
130131
| `display_name_change` | `previous: string\|null, value: string\|null` |
131132
| `tags_change` | `previous, value` (full snapshots) |
132133
| `metadata_change` | `previous, value` containing only changed `displayName` and tag keys; absent tag values are `null` |

src/cli.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,12 @@ import {
4242
getPidPath,
4343
getMetadataPath,
4444
DEFAULT_SESSION_DIR,
45+
hasProcessExitedForReap,
4546
type SessionInfo,
4647
type SessionMetadata,
4748
} from "./sessions.ts";
49+
import { snapshotDescendantProcesses } from "./process-tree.ts";
50+
import { aftermathOf, killOutcomeLines } from "./kill-report.ts";
4851
import { spawnDaemon, resolveCommand } from "./spawn.ts";
4952
import {
5053
acquireEventLock, appendEventSyncLocked, EventFollower, EventWriter, EventType, releaseEventLock,
@@ -2637,6 +2640,12 @@ async function cmdKill(name: string): Promise<void> {
26372640
} catch {}
26382641
}
26392642

2643+
// Take the tree BEFORE the signal. After the daemon exits its children are
2644+
// reparented to init or a subreaper, so the parent links that identify them
2645+
// as this session's processes are gone. This snapshot is the only chance to
2646+
// learn which processes the word "killed" would be a claim about.
2647+
const before = snapshotDescendantProcesses(session.pid);
2648+
26402649
try {
26412650
process.kill(session.pid, "SIGTERM");
26422651
} catch {
@@ -2662,7 +2671,12 @@ async function cmdKill(name: string): Promise<void> {
26622671
return;
26632672
}
26642673
cleanupSocket(name);
2665-
console.log(`Session "${name}" killed.`);
2674+
const outcome = killOutcomeLines(
2675+
name,
2676+
aftermathOf(before, readProcessStartToken, hasProcessExitedForReap),
2677+
);
2678+
for (const line of outcome.out) console.log(line);
2679+
for (const line of outcome.err) console.error(line);
26662680

26672681
if (wasPermanent && session.metadata?.tags?.ptyfile) {
26682682
console.error(`Note: this session is managed by ${session.metadata.tags.ptyfile}`);

src/events.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ export const EventType = {
1919
SESSION_RESPAWN: "session_respawn",
2020
SESSION_ABANDONED: "session_abandoned",
2121
SESSION_FLAPPING: "session_flapping",
22+
SESSION_DESCENDANTS_SURVIVED: "session_descendants_survived",
2223
} as const;
2324

2425
export type EventType = (typeof EventType)[keyof typeof EventType];
@@ -85,6 +86,21 @@ export interface SessionExecEvent extends EventBase {
8586
command: string;
8687
}
8788

89+
/** Emitted by a daemon that signalled its child's process tree with TERM and
90+
* then KILL and found processes still alive. The daemon also warns on its own
91+
* standard error, which has had no reader since the command that launched it
92+
* stopped listening — so this log line is the copy a person can find.
93+
*
94+
* A record, not a guarantee: the daemon reports what it could not kill, and
95+
* cannot report a process that left its tree before the snapshot. */
96+
export interface SessionDescendantsSurvivedEvent extends EventBase {
97+
type: "session_descendants_survived";
98+
/** The surviving pids, deepest descendant first. Nested under `data` to
99+
* match the Rust tool byte for byte; both render through the unknown-type
100+
* fallback, so the printed line is identical. */
101+
data: { pids: number[] };
102+
}
103+
88104
/** Emitted by `pty gc` whenever it respawns a `strategy=permanent`
89105
* session that's exited/vanished. Carries no payload beyond the
90106
* envelope — the restart is stateless, there is no attempt counter,
@@ -185,6 +201,7 @@ export type EventRecord =
185201
| SessionRespawnEvent
186202
| SessionAbandonedEvent
187203
| SessionFlappingEvent
204+
| SessionDescendantsSurvivedEvent
188205
| UserEvent
189206
| DisplayNameChangeEvent
190207
| TagsChangeEvent

src/kill-report.ts

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
/** What `pty kill` may claim, and how it says it.
2+
*
3+
* The command signals the daemon and waits for that one PID. The child, and
4+
* everything the child started, is a separate question. This module answers it
5+
* from a snapshot taken before the signal, and turns the answer into the lines
6+
* the command prints.
7+
*
8+
* Kept out of `cli.ts` because that module runs `main()` on import and cannot
9+
* be loaded by a test.
10+
*/
11+
12+
import type { ProcessIdentity } from "./process-tree.ts";
13+
14+
/** What the pre-kill snapshot looks like once the daemon has gone. */
15+
export interface Aftermath {
16+
/** The start token still matches, so this is the same process and it is
17+
* still running. */
18+
survived: number[];
19+
/** The PID has not exited but its start token could not be read. We cannot
20+
* tell whether it is the same process or a PID the kernel has reused.
21+
*
22+
* This case gets its own list rather than joining either side. Folding it
23+
* into `survived` would invent a survivor; dropping it would repeat the
24+
* defect this module exists to remove, which is a failure to measure
25+
* reported as an answer. */
26+
unknown: number[];
27+
}
28+
29+
export function allGone(after: Aftermath): boolean {
30+
return after.survived.length === 0 && after.unknown.length === 0;
31+
}
32+
33+
/** Re-check a snapshot against the live process table.
34+
*
35+
* `exited` must be `hasProcessExitedForReap`, not `!isProcessAlive`. A zombie
36+
* answers `kill(pid, 0)` and keeps a readable start token, so the two cheaper
37+
* predicates both call it a survivor. It is a dead process waiting to be
38+
* reaped, and reporting it as still running would be this command
39+
* over-claiming again, only in the other direction.
40+
*/
41+
export function aftermathOf(
42+
before: ProcessIdentity[],
43+
readStartToken: (pid: number) => string | null,
44+
exited: (pid: number) => boolean,
45+
): Aftermath {
46+
const after: Aftermath = { survived: [], unknown: [] };
47+
for (const identity of before) {
48+
if (exited(identity.pid)) continue;
49+
const token = readStartToken(identity.pid);
50+
if (token === identity.processStartToken) after.survived.push(identity.pid);
51+
// A different token is a PID the kernel handed to somebody else.
52+
else if (token === null) after.unknown.push(identity.pid);
53+
}
54+
return after;
55+
}
56+
57+
/** Say what was verified, and nothing more.
58+
*
59+
* `killed` is a claim about the whole tree, so it appears only when every
60+
* process in the snapshot is gone. Otherwise standard output carries the part
61+
* that was verified — the daemon stopped — and standard error carries what
62+
* survived it. The two never appear together, so a reader who greps for the
63+
* success line cannot find it beside a warning that contradicts it.
64+
*/
65+
export function killOutcomeLines(
66+
name: string,
67+
after: Aftermath,
68+
): { out: string[]; err: string[] } {
69+
if (allGone(after)) return { out: [`Session "${name}" killed.`], err: [] };
70+
const err: string[] = [];
71+
if (after.survived.length > 0) {
72+
err.push(
73+
`Session "${name}": ${after.survived.length} process(es) survived the kill ` +
74+
`and are still running: ${after.survived.join(", ")}`,
75+
);
76+
}
77+
if (after.unknown.length > 0) {
78+
err.push(
79+
`Session "${name}": ${after.unknown.length} process(es) may still be running: ` +
80+
`${after.unknown.join(", ")}. Their start tokens could not be read, so this ` +
81+
"is not a conclusion.",
82+
);
83+
}
84+
return { out: [`Session "${name}" daemon stopped.`], err };
85+
}

src/server.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1395,10 +1395,16 @@ export class PtyServer {
13951395
}
13961396
const survivingDescendants = await descendantsDone;
13971397
if (survivingDescendants.length > 0) {
1398+
const pids = survivingDescendants.map((d) => d.pid);
13981399
console.error(
13991400
`pty daemon "${this.name}": ${survivingDescendants.length} child process(es) ` +
1400-
"did not exit after exact TERM and KILL signals",
1401+
`did not exit after exact TERM and KILL signals: ${pids.join(", ")}`,
14011402
);
1403+
// And somewhere a person can find it. The warning above goes to this
1404+
// daemon's standard error, which has had no reader since the command
1405+
// that launched it stopped listening — so the one moment it has
1406+
// something worth saying is the one moment nobody is there.
1407+
this.emitEvent(EventType.SESSION_DESCENDANTS_SURVIVED, { data: { pids } });
14021408
}
14031409
if (this.exited) await this.saveExitMetadataUntilSettled(this.exitCode);
14041410
try { await this.eventWriter.flush(); } catch {}

src/sessions.ts

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -798,7 +798,12 @@ type ReapObservedResult =
798798
signalled: boolean;
799799
};
800800

801-
function hasProcessExitedForReap(pid: number): boolean {
801+
/** Is `pid` gone for reaping purposes? A zombie counts as exited.
802+
*
803+
* Exported because `pty kill` needs the same question answered. `!isProcessAlive`
804+
* is not a substitute: an unreaped process still answers `kill(pid, 0)` and still
805+
* has a readable start token, so the cheap predicates call a corpse a survivor. */
806+
export function hasProcessExitedForReap(pid: number): boolean {
802807
if (!isProcessAlive(pid)) return true;
803808
try {
804809
if (process.platform === "linux") {
@@ -810,12 +815,26 @@ function hasProcessExitedForReap(pid: number): boolean {
810815
encoding: "utf8",
811816
timeout: 1000,
812817
}).trim();
813-
return state === "" || state.startsWith("Z");
818+
return reapedFromPsState(state, () => isProcessAlive(pid));
814819
} catch {
815820
return !isProcessAlive(pid);
816821
}
817822
}
818823

824+
/** Read a `ps -o stat=` field. `stillAlive` is asked only when the field is
825+
* empty, and it is a FRESH answer rather than the one taken before `ps` ran.
826+
*
827+
* An empty field is two answers wearing one shape: the process is gone, or
828+
* `ps` did not manage to say. Reading it as "gone" is a failure folded into an
829+
* answer about what is there — so on an empty field we ask the kernel again
830+
* instead of reading silence as death. Under load on macOS `ps` is exactly
831+
* the thing that goes quiet. */
832+
export function reapedFromPsState(state: string, stillAlive: () => boolean): boolean {
833+
if (state.startsWith("Z")) return true;
834+
if (state === "") return !stillAlive();
835+
return false;
836+
}
837+
819838
/** Signal only after proving ownership, then reacquire after daemon shutdown
820839
* so its final event/metadata flush cannot recreate artifacts after cleanup. */
821840
async function reapObservedSession(

0 commit comments

Comments
 (0)