Skip to content

feat(acp): pipe lifeline — reap the bridge tree on abrupt owner death#433

Open
superWorldSavior wants to merge 2 commits into
openclaw:mainfrom
superWorldSavior:fix/bridge-lifeline
Open

feat(acp): pipe lifeline — reap the bridge tree on abrupt owner death#433
superWorldSavior wants to merge 2 commits into
openclaw:mainfrom
superWorldSavior:fix/bridge-lifeline

Conversation

@superWorldSavior

@superWorldSavior superWorldSavior commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

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:

  • cooperative close could kill only the bridge process and leave bridge-spawned children alive;
  • initialize/startup failure could hit a PID-only child.kill() path before the normal close machinery owned the bridge;
  • abrupt owner death (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:

  • The bridge is spawned as a detached process-group leader when a POSIX lifeline helper is available.
  • Normal 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 with SIGTERM, waits, then falls back to SIGKILL.
  • Initialize/startup failure now also uses group-aware termination instead of PID-only child.kill().
  • Abrupt owner death is covered by a tiny native pipe lifeline helper. The helper inherits a pipe from the owner; owner EOF means the owner died, so the helper reaps the bridge process group.

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:

  • strict PGID parsing rejects malformed or unsafe input;
  • kill(-pgid, 0) probes the process group itself, not just the leader, so TERM-resistant descendants still receive the SIGKILL fallback;
  • if the watched process group disappears while the owner is still alive, the helper disarms and exits cleanly, avoiding a stale-PGID kill on later owner EOF;
  • POLLERR/POLLNVAL on 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:

  1. ACPX_LIFELINE_HELPER override
  2. package-relative prebuilt helper, if present in a source checkout or future platform package
  3. lazy one-time compile into ~/.acpx/native/lifeline-<version>-<platform>-<arch> using cc

If 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

  • POSIX-only. Windows keeps current behavior.
  • Process-group cleanup, not a universal process-tree reaper. A descendant that deliberately starts a new session/process group can escape.
  • There is still a tiny best-effort window between bridge spawn and watchdog spawn where an immediate owner SIGKILL can orphan the bridge.
  • If a helper starts and then immediately dies, abrupt-death cleanup is not protected, but normal close still group-kills the bridge descendants.

Regression coverage

Added real-process tests for:

  • initialize failure with a long-lived bridge grandchild;
  • normal close with long-lived bridge grandchildren;
  • cooperative bridge exit while a TERM-resistant grandchild survives;
  • native lifeline SIGKILL fallback after the bridge leader exits;
  • native lifeline disarm when the watched process group disappears before owner EOF;
  • queue-owner SIGKILL cleanup with a TERM-resistant descendant.

Verification

Final HEAD: 6c5a4f0e577ff677786f2b289e7785f59e83846a

Local validation:

  • pnpm run check -> pass (864 runtime tests + 109 flow/runtime coverage tests, 0 failures; coverage gates passed)
  • pnpm run mutate -> pass (Stryker score 90.55, threshold 80, 0 timeouts)
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base upstream/main -> clean, no accepted/actionable findings
  • pnpm dlx slophammer-ts@latest rules --format text -> pass
  • pnpm dlx slophammer-ts@latest dry . -> DRY candidates: 0; maximum: 0
  • pnpm dlx slophammer-ts@latest check . --only ts.dependency-boundaries-required -> pass

GitHub CI on ubuntu-24.04:

  • Format, Typecheck, Lint, Build, Conformance Smoke, Test, Mutation, Slophammer -> pass
  • Docs -> skipped by scope

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).

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
@superWorldSavior superWorldSavior requested a review from a team as a code owner July 5, 2026 04:19
@clawsweeper

clawsweeper Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 5, 2026, 10:39 PM ET / 02:39 UTC.

