This repository was archived by the owner on Nov 7, 2026. It is now read-only.
Repository navigation
fix(core): reap shell process groups on cancellation - #380
Merged
Merged
Conversation
Plan: 1. Trace ShellTool cancellation, spawner scope release, SessionProcessor cleanup, and the Bun test boundary with PID/start-identity-owned real fixtures; establish whether the production path or fixture lifetime leaks before emergency cleanup. 2. Repair the responsible owner and add producer-driven bounded lifetime coverage that preserves interrupted output and TERM-handler behavior; make emergency cleanup cover success, assertion failure, interruption, and timeout. 3. Remove the guarded behavior to prove the regression fails within its bound and still reaps its own fixtures, restore it, then run every changed test file whole and record the evidence.
Author
|
Session ID: unknown |
brentyates-swx
changed the title
brentyates-swx
marked this pull request as ready for review
November 2, 2026 03:02
This was referenced Nov 2, 2026
brentyates-swx
added a commit
that referenced
this pull request
Nov 6, 2026
* docs: plan transport and lifecycle backport Implement the assigned transport and lifecycle behavior in existing fork owners. - Pick each donor in its own -x commit; omit shell-failure UI and absent CLI/MCP payload. - Disable Bun’s independent fetch deadline only for long network clients and native provider requests; preserve caller signals, domain deadlines, and the #303 watchdog. - Track detached POSIX groups only while cleanup is owed, preserving #380 escalation and release of intentional descendants; route terminal SIGHUP through the TUI lifecycle. - Add real-server and child-process regressions with removal proofs, then record donor coverage and focused checks in CHANGESET.md. * fix(tui): keep long-pending requests past Bun's fetch deadline (anomalyco#55149) Give the TUI client timeout: false, share the no-timeout fetch helper across run, mini and the TUI via @opencode/util/no-timeout-fetch, and surface existing-session shell failures with a toast and composer restore. Closes anomalyco#54974 Co-authored-by: Finn763 <[email protected]> (cherry picked from commit 52a10f5) * fix(core): disable Bun's fetch idle timeout for provider requests (anomalyco#55007) (cherry picked from commit 460c626) * fix(util): kill spawned process groups when opencode exits (anomalyco#59411) (cherry picked from commit e2bc79d) * fix(core): type Bun fetch timeout overrides * test(opencode): verify mini attach timeout wiring Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * test(core): verify abrupt owner process-group cleanup Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * test(tui): verify SDK long-call fetch policy Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * fix(tui): preserve nohup SIGHUP behavior Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * test(core): verify native fetch timeout override Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * test(core): prove native requests outlive default idle timeout Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * test(opencode): verify attached runs survive Bun idle timeout Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * docs: record transport and lifecycle backport Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * docs: clarify backport regression ownership Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * test(opencode): allow Bun 1.4.4 server startup window Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * test: shorten idle-timeout acceptance tests Run the native and attached held-response witnesses in bounded Bun 1.4.4 child processes with BUN_CONFIG_HTTP_IDLE_TIMEOUT=2 so they stay out of the multi-minute range. Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * test(tui): prove SDK long calls survive Bun idle expiry Exercise SDKProvider's external SDK and SSE paths with a held event stream, a real timeout control, and a completed event after the injected short idle expiry. Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * test(opencode): prove mini attach survives Bun idle expiry Hold the real mini attach session response in a Bun 1.4.4 child process, observe an unmodified control fetch time out first, then release the expected 404 response. Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * test(tui): make SIGHUP fixture reports truthful Keep the nohup fixture scoped to listener policy and process survival, and add a non-TTY non-nohup default-disposition witness while retaining lifecycle assertions only from the real TUI producer. Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * fix(tui): clean Worker-owned process groups on exit Acknowledge Worker group ownership and release events, kill and await remaining groups before the shutdown reply, and use the recorded PGID ledger as the bounded TUI-owner backstop when shutdown is unconfirmed. Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * docs: finalize transport exit review coverage Record the acknowledged TUI Worker group lifecycle, truthful SIGHUP behavior, and short Bun 1.4.4 behavioral witnesses in the backport ledger. Claude-Session: c368ed23-1e31-4f2c-b305-b84f21d79cc5 * fix(tui): centralize Worker process-group ownership * docs: plan transport exit review fixes 1. Make worker and parent ownership ledgers converge on each local acquire, release, and cleanup decision even when acknowledgement is rejected or delayed; validate recorded identities before parent-side signalling. 2. Add rejected/delayed acknowledgement regressions for acquire, release, cleanup announcement, and release/reacquire identity fallback. 3. Harden fixture cleanup and deadlines; add Linux-only guards; correct CHANGESET ownership and SIGHUP claims; remove only test hooks no longer needed. 4. Verify focused changed cases and then each importing test file once under checksum-verified Bun 1.4.4; commit without pushing. * fix(core): reconcile process-group acknowledgement failures Reconcile Worker and parent ownership ledgers on local acquire, release, and cleanup facts, independent of rejected or delayed acknowledgements. Validate recorded member identities before signalling and preserve released member identities across reacquisition. Bound identity-owned process fixture cleanup, gate procfs child fixtures to Linux, retain portable SDK cancellation coverage, and correct the #410 SIGHUP and ownership ledger. Claude-Session: unknown * test(opencode): gate shell output on spawn readiness Wait for the process spawn acknowledgement before the ShellTool first-output deadline and reap the child on readiness failure. Tighten process-group fixture types for package typecheck. Claude-Session: unknown * fix(core): restore cross-platform process group cleanup Claude-Session: unavailable (OpenCode session) * fix(core): unify process-group ownership decisions Claude-Session: unavailable (OpenCode session) * fix(core): publish process-group ownership updates Claude-Session: unavailable (OpenCode session) * test(opencode): cover cancellation-overlap ownership Claude-Session: unavailable (OpenCode session) * test(core): wait for process-group cleanup Claude-Session: unavailable (OpenCode session) * test(opencode): harden process-group test results Claude-Session: unavailable (OpenCode session) * test(opencode): validate cancellation result shape Claude-Session: unavailable (OpenCode session) * test(opencode): type fixture results as unknown Claude-Session: unavailable (OpenCode session) --------- Co-authored-by: Finn763 <[email protected]> Co-authored-by: Finn763 <[email protected]> Co-authored-by: Aiden Cline <[email protected]>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fixes #379.
The Core spawner preserves process-group cleanup across leader exit, kill-fiber interruption, and scope release. Graceful group waits are used only when
forceKillAfteris defined; zero means immediate escalation. After SIGKILL, cleanup has a final bound and reports surviving member PIDs. Failed initial signals from an already-exited leader no longer abort scope release before pipe detachment. ShellTool fixtures retain their interruption and output assertions while verifying the exact TERM-resistant child identity.Handoff — review round 7
Head:
fe40da0ddb589f1ad5a0d05fcd7d672ca3708a9e(pushed; live branch ref matches, PR #380 is still draft). All required findings from rounds 1–7 are fixed; none are deferred.Done: The pending-close fixture now recovers from a failed PID-file read while its recorded parent is still live: it finds the sole direct child by parent PID, verifies both start identities and the child's detached process group, records the child, and reaps it. The fixture keeps the parent alive through the acquisition result. Round-6 atomic publication and the round-5 pre-wait ownership anchor remain intact.
Round 7 regression: The bounded real-process regression holds the PID temp file empty past the reader's 5-second bound while a TERM-resistant detached child runs. Recovery reaps the child and asserts its group is gone before emergency cleanup. Temporarily disabling the recovery branch made the test fail at
recovered === truein 5.19 seconds; the recorded-parent finalizer then recovered and cleaned the child and parent, and no survivor checks failed. With recovery restored, the focused test passed (1 pass, 0 fail, 4 expect calls).Reviewer PID
1372805is not covered by a local recorded identity, so it was left untouched.Round 6 carried forward: The atomic-publication regression remains green; with direct final-path publication restored, it failed at the temp-path assertion and its recorded-group cleanup left no survivors.
Whole-file validation on 2026-11-02:
packages/core:bun run test -- ./test/effect/cross-spawn-spawner.test.ts— 40 pass, 0 fail, 143 expect calls.packages/opencode:bun run test -- ./test/tool/shell.test.ts— 33 pass, 0 fail, 231 expect calls.bun typecheckpassed from both package directories./home/byates/fm-llmrouter/bin/fm-commit-identity-check.sh --crew .passed for1afad3765fe9940cc81b000db01b8599b5c3e0b8..fe40da0ddb589f1ad5a0d05fcd7d672ca3708a9e.bin/fm-lint.sh.Remaining: None in this crew. Next step: Firstmate re-reviews this pushed head to convergence, then runs
/home/byates/fm-llmrouter/bin/fm-final-suite.sh opencode-shell-fixture-leak-379 <immutable-merge-head>for the required./tests/run.shgate.