feat(acp): pipe lifeline — reap the bridge tree on abrupt owner death#433
feat(acp): pipe lifeline — reap the bridge tree on abrupt owner death#433superWorldSavior wants to merge 2 commits into
Conversation
killAgentIfRunning() signalled only the bridge PID, so processes the bridge itself spawned (the model process, MCP servers such as serena) were never signalled and survived as orphans, accumulating across runs until the agent app-server choked and new sessions hung at session/new. Spawn the bridge in its own process group (detached on Unix) and, on close, signal the whole group via a negative PID. The group signal is gated on an agentGroupLeader flag so acpx never signals its own process group; Windows and any non-detached path keep the previous single-process kill. Adds a real-process regression test: a mock agent forks a long-lived grandchild and records its PID; after AcpClient.close() that PID must be dead. Co-Authored-By: Codex <codex@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuCHKuxEBNVTxHm9bSmwL
|
Codex review: needs maintainer review before merge. Reviewed July 5, 2026, 10:39 PM ET / 02:39 UTC. Summary Reproducibility: yes. source-reproducible: current main only signals the bridge PID during close and cannot run owner-death cleanup after SIGKILL. I did not run a live current-main repro in this read-only review, but the PR includes terminal proof and real-process tests for the failing paths. Review metrics: 3 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Land this as the canonical bridge-lifecycle fix if maintainers accept the native lifeline contract; otherwise split cooperative group cleanup from abrupt-death reaping and track the latter separately. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: current main only signals the bridge PID during close and cannot run owner-death cleanup after SIGKILL. I did not run a live current-main repro in this read-only review, but the PR includes terminal proof and real-process tests for the failing paths. Is this the best way to solve the issue? Yes with maintainer sign-off: the PR is focused on the core lifecycle boundary, has regression coverage, and I found no discrete correctness finding. The safer alternative, if maintainers do not want the native contract, is to split cooperative close cleanup from abrupt-death reaping. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6a24a546d234. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (10 earlier review cycles; latest 8 shown)
|
821f77d to
c4272a5
Compare
|
Addressed the P1 (detach without a lifeline could regress abrupt group-signal death):
Full suite 858/858, @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Reworked the fix after a closer look — the previous
On the "fall back to attached single-PID" suggestion: for the failure this P1 is about — the sole owner dying via a plain SIGKILL / crash / OOM — an attached bridge is orphaned just the same (SIGKILL does not cascade to the group). Attaching would only help a group-wide Also hardened:
Known best-effort limit (documented in code): a helper that execs then exits immediately (e.g. a corrupt Full suite green (860), typecheck + lint clean. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
5f78cb6 to
6c5a4f0
Compare
|
@clawsweeper re-review Updated the branch after the P1 review. The fix now covers the process-tree lifecycle as three explicit cases:
Regression coverage added for:
Verification run on final HEAD
The branch was also squashed down to two commits for a cleaner review history. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Final CI state is now fully terminal on HEAD
Requesting a final refresh because the previous ClawSweeper review still listed Mutation as pending at review time. |
Problem
AcpClient.close()and abrupt queue-owner death both need to clean up the whole ACP bridge process group, not just the bridge PID.Before this PR, bridge descendants could survive in several cases:
child.kill()path before the normal close machinery owned the bridge;SIGKILL, OOM kill, crash) runs no JavaScript cleanup at all, so a detached bridge tree can be orphaned under PID 1.We hit the abrupt-death variant in production with leaked bridge/model/MCP processes accumulating until new sessions hung at
session/new.Fix
This PR makes the bridge lifecycle group-aware on POSIX:
AcpClient.close()first closes stdin for graceful ACP shutdown, but then checks whether the bridge process group is still alive. If descendants remain, it signals the group withSIGTERM, waits, then falls back toSIGKILL.child.kill().Lifeline safety model
The lifeline uses pipe identity, not owner PID polling, so owner PID reuse is not part of the death-detection mechanism.
The helper also guards the bridge PGID side:
kill(-pgid, 0)probes the process group itself, not just the leader, so TERM-resistant descendants still receive theSIGKILLfallback;POLLERR/POLLNVALon the lifeline fd degrade without reaping rather than risk killing a live session.No binaries shipped
The npm package ships the C source only. Resolution order is:
ACPX_LIFELINE_HELPERoverride~/.acpx/native/lifeline-<version>-<platform>-<arch>usingccIf no usable helper can be resolved or compiled, acpx degrades to the previous non-detached/no-lifeline behavior instead of adding a hard native dependency.
Scope and limitations
SIGKILLcan orphan the bridge.Regression coverage
Added real-process tests for:
SIGKILLfallback after the bridge leader exits;SIGKILLcleanup with a TERM-resistant descendant.Verification
Final HEAD:
6c5a4f0e577ff677786f2b289e7785f59e83846aLocal validation:
pnpm run check-> pass (864runtime tests +109flow/runtime coverage tests, 0 failures; coverage gates passed)pnpm run mutate-> pass (Stryker score90.55, threshold80, 0 timeouts).agents/skills/autoreview/scripts/autoreview --mode branch --base upstream/main-> clean, no accepted/actionable findingspnpm dlx slophammer-ts@latest rules --format text-> passpnpm dlx slophammer-ts@latest dry .->DRY candidates: 0; maximum: 0pnpm dlx slophammer-ts@latest check . --only ts.dependency-boundaries-required-> passGitHub CI on
ubuntu-24.04:Relationship to #432
This PR supersedes #432 and is self-contained: it includes the cooperative close/group cleanup plus the abrupt-owner-death lifeline work.
Maintainer edits enabled (personal fork).