Summary
The branch adds POSIX group-aware ACP bridge teardown, a native pipe-lifeline helper with lazy resolver/build packaging, and real-process lifecycle regression tests.

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.

  • Changed surface: 11 files, +1691/-26. The PR spans runtime process management, native C code, package scripts, and real-process tests.
  • Native execution surface: 1 C helper, 1 build script, 1 runtime resolver added. The helper and lazy compile/cache path are the main merge-risk surfaces beyond ordinary TypeScript behavior.
  • Current checks: 9 successful checks, 1 skipped docs job. The final GitHub status is terminal and green for the code paths this PR changes.

Root-cause cluster
Relationship: canonical
Canonical: #433
Summary: This PR is the active canonical landing candidate for bridge process-tree lifecycle cleanup; the earlier cooperative-close PR is closed unmerged and superseded by this branch.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Get explicit maintainer sign-off on the POSIX native helper, lazy compile/cache behavior, and env override contract.

Risk before merge

  • [P1] Merging this adopts a POSIX native helper, lazy local compilation/cache behavior, and an ACPX_LIFELINE_HELPER override surface that maintainers should explicitly own as runtime contract.
  • [P1] The watchdog is intentionally best-effort: Windows remains unchanged, re-detached descendants are out of scope, and helper start-then-exit failures can leave abrupt-death cleanup unprotected.

Maintainer options:

  1. Accept after lifecycle sign-off (recommended)
    Maintainers can accept the POSIX native-helper contract and merge because final CI, proof, and review are now clean.
  2. Request a narrower split
    Ask for cooperative process-group close cleanup to land separately if the native watchdog and lazy compile path are too much operating surface for this PR.
  3. Pause for native-helper policy
    Pause or close if maintainers want a broader cross-platform or native-code policy before changing bridge ownership semantics.

Next step before merge

  • [P2] No narrow automated repair remains; the next step is maintainer acceptance of the native helper and process-lifecycle semantics.

Maintainer decision needed

  • Question: Should acpx adopt this POSIX native pipe-lifeline and lazy-compiled helper as the supported bridge lifecycle model for abrupt owner death?
  • Rationale: The patch has no concrete code finding, but only maintainers can accept the long-term operating contract around native helper compilation, process-group signals, POSIX-only coverage, and package ownership.
  • Likely owner: steipete — Recent queue-owner shutdown and runtime lifecycle history makes this the strongest routing owner for accepting the process-lifecycle contract.
  • Options:
    • Accept the lifeline contract (recommended): Treat the POSIX helper and lazy compile/cache path as the supported best-effort lifecycle model and merge after maintainer review.
    • Split the lifecycle change: Land cooperative process-group close cleanup separately and decide abrupt owner-death reaping in a narrower follow-up.
    • Pause for native policy: Hold the PR until maintainers define whether runtime native compilation and helper override env vars belong in acpx core.

Security
Cleared: The diff adds native helper execution, a build script, an env override, and lazy local compilation, but I found no concrete secret, dependency, remote-download, permission, or supply-chain regression.

Review details

Best 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 changes

Label justifications:

  • P1: The PR targets orphaned bridge descendants that can accumulate until new sessions hang, which is a broken agent workflow affecting real users.
  • merge-risk: 🚨 compatibility: The PR changes POSIX bridge process-group behavior and adds a native helper/env override path that existing operators may need to understand during upgrade.
  • merge-risk: 🚨 availability: The change is intended to prevent process leaks and hangs, but helper failure modes and POSIX-only coverage remain availability-sensitive operating behavior.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body and latest comments include copied terminal validation for the real process lifecycle paths plus final green CI, so no contributor proof action remains.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and latest comments include copied terminal validation for the real process lifecycle paths plus final green CI, so no contributor proof action remains.
Evidence reviewed

