Make pty kill verify and finish the kill, read the process table in one place, and stop waiting to say a name is taken - #170
Merged
Merged
Conversation
myobie
changed the title
This was referenced Sep 3, 2026
`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.
The daemon tears down the child's tree on its way out, so the teardown races its own exit. Whatever it does not manage is nobody's work after that, and the command that outlives it does nothing about it. `pty kill` now re-reads the process table after the daemon has gone. If anything from its pre-kill snapshot is still alive, it signals the process groups the session left behind, waits, escalates to SIGKILL, reads the table again, and reports what is still there. It never reports the sending. Process groups rather than pids, because a group signal needs no identity. The snapshot drops a descendant whose start token cannot be read, and that process is then never signalled; groups are collected from the raw listing, so it is reached anyway. The blind spot is not solved, it is made irrelevant. A sweep also costs one `ps` in total, against one per descendant for tokens. The daemon's own group is never a target. The pty child calls setsid, so the daemon sits alone in its group and signalling it reaches the daemon only. The running process's own group is never a target either, so the command survives to print its result. A zombie is not a group member. `ps` lists it with its group, so counting it makes the sweep report a group it has already emptied. A test against a real process group found this; inspection did not.
The success line and the exit status read only the pre-kill snapshot. That snapshot drops a descendant whose start token could not be read, and it never contained a process spawned after it was taken. So a process the sweep found and could not kill can be absent from it entirely, and the command would print `killed` and exit 0 over a process that had just survived SIGKILL. That is the defect this command exists to stop making, reintroduced by the escalation that was supposed to end it. Both halves are now required: the snapshot must be clear AND the sweep must have left nothing behind. Found by reading the diff rather than by a failing test, so the test came after; it fails without the fix.
Every caller that needed a fact about a process ran its own `ps` and treated the output as fact. A subprocess can be slow, truncated or silent, and all three look exactly like "the process is gone". Node cannot make the syscalls the Rust tool uses, so this does what Node can. On Linux there is now no subprocess at all: `/proc` carries ppid, pgid, state and starttime, which is every fact the callers ask for. On macOS `ps` stays, but the table is read once per operation rather than once per process per poll. In the teardown loop that is the difference between 240 spawns inside a 1500 ms deadline and 60. Silence is a third answer everywhere. Every query separates the fact from "the table was read and this process is not in it" from "I could not find out", with no default and no conversion that turns the last into the middle by accident. Treating silence as death requires calling `orAbsentWhenUnknown`, which greps in one command. That makes the mistake visible rather than impossible. A listing that does not contain the process that read it was truncated, not empty. `ps` always lists itself. `recovery.processStartToken` is untouched and still comes from `ps -o lstart=`. Its exact text is a contract with the Rust tool through a shared registry, so the parser takes the tail verbatim rather than re-joining split fields, which would have rewritten `Wed Sep 3` as `Wed Sep 3`. The in-memory identity is a separate branded type so the two can never be compared. Production `ps` call sites: six to three, none in a per-process poll loop.
An unreaped descendant keeps its `/proc` row and its identity on Linux, so matching on identity alone counted it as alive. The teardown would wait out its whole TERM budget for a process that had already died, and then report it as having survived a SIGKILL. That is the kill over-claiming again, in the other direction. macOS never had this, because `ps` stops listing a process the moment it exits. The two platforms disagreeing is what exposed it. Two test fixes from a real Mac run, reported by Silber.pty: macOS has the `setsid` system call but no `setsid` executable. The real process-group test spawned the binary, so on the one platform where process groups are the whole escalation story, the test could not run at all. It now uses `detached: true`, which is the same thing without the command. The zombie test asserted the Linux mechanism rather than the conclusion. On Linux the corpse keeps a row with state Z; on macOS it is dropped from the listing at once. The test now asserts what every caller depends on, which is the same on both. A single-pid query no longer reads the whole table. `hasProcessExitedForReap` sits inside poll loops that run every 25 ms, and asking about one pid should not pay for every process on the machine.
The single-process read handed the subprocess's raw stdout to the single-line parser. `ps` ends its output with a newline and that parser's `$` does not match before one, so on macOS a live process read as "field-empty" and a zombie read as NOT EXITED. The teardown would then have waited out its whole budget for a corpse. That is the corpse defect a third time, reintroduced by a second parsing path that Linux never exercised, because on Linux this read goes to `/proc`. There is one parser now. The single-process read goes through `parsePsListing`, which splits lines first and checks that the row for the pid it asked about is actually present, so both jobs are done by the code that was already tested. The other two `ps` call sites were checked for the same seam. `server.ts` and `recovery.ts` both trim before parsing; only the path added by this branch did not. Nothing caught it because every test built its input the way the parser expected. They all agreed with each other and none of them agreed with `ps`. So there is now a test that runs the real command with the real arguments and feeds the parser exactly what the subprocess wrote, newline and all. Reverting to the old behaviour fails it. Found on a real Mac by Silber.pty.
A `pty run` that loses a creation race waited out the whole start budget and then reported a generic publication timeout. `wait_for_publication` compares the published metadata against its OWN pid, so for a loser that check is false for the rest of the budget. Its only other way out of the loop is noticing its own daemon die. When that is slow — a loaded machine, a daemon still starting up — the loop spends the entire thirty second default and reports a timeout, when the true answer was on disk in the first iteration. Measured on a Mac by Silber.pty on 2026-09-03: 30.06 s against a 30 s budget, saying "Timed out waiting for daemon publication" instead of "is already running". The loop now checks whether the name is published by a live process that is not us, and stops with the sentence `pty run` already prints when it sees a running session before it spawns. Losing the race later should not produce a different explanation of the same situation. All three conditions matter and each is tested. Published, or a name whose metadata is still being written would be refused. A different pid, or a successful spawn would refuse itself. A live one, or stale metadata from a dead daemon would make the name permanently unusable. The safety property was never in question. The test that found this asserts exactly one winner before it checks the loser message, and that assertion passed every time. What was wrong is what the loser said, and how long it took.
The new check refuses a session name when it is published by a live process that is not us. It asked `pid_alive` / `isProcessAlive`, and a zombie answers `kill(pid, 0)`. So an unreaped daemon would have counted as live and the name would have been refused for as long as the corpse went unreaped. That is the exact failure the liveness condition exists to prevent, arrived at by the check that was supposed to prevent it. An unreaped daemon is the precise case that matters here: dead, not reaped, still in the process list. Both now ask `has_process_exited_for_reap` / `hasProcessExitedForReap`, which reads the process state and counts a zombie as gone. Tested against a real corpse in both languages, with the predicate the production path actually passes. Reverting to the cheap predicate fails both. Node also exports `hasProcessExitedForReap`, which was private. Silber.cos asked what this check does with a zombie daemon on macOS. The answer was worse than the question: it was wrong on both platforms.
`pty run --id X` refused when a session record named X existed on disk, without asking whether anything was answering for it. A record whose owner is a zombie has no socket, so the name was unusable until something reaped the corpse. That is the same failure the liveness check in the spawner exists to prevent, one command earlier and still shipping. The Rust tool already asks `session_exists(name) && client::is_alive(name)` here. This adds the missing half: `isSessionAlive`, a faithful port of `client::is_alive`, which is a socket connect. Measured on a Mac by Silber.pty on 2026-09-03 with a real zombie owner: Rust created a replacement in 186 ms, Node exited 1 with "is already in use". Verified end to end on Linux with a real zombie-owned record and no socket: Node now creates the session, and reverting the liveness condition refuses it again.
myobie
force-pushed
the
kill-escalates
branch
from
September 3, 2026 13:18
8d6a0ff to
1e0debf
Compare
myobie
changed the title
myobie
marked this pull request as ready for review
September 3, 2026 14:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three changes, kept as separate commits, kept separate so they stay legible individually.
1.
pty killsays only what it verified. It used to signal the daemon, wait for that one pid, and printSession "X" killed.— without ever looking at the child or anything the child started. It now snapshots the tree, re-checks after, prints success only when everything is gone, and sweeps and escalates what is left. Compatibility break: it exits non-zero when survivors remain, so a script that checked the status will now fail where it used to pass on a false success.2. One reader for the process table. Every caller ran its own
psand read the output as fact, and a subprocess that is slow, truncated or silent looks exactly like "the process is gone". The teardown spawned one per descendant every 25 ms, so a tree of four cost 240 spawns inside a 1500 ms deadline. Now/procon Linux and libproc on macOS, behind one type that distinguishes "gone" from "could not find out".3. Starting something already running says so at once. It used to wait out the full 30 second budget and report a generic timeout, when the answer — the session is published by another live pid — was available in the first iteration.
Notes for review
Every process question now goes through one reap-aware predicate. A zombie answers
kill(pid, 0), so the cheap check reported corpses as alive; that made a zombie-owned session name refusepty rununtil something reaped it.Verified on macOS as well as Linux, on real zombies, against the Node implementation for parity — the two disagreed three times before they agreed.
pty upstill refuses a zombie-owned pinned id. Both tools do it identically, and it is deferred rather than fixed here.https://claude.ai/code/session_01QudBUTk4yMreUaF5mUB1Re