Repository navigation
fix(runtime): budget explicit terminal close for the daemon's immediate-kill verdict - #22385
Conversation
…te-kill verdict Explicit terminal close (worker-release, worker-stop, `orca terminal close`) gave the daemon kill RPC a fixed 2s deadline. The daemon's immediate kill captures descendants, SIGTERMs them with a 2.5s verification window, then waits up to 8s for the root's physical exit. An agent that runs exit hooks after SIGTERM (Muse: ~3s) outlived main's 2s timeout, so close reported the PTY unverifiable and worker-release returned release_unknown even though the daemon confirmed the exit ~200ms later. Derive the close budget from the daemon's own immediate-kill reply budget (now in an import-free module) plus 2s for the post-kill inventory check. A process that exits within the daemon's budget is released; a wedged process or unreachable host still times out as unverifiable.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Shared kill budget — new import-free
src/main/daemon/immediate-kill-reply-budget.tsbecomes the single source for the daemon's immediate-kill timings and derivesIMMEDIATE_KILL_REPLY_BUDGET_MS(capture 1s + verify 2.5s + two 250ms table reads + 8s physical exit = 12s). - Daemon reads the shared timings —
session-termination-controller.tsimports and re-exportsIMMEDIATE_KILL_PHYSICAL_EXIT_TIMEOUT_MS(existing importers keep working), andterminal-descendant-shutdown.tsnow reads the verify/table windows from the module. - Main's explicit-close budget derived from it —
EXPLICIT_TERMINAL_CLOSE_STOP_TIMEOUT_MSgrows from a fixed 2s toIMMEDIATE_KILL_REPLY_BUDGET_MS + 2000(14s), which is thedeadlineMsthat becomes the daemonkillrequest timeout. - Regression coverage — new
terminal-close-slow-agent-exit.test.tsruns the real daemon over a socket plus the real runtime close/release logic: slow 2.3s exit endsreleased, aSessionNotFoundErrorreply counts as exited, and budget expiry endsrelease_unknownwith the fallback stop.
I traced the daemon sequence (killWithDescendantSweep → terminateShutdownDescendants → forceKillAndWaitForExit) and the budget's worst case lands around 12.0–12.3s, so main's 14s leaves the follow-up inventory check its 2s. I also confirmed the new test is real coverage: reverting EXPLICIT_TERMINAL_CLOSE_STOP_TIMEOUT_MS to 2000 locally makes three of its four cases fail (the slow-exit case returns at ~2s and budgetMs reads 2000).
ℹ️ Capture timeout is mirrored, not shared
The budget module documents IMMEDIATE_KILL_CAPTURE_TIMEOUT_MS as mirroring DESCENDANT_SNAPSHOT_TIMEOUT_MS, but the daemon's initial capture still reads the latter directly, so the two values can drift apart. The new test's IMMEDIATE_KILL_CAPTURE_TIMEOUT_MS === DESCENDANT_SNAPSHOT_TIMEOUT_MS assertion does catch that drift, so this is intentional and safe — flagging only so the coupling is understood as deliberate.
Technical details
# Capture timeout mirror
## Affected sites
- `src/main/daemon/immediate-kill-reply-budget.ts:7-8` — declares `IMMEDIATE_KILL_CAPTURE_TIMEOUT_MS = 1000` as a copy of `DESCENDANT_SNAPSHOT_TIMEOUT_MS`.
- `src/main/pty-descendant-termination.ts:217` — `captureDescendantSnapshot` uses `deps.timeoutMs ?? DESCENDANT_SNAPSHOT_TIMEOUT_MS`; the kill-sweep call sites set neither, so this is the real capture bound.
- `src/main/runtime/terminal-close-slow-agent-exit.test.ts:106` — pins the two constants equal.
## Required outcome
- None. The module stays import-free (its stated purpose), and the test is what keeps the copy honest, so this is a note rather than a request.
## Suggested approach (optional)
- If a future change removes the drift test, prefer importing `DESCENDANT_SNAPSHOT_TIMEOUT_MS` (or asserting equality at module load) instead of relying on the comment.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change adds a force-kill retry wait to the immediate-kill reply budget and uses shared timeout constants in daemon shutdown code. Terminal-close handling now sets its stop timeout to the immediate-kill reply budget plus 2,000 ms. Integration tests cover slow exits, delayed exit frames, and stop deadlines that expire while a process is still running. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The longer close window allows delayed daemon exits to be confirmed, while stuck processes still produce an unconfirmed outcome. No actionable merge risk introduced by this change remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: b0289ba1-3ed8-499f-bb6c-ace77de69b22
📒 Files selected for processing (5)
src/main/daemon/immediate-kill-reply-budget.tssrc/main/daemon/session-termination-controller.tssrc/main/daemon/terminal-descendant-shutdown.tssrc/main/runtime/orca-runtime-core.tssrc/main/runtime/terminal-close-slow-agent-exit.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
- Derived the budget from the force-kill retry —
SESSION_FORCE_KILL_RETRY_MS(250 ms) moved intoimmediate-kill-reply-budget.tsand is now part ofIMMEDIATE_KILL_REPLY_BUDGET_MS(12,250 ms), raising main'sEXPLICIT_TERMINAL_CLOSE_STOP_TIMEOUT_MSto 14,250 ms;session-termination-controller.tsre-exports it so existing importers are unchanged. - Exercised an expired close deadline — the budget-expiry case now drives the real
stopAndWaitPtyFromRuntimeControlleragainst a wedged daemon session with a frozenDatejumped to the deadline, instead of a stub that returnedfalseimmediately.
I traced the daemon sequence (killWithDescendantSweep → terminateShutdownDescendants → forceKillAndWaitForExit, including requestForceKillWithRetry's single retry wait) and the new term closes the gap the prior review flagged. The test is real coverage: reverting EXPLICIT_TERMINAL_CLOSE_STOP_TIMEOUT_MS to 2000 fails 3 of its 4 cases.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Dropped the tautological expiry assertion — the budget-expiry test no longer computes
expiredByand asserts it is>= 0. WithDatefaked and never advanced, that value was always0, so the check could not fail. The prior inline thread atterminal-close-slow-agent-exit.test.ts:165is resolved.
The remaining coverage still holds: the test jumps the frozen clock to deadlineMs, asserts budgetMs > IMMEDIATE_KILL_REPLY_BUDGET_MS, and keeps the release_unknown receipt, single fallback-kill, and unverifiable liveness assertions. This delta is the only change since the prior pullfrog review, and it addresses that review's one open thread.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
1 participant
ELI5
When Orca releases an orchestration worker, it closes the worker's terminal and checks that the agent really stopped. It only waited 2 seconds for that check. Some agents take longer to shut down cleanly (Muse Code takes about 3 seconds), so Orca reported "could not confirm it stopped" even though it had. Orca now waits as long as the terminal daemon's own shutdown is allowed to take.
What Changed
Before:
orca orchestration worker-releaseon a Muse worker returnedrelease_unknown. The error was "The agent terminal was closed but its process could not be confirmed stopped", andworker-showthen reported livenessunverifiable. The dev log showedRequest kill timed out after 2000ms, although the daemon log shows the session exited cleanly about 3 s after the release.After: the same release returns
released, and liveness readsexited.Root cause:
EXPLICIT_TERMINAL_CLOSE_STOP_TIMEOUT_MSwas 2 s (src/main/runtime/orca-runtime-core.ts). That became the timeout on main'skillrequest to the terminal daemon. The daemon's immediate kill can legitimately take much longer by design:When main timed out first, the close path issued a second stop and marked the terminal
unverifiablestraight away. Worker release saved that asrelease_unknown, and the exit that arrived moments later couldn't change the saved receipt.Fix:
src/main/daemon/immediate-kill-reply-budget.ts), and the daemon reads its values from there. They add up to 12 s.Why
The two sides disagreed about how long a kill may take. Main gave up before the daemon had finished its own bounded check, so a clean exit was reported as unknown. Deriving main's timeout from the daemon's timings fixes the cause instead of picking a new magic number.
The rules in
docs/reference/ssh-execution-boundary.mdstill hold:exited.release_unknown/unverifiable.Linked Issue
Found while validating #22383 (Muse orchestration workers, #19823).
Visual Proof
Before (
main, Muse worker):After (this PR, two Muse workers, about 2.4 s per release):
exited(sourceresource_release).Request kill timed out.session-exitedcomes about 2.1 s after release, thensession-killed immediate:true, with nosession-kill-failed.Both worker tabs are gone after release, and only the coordinator tab remains:
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Manual (macOS): a dev build with two real Muse Code workers; each ran its task, sent
worker_done, and was released as above.New test file:
src/main/runtime/terminal-close-slow-agent-exit.test.tsruns the real daemon over a socket, the real runtime close and the real release logic. It covers:released(this test fails with the old 2 s),release_unknown, with the second stop issued and the terminal markedunverifiable,SessionNotFoundErrorreply for a process that had already exited counts as exited,Existing suites: daemon, PTY, worker-release, worktree-teardown, relay and
orca-runtimeall pass, as dotc:nodeand the changed-code quality gate.AI Disclosure
Anthropic Claude assisted with implementation, validation, and this pull request description.
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
orca terminal close, mobile close) can now wait up to 14 s instead of 2 s, and only for a process that won't stop. That's still within the CLI's 60 s and federated release's 30 s timeouts. If an SSH provider is gone entirely, close still fails fast.taskkill's own stop timings, against the new 14 s.-1can overwrite the recorded exit reason withstop_unverified. The liveness verdict staysexited.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (tc:node and the affected suites pass locally; CI covers the rest)