What I checked:

  • Repository policy read and applied: AGENTS.md was read fully; its guidance treats runtime/session behavior and new conventions as product surface needing full validation and maintainer scrutiny. (AGENTS.md:37, 6a24a546d234)
  • Vision context supports robust lifecycle work: VISION.md identifies lifecycle management and backend reliability as core acpx concerns, which supports the problem area while still requiring convention scrutiny. (VISION.md:43, 6a24a546d234)
  • Current main only signals the bridge PID: Current main spawns the ACP bridge without detaching and closes by calling child.kill(signal), so bridge-spawned descendants are not directly targeted. (src/acp/client.ts:1412, 6a24a546d234)
  • PR gates detached spawn on lifeline helper availability: The branch resolves or compiles a lifeline helper before spawn and only uses detached process-group mode on non-Windows when a helper is available. (src/acp/client.ts:716, 6c5a4f0e577f)
  • PR reaps live process groups on close: The close path now checks for a live process group, sends SIGTERM/SIGKILL to the group, and waits for group cleanup rather than treating bridge PID exit as sufficient. (src/acp/client.ts:1456, 6c5a4f0e577f)
  • PR handles initialize failure through the same termination path: Initialize/startup failure now normalizes the error and then calls terminateInitializeFailedProcess before releasing the lifeline watchdog. (src/acp/client.ts:890, 6c5a4f0e577f)

