fix: reap the headroom proxy and read zombie pids as dead - #32
Merged
Merged
Conversation
The crash-restart path never fired after a kill -9 of the daemon, for three compounding reasons, all fixed at their roots. The supervisor spawned headroom detached and unref'd with no exit listener, so nothing ever waited on the child: it lingered as a zombie with the supervisor as its parent. It now keeps every ChildProcess handle and consumes the exit event, which is both the reaping and the crash signal; unref is gone, because a handle taken off the event loop is a child nobody reaps. Even with a listener, the loop slept with Atomics.wait, which blocks the event loop and stops exit events being delivered at all. The SupervisorPorts sleep is now a real timer the loop awaits, so the loop turns between ticks. Crash detection polled liveness with kill(pid, 0), and a zombie answers that as alive. Every liveness check in the coordination layer (the supervisor, the ensure step, session pruning, status, doctor) now uses a zombie-aware predicate: realIsProcessRunning reads ps's state column and treats Z as dead, documented against the signal-0 check a defunct process defeats. The test fakes model existence, zombification, and the exit event as distinct facts, which the old always-alive fakes could not. Also: the deliberate stop path escalates SIGTERM to SIGKILL through a bounded poll, as a pure stopSupervisedProcess with its own tests (this proxy ignores SIGTERM outright); a detected crash clears the daemon fields from state immediately, so status never claims a dead port; the supervisor handles SIGTERM and SIGINT and routes them through an orderly exit, because Node skips exit handlers on unhandled signal death, which orphaned the daemon; and a successor supervisor stops an orphan daemon its predecessor left running before starting its own. Verified live against the real daemon: kill -9 of the proxy restarts it within one tick with the old pid fully reaped, a drift restart serves the new allowlist, and SIGTERM of the supervisor takes the daemon with it.
The per-identity resync lock judged its holder with kill(pid, 0), which an unreaped launcher still answers as alive. A launcher that died mid-resync with nobody to reap it could therefore hold the lock as a zombie and turn every other session's launch into a lock-busy failure until the zombie was collected. The lock's predicate is now the same zombie-aware realIsProcessRunning the headroom coordination layer uses, renamed from isProcessAlive across the lock, farm, and FarmRuntime wiring to say what it means.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
Summary
Three compounding defects in the headroom supervisor's crash handling, found by SIGKILLing the daemon during a live run:
unref'd with no exit listener, so nothing ever reaped it and a killed daemon lingered as a zombie;kill(pid, 0), which a zombie answers as alive, so the restart branch never fired andstate.jsonkept advertising a healthy port nobody was listening on;Atomics.wait), blocking the event loop so exit events could never have been delivered between ticks anyway.The supervisor now keeps every spawned
ChildProcesshandle, consumes its exit event (which is also the reap), awaits its sleeps on real timers, and reads liveness through a zombie-aware predicate (signal-0 aliveANDps -o stat=not defunct, Windows falling back to signal-0). Stopping escalates SIGTERM to SIGKILL on a bounded grace, because this proxy ignores SIGTERM outright. A crash clears the daemon fields fromstate.jsonimmediately. En route, two more of the same class: the supervisor now handles SIGTERM/SIGINT (Node skipsexithandlers on unhandled signals, so killing it orphaned the daemon), a successor supervisor stops any orphan a dead predecessor left, and the identity resync lock judges its holder with the same zombie-aware predicate, since a zombie launcher could pin the lock and fail every other launch with lock-busy.Testing
pnpm run typecheckpnpm run lintpnpm run test(910 tests)pnpm schemaregenerated, zero diffNew fakes model existence, zombie and exit-event as distinct facts, so the regression tests cover the exact gap that let this escape: a zombie the signal-0 table still lists, exit-event-driven restart, state clearing on crash, SIGTERM/SIGKILL escalation, orphan takeover, and zombie launcher pruning.
Checklist