fix(coding-agent): one zombie-aware process-liveness probe - #2041
Open
snimu wants to merge 5 commits into
Open
Conversation
Five modules hand-rolled bare kill(pid, 0) probes that count zombie processes as alive: the supervisor ownership registry, session leases, the worker-side supervisor launch lock, daemon-ps process stops, and update-restart identity checks. A zombie supervisor therefore kept its ownership record readable as a live owner, wedging every subsequent daemon start on that socket until manual cleanup, and a zombie lease owner kept a session file locked. Delete the local copies and import isProcessAlive from utils/child-process, which already exists and is zombie-aware.
…mbie checks Two consequences of unifying on the zombie-aware liveness probe: stopTrackedProcess signals a process GROUP, but its completion condition checked only the leader pid. With the zombie-aware probe, a leader that zombifies after the group SIGTERM read as stopped, skipping the SIGKILL escalation and letting the caller drop worker records while descendants in the group kept running (the pre-unification bare probe accidentally escalated in that case). Completion now requires the leader gone AND the group empty via the new processGroupExists helper, which also catches the pre-existing leak where a fully reaped leader left live descendants behind. assertDaemonSupervisorOwnerCurrent runs in the 250ms per-claim fence poll, and the zombie half of isProcessAlive spawns a synchronous ps on macOS/BSD, so unification turned every fence tick into a process spawn. The fence path now checks existence with kill(0) on every tick (reaped owners are caught immediately) and confirms non-zombie state at most once per 5s per owner pid, mirroring the supervisor's existing throttled-identity-probe pattern. Acquisition and admission paths keep the full-strength probe.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 06da0f8. Configure here.
…entity degradation, bounded confirmation cache Review follow-ups on the group-stop reshape: - An unreaped zombie member held processGroupExists true forever, so a stop whose targets had all exited burned the full escalation budget and reported failure. Completion now uses processGroupHasLiveMember: zombies have exited and only their parent can reap them, so they no longer block completion while running descendants still do. - A reaped leader failed the getProcessStartId identity gates and the stop bailed before signaling the group. The gates protect against pid reuse, which only applies while the leader process exists; with the leader gone, teardown degrades to the group checks (a pgid cannot be reused while members hold it). - The owner zombie-confirmation cache never dropped entries for supervisors nothing asserts anymore; expired entries are now pruned when a confirmation is recorded, keeping the cache bounded.
…comments One spawnZombieProcess fixture replaces three copies of the perl fork scaffold, and multi-line comment blocks collapse to one-line invariant guards. No behavior or coverage change.
…lding the pgid Degrading the identity gates when the leader is gone opened a reuse window the old refuse-to-signal behavior did not have: if every group member exits between the checks and the signal, the pgid can be recycled (a new process taking that pid as its session/group id) and the group SIGKILL lands on an unrelated same-user workload. signalProcessGroupIfHeld now re-checks at signal time: a leader process (even a zombie) anchors its pgid against reuse, and once the leader is gone a live member must hold the pgid — narrowing the residual exposure to the inherent kill() TOCTOU that every single-pid signal, including the old code's, already has. Empty groups are not signaled at all.
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.
Part of the worker-state single-truth program (Linear RES-1270); squashes discussion #1732.
Purpose
A zombie process passes a bare
kill(pid, 0)probe, andgetProcessStartIdstill reports its original start time, so identity checks pass too. Five modules hand-rolled that bare probe and therefore disagreed with the canonical zombie-awareisProcessAliveinutils/child-process.ts:daemon-supervisor-ownership.ts— a zombie supervisor's ownership record read as a live owner: every later daemon start on that socket throwsDaemonSupervisorAlreadyRunningErroruntil someone manually reaps the zombie (the [Bug] Daemon can never start again after its supervisor is left unreaped as a zombie #1732 report).core/session-lease.ts— a zombie lease owner kept a session file locked.daemon-mode.ts(worker-side supervisor launch lock) — a zombie lock owner suppressed supervisor relaunch.cli/daemon-ps.ts— stop/kill wait loops treated an exited-but-unreaped process as still running.cli/daemon-update-restart.ts— restart coordination treated a zombie predecessor as alive.Change
Pure consolidation: delete the five local
kill(pid, 0)copies and import the sharedisProcessAlive(zombie-aware, EPERM-tolerant). No new mechanism; net src LOC: -41 (+6/-47, all additions are import lines).The evidence sweep listed four copies;
daemon-mode.tshad a fifth identical private method (added in #383), deleted here for the same reason. Remainingkill(pid, 0)sites were left alone deliberately:daemon-ps.tsverifyHelloSupervisorPidchecks a pid that just answered a hello (cannot be a zombie mid-reply),daemon-launch.tshasProcessIdentityExitedis an inverted has-exited check where a zombie only extends a bounded wait, andkernel/bootstrap.tsbelongs to the atomic-persistence program.Tests
One new pin in
daemon-supervisor-ownership.test.ts: a real zombie (perl fork trick, same aschild-process.test.ts) written into a conflicting owner record must be reclaimed by a successor acquisition instead of throwingDaemonSupervisorAlreadyRunningError. Verified fail-unfixed: reverting the ownership hunk makes the test fail withAlreadyRunning. The shared helper's zombie semantics were already pinned inchild-process.test.ts.Ran locally: daemon-supervisor-ownership, session-lease, child-process, daemon-ps, proper-lockfile-compromise, daemon-supervisor-monitor, regressions 4606/4600/879 — 171/171 pass.
Note
Medium Risk
Changes ownership, lease, and shutdown liveness in the daemon path; a 5s owner-alive cache can briefly treat a just-zombied PID as live until re-probed.
Overview
Unreaped zombie processes no longer count as live owners across daemon supervision, session leases, supervisor launch locks,
daemon psworker stops, and update-restart coordination. Five duplicatedkill(pid, 0)helpers are removed in favor of the sharedisProcessAliveinchild-process.ts(existence plus non-zombie).child-process.tsgains process-group helpers:processGroupHasLiveMember(zombies do not block “stopped”),signalProcessGroupIfHeld, andprocessGroupExists.daemon-psstopTrackedProcesstreats a stop as complete only when the leader is not alive and no running group member remains.daemon-supervisor-ownershipusesisOwnerProcessAlivewith a 5s positive cache so 250ms fence polls stay cheap (kill(0)every tick,pszombie check at most once per interval). Tests add aspawnZombieProcessfixture and pins for zombie owners, group-stop behavior, and fence-poll caching.Reviewed by Cursor Bugbot for commit 92a0eac. Bugbot is set up for automated code reviews on this repo. Configure here.
LOC
Total src: +110/−58 (net +52); tests: +146/−24 (net +122).
Note
Replace local liveness checks with shared zombie-aware process probe
isProcessAlive,processGroupHasLiveMember, andsignalProcessGroupIfHeldto child-process.ts and replaces module-localisProcessAlivehelpers across daemon-ps, update-restart, session-lease, and daemon-mode with the shared probeps-backed zombie probes more than once per 5 seconds per pidtrackedProcessStoppedrequires both the leader to be non-alive and no live group member; a group with live descendants that cannot be listed bypswill conservatively report a live member, which may delay stop completion on platforms wherepslisting failsMacroscope summarized 92a0eac.