Likely related people:

  • steipete: Recent merged queue-owner shutdown work and current-main release/blame touch the process lifecycle paths most affected by this PR. (role: recent lifecycle contributor; confidence: high; commits: 87327c6e4ac2, 43013ead1ab8, 6a24a546d234; files: src/cli/session/queue-owner-runtime.ts, test/queue-owner-lifecycle.test.ts, src/acp/client.ts)
  • vincentkoc: Recent ACP client and queue/process hardening commits touched adjacent runtime process handling surfaces. (role: recent area contributor; confidence: high; commits: 77715c8d7c18, de042d12cd63, f29a0e5ae133; files: src/acp/client.ts, src/acp/client-process.ts, src/cli/queue/lease-store.ts)
  • superWorldSavior: Beyond this PR, this contributor is credited as co-author on the merged queue-owner shutdown replacement and authored the earlier related process-lifecycle branch. (role: adjacent process-lifecycle contributor; confidence: medium; commits: 87327c6e4ac2; files: src/cli/session/queue-owner-runtime.ts, test/mock-agent.ts, test/queue-owner-lifecycle.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (10 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-05T05:11:42.192Z sha c4272a5 :: needs changes before merge. :: [P1] Handle watchdog exec failure before detaching
  • reviewed 2026-07-05T09:38:32.143Z sha 8f3c514 :: needs changes before merge. :: [P1] Prove the watchdog stays alive before returning
  • reviewed 2026-07-05T10:48:54.498Z sha 5f78cb6 :: needs changes before merge. :: [P1] Terminate detached startup failures by group
  • reviewed 2026-07-05T10:52:58.410Z sha 5f78cb6 :: needs changes before merge. :: [P1] Terminate detached startup failures by group
  • reviewed 2026-07-05T14:47:54.624Z sha 6c5a4f0 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-05T14:52:50.636Z sha 6c5a4f0 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-05T14:57:37.404Z sha 6c5a4f0 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-05T15:06:22.435Z sha 6c5a4f0 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 5, 2026
@superWorldSavior

Copy link
Copy Markdown
Contributor Author

Addressed the P1 (detach without a lifeline could regress abrupt group-signal death):

  • POSIX detachment is now gated on a resolved lifeline helper. spawnAgentProcess resolves/compiles the helper before the spawn; agentGroupLeader (and therefore detached, the close-time group reap, and the watchdog) is only enabled when a working helper resolves. If no helper compiles/resolves, the bridge stays attached with the pre-lifeline single-PID kill — zero behavior change. We only ever detach when a watchdog can reap the group.
  • Regression coverage on both paths: existing helper-present tests (grandchild reaped on cooperative close + abrupt owner SIGKILL) and a new helper-unavailable test (ACPX_LIFELINE_HELPER → invalid path ⇒ bridge stays attached, no watchdog, close() still kills the bridge).

Full suite 858/858, typecheck + lint clean. The native compile/cache surface is unchanged (security was already cleared) — that operational-ownership call remains yours.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 5, 2026
@superWorldSavior

Copy link
Copy Markdown
Contributor Author

Reworked the fix after a closer look — the previous agentGroupLeader = false on watchdog failure was the wrong lever.

agentGroupLeader means "the bridge was spawned detached", not "the watchdog is alive". The bridge is detached the moment a helper resolves and cannot be re-attached post-spawn, so clearing the flag did not fall back to an attached bridge — it only disabled the close-time group-kill, leaking the bridge's grandchildren on cooperative close. The new regression test asserts the opposite: a resolved-but-unusable helper still group-kills the bridge's descendants at close.

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 kill -9 -<owner-pgid> or a terminal SIGHUP, which is not the reported case; and the old rollback was not attaching anyway. A real attached fallback would need a probe or re-spawn, which we deliberately skip: the helper is compiled lazily for the host arch and blocks on poll(stdin), so it never fails to run — the failure modes are a misconfigured ACPX_LIFELINE_HELPER or a corrupt cache, where degrading to main's no-lifeline behavior is acceptable.

Also hardened:

  • Resolution now requires a regular executable file (X_OK), not mere existence — a present-but-non-executable helper is unavailable before the detach decision.
  • await waitForSpawn(watchdog) surfaces spawn errors (ENOENT/EACCES) without an unhandled rejection.

Known best-effort limit (documented in code): a helper that execs then exits immediately (e.g. a corrupt 0755 binary hitting the glibc execvp -> /bin/sh fallback) leaves the watchdog dead — close still group-kills, abrupt death is unprotected, same as main.

Full suite green (860), typecheck + lint clean.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@superWorldSavior

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated the branch after the P1 review. The fix now covers the process-tree lifecycle as three explicit cases:

  • initialize/startup failure: handleInitializeFailure now uses the same group-aware termination path instead of PID-only child.kill() before releasing the lifeline.
  • normal cooperative close: AcpClient.close() no longer treats bridge exit as sufficient when the detached process group still has descendants; it signals/waits on the group and falls back to SIGKILL.
  • abrupt owner death: the native lifeline now kills TERM-resistant descendants, but disarms itself once the watched process group disappears before owner EOF, avoiding stale PGID reuse risk.

Regression coverage added for:

  • initialize failure with long-lived bridge grandchild
  • cooperative bridge exit with TERM-resistant grandchild
  • native lifeline SIGKILL fallback after bridge leader exit
  • native lifeline disarm when the process group disappears before owner EOF
  • queue-owner SIGKILL lifecycle with TERM-resistant descendant

Verification run on final HEAD 6c5a4f0:

  • pnpm run check -> pass (864 runtime tests + 109 flow/runtime coverage tests, 0 failures; coverage gates passed)
  • pnpm run mutate -> pass (Stryker score 90.55, threshold 80, 0 timeouts)
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base upstream/main -> clean, no accepted/actionable findings
  • pnpm dlx slophammer-ts@latest rules --format text -> pass
  • pnpm dlx slophammer-ts@latest dry . -> DRY candidates: 0; maximum: 0
  • pnpm dlx slophammer-ts@latest check . --only ts.dependency-boundaries-required -> pass

The branch was also squashed down to two commits for a cleaner review history.

@clawsweeper

clawsweeper Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 5, 2026
@superWorldSavior

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Final CI state is now fully terminal on HEAD 6c5a4f0:

  • Build, Conformance Smoke, Format, Lint, Slophammer, Test, Typecheck: pass
  • Mutation: pass (6m55s)
  • Docs: skipped by scope

Requesting a final refresh because the previous ClawSweeper review still listed Mutation as pending at review time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant