Skip to content

Say what the kill verified, not what it hoped - #169

Closed
myobie wants to merge 2 commits into
mainfrom
kill-honest-report
Closed

Say what the kill verified, not what it hoped#169
myobie wants to merge 2 commits into
mainfrom
kill-honest-report

Conversation

@myobie

@myobie myobie commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Draft. Opened from the Rust-parity side. The matching change to the Rust
port is compoundingtech/pty-rust#6; the two are the same fix and should be
read together.

The defect

pty kill sends SIGTERM to the daemon, waits up to seven seconds for that one
pid to disappear, and prints Session "X" killed. It never looks at the child,
or at anything the child started. The word is a claim about a session, made on
evidence about a daemon.

The daemon does walk the child's tree, escalate TERM to KILL, and collect what
survived — src/server.ts. It then reports that to its own standard error,
which has had no reader since the command that launched it stopped
listening.
So the one moment the daemon has something worth saying is the one
moment nobody is there.

This is what an operator hit on 2026-09-02: a coding agent survived a
pty kill, they read killed, started the session again, and ended with two
processes writing to one 14.7 MB transcript. The word is what made the second
start look reasonable.

What this changes

pty kill takes a snapshot of the daemon's process tree before it sends the
signal, and re-checks that snapshot once the daemon has exited.

outcome stdout stderr
every process gone Session "X" killed.
survivors Session "X" daemon stopped. N process(es) survived the kill and are still running: …
cannot tell Session "X" daemon stopped. N process(es) may still be running: … Their start tokens could not be read, so this is not a conclusion.

killed now means the tree is empty. The line replacing it says the part that
was verified, so nothing is lost, and a reader who greps for killed can no
longer find it next to a warning that contradicts it.

The snapshot has to come first. Once the daemon exits its children reparent
away, and the links that identify them as this session's processes are gone. The
pre-kill snapshot is also taken at a calm moment, while the daemon takes its own
during shutdown under whatever load the machine is carrying — so the command's
view can be wider than the daemon's, which is exactly the case where the
daemon's teardown skipped something.

Three outcomes, not two. A pid is reported as surviving only when its start
token still matches. A different token is a pid the kernel reissued. A token
that cannot be read at all, on a process that has not exited, is reported
separately as undecided. Folding that third case into either of the others
would repeat the defect this PR exists to remove.

Two supporting changes, called out because they are not the headline

1. The daemon now records what it could not kill. It appends
session_descendants_survived (data: { pids }) to the session event log, and
names the pids in its stderr warning, which previously gave only a count. The
Rust port already writes this event; this closes the gap, and the two are byte
identical on the wire. Both tools render it through the unknown-type fallback,
so the printed line matches — measured, not assumed:

[12:00:00] ev1: session_descendants_survived {"pids":[4321,8765]}

docs/disk-layout.md and the changelog are updated, as
tests/disk-layout-docs.test.ts requires.

2. On macOS an empty ps -o stat= field no longer counts as a dead
process.
hasProcessExitedForReap read state === "" as exited. An empty
field is two answers wearing one shape: the process is gone, or ps did not
manage to say
— and under load ps is precisely the thing that goes quiet.
The new check depends on this predicate, so it could not be left reading silence
as death; a slow ps would make the kill report "all gone" without measuring
anything. The Rust port already does it this way.

This sends no additional signals. Same single SIGTERM, same seven-second
wait. It kills nothing extra. Widening the kill is a separate question, and an
unverified wider signal would leave the same orphans.

Exit code, and a compatibility break to read before merging

pty kill now exits non-zero when anything survived, and when a start token
could not be read so the outcome is undecided.
Nathan decided the first on
2026-09-03: "kill that doesn't actually kill everything is def a non-zero."
Silber.cos decided the second, on the grounds that "I could not confirm the
tree is empty" is not success and a caller that reads 0 as done would be wrong.
That second one is Silber.cos's call rather than Nathan's, so it can be
overruled separately.

This breaks callers, and that is the point rather than a side effect. A
script that runs pty kill and checks the status will now fail where it used to
pass — because it was passing on a false success. If you maintain such a caller,
the fix is to stop treating an unverified kill as a completed one.

The success path is unchanged: a verified empty tree still exits 0.

What is tested, and what is not

1630 passed, 2 failed — both in tests/shells.test.ts, one for fish and
one for zsh. Neither shell is installed on the machine I ran on. I checked
this rather than assuming: running tests/shells.test.ts on unmodified main
produces the same two failures and no others.

Pinned by the 14 new tests in tests/kill-report.test.ts:

  • the classification, including two that run against the real process table —
    one live process, and one real zombie;
  • the empty-ps-field reading, in both directions;
  • the exact output lines, including that the success line and a survivor report
    never appear together.

A zombie is why the predicate matters. An unreaped process answers
kill(pid, 0) and keeps a readable start token, so both obvious predicates call
it alive. A control run confirms the test bites: with !isProcessAlive in place
of hasProcessExitedForReap, does not call a real zombie a survivor fails.

Not pinned: the survivor branch end to end. Reaching it needs a descendant
that outlives a SIGKILL, or a start token that cannot be read while the process
is alive. Neither can be manufactured on Linux without fault injection this PR
does not add. The branch is proven by unit test and by construction, and I would
rather say that than imply more.

tests/kill-wait.test.ts asserts only that stdout contains killed, which
passes whether or not a child outlived the daemon. It still passes here. The
Rust PR adds a conformance test that checks the tree, and that suite runs
against both binaries.

What this does not fix

A process that leaves the daemon's tree before the snapshot is invisible to
this command, exactly as it is invisible to the daemon.
I measured one: a
grandchild orphaned by its intermediate shell reparents away within
milliseconds. In that case the pty's own hangup still killed it, but a
descendant that calls setsid would not be reached.

The start-token blind spot is related but separate, and I am keeping them
separate.
snapshotDescendantProcesses drops a descendant whose start token
cannot be read, so it is never signalled. This PR makes such a process visible
in the report
; it does not make it die. Closing it needs a different change
and its own review.

Both are the macOS ps dependency in another coat: there a start token is a
ps call per descendant, and one that answers slowly drops that process from
the teardown. That dependency owes a real fix and is tracked on its own.

One note on the existing changelog

The Complete session termination entry says pty kill now stops "its complete
descendant tree". That is the claim this PR exists to narrow. I have left the
wording alone rather than editing someone else's entry, but it is worth a look.

`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.
`pty kill` printed a survivor report and exited 0. A caller that reads only the
status reached the opposite conclusion from one that reads the output, which
leaves the honest line as decoration.

It now exits non-zero when anything survived, and when a start token could not
be read so the outcome is undecided. "I could not confirm the tree is empty" is
not success.

This is a compatibility break. A script that checks the status of `pty kill`
will fail where it used to pass, because it was passing on a false success. The
fix for such a caller is to stop treating an unverified kill as a completed one.

Nathan decided the survivor case on 2026-09-03. Silber.cos decided the
undecidable one.
@myobie

myobie commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Folded into #170, which now targets main and carries all four commits in order. Nathan asked for one pull request per binary rather than four. Nothing is lost: the commits are still separate, and the honesty change still comes first so the escalation ships against a check that can verify it.

@myobie myobie closed this Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant