Repository navigation
fix(orchestration): record why a terminal's process is gone (STA-4603, STA-4536) - #15244
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (39)
🚧 Files skipped from review as they are similar to previous changes (36)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change adds structured terminal exit causes for normal exits, signals, deliberate stops, unverified stops, and unavailable host status. Causes propagate through PTY providers, daemon sessions, IPC callbacks, runtime state, terminal summaries, and wait results. Runtime dispatch failures now store termination reasons and use pane identity fallback. Database schema version 29 adds the nullable 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/runtime/orchestration/db/dispatch-context/dispatch-completion.ts (1)
80-105: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestrict
terminationReasonto the exit-cause union.Line 80 accepts any string.
DispatchContextRow.termination_reasonpermits onlyTerminalExitCause['kind'] | null. An unsupported value can be persisted and returned as an invalid typed row.Change
terminationReasontoTerminalExitCause['kind'].
🧹 Nitpick comments (1)
src/shared/terminal-exit-cause.ts (1)
1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the module comment.
Keep the module rationale in one or two lines. The current banner repeats implementation detail that the types and resolver show.
As per coding guidelines, “Comments must be concise, non-obvious, and brief—prefer one line.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fbd6f41-5909-4159-9b9c-171cea61849e
📒 Files selected for processing (38)
src/main/daemon/daemon-pty-adapter.test.tssrc/main/daemon/daemon-pty-adapter.tssrc/main/daemon/daemon-pty-router-events.tssrc/main/daemon/daemon-server.tssrc/main/daemon/daemon-stream-events.tssrc/main/daemon/pty-subprocess.tssrc/main/daemon/session-output-plane.tssrc/main/daemon/session-subprocess-handle.tssrc/main/daemon/session.test.tssrc/main/daemon/session.tssrc/main/daemon/terminal-host-create-contract.tssrc/main/ipc/pty-daemon-ssh-lease-lifecycle.test.tssrc/main/ipc/pty-listener-teardown-and-orphans.test.tssrc/main/ipc/pty-runtime-kill-and-exit.test.tssrc/main/ipc/pty.tssrc/main/providers/local-pty-provider-io-events.test.tssrc/main/providers/local-pty-provider-shutdown.test.tssrc/main/providers/local-pty-provider.tssrc/main/providers/local-pty-utils.test.tssrc/main/providers/local-pty-utils.tssrc/main/providers/macos-tcc-login-shell.tssrc/main/providers/pty-provider-contract.tssrc/main/runtime/exit-provenance-audit.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/orchestration/db/contract-constants.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-completion.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-lookup.tssrc/main/runtime/orchestration/db/schema/create-graph-tables-sql.tssrc/main/runtime/orchestration/db/schema/migrate-v13-v29.tssrc/main/runtime/orchestration/db/schema/migrate.tssrc/main/runtime/orchestration/federation-acknowledgment-migration.test.tssrc/main/runtime/orchestration/mutation-receipt-capacity.test.tssrc/main/runtime/orchestration/orchestration-db-retention-pagination.test.tssrc/main/runtime/orchestration/run-coordinator-handle-migration.test.tssrc/main/runtime/orchestration/types.tssrc/shared/runtime-types.tssrc/shared/terminal-exit-cause.test.tssrc/shared/terminal-exit-cause.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 4 remain after this review.
…, STA-4536)
An operator close, a clean finish and an OOM kill all left the same record:
`dispatch_contexts.status='failed'` with `last_failure="Agent exited with
code 0"`, and a terminal record carrying no exit information at all. A
coordinator could not tell "lane delivered and went home" from "lane died",
and post-mortem triage had to reconcile against external CLI logs to prove a
deliberate close was not a crash.
Two separate things were broken.
The exit datum was destroyed before Orca could read it. node-pty reports a
signalled death as `{exitCode: 0, signal: N}` and both PTY paths destructured
`exitCode` alone, so SIGKILL arrived as 0. On macOS every PTY is wrapped in
`/usr/bin/login -flpq` for TCC attribution, and login(1) returns its own
status: a shell that exits 42 and a shell that is SIGKILLed both report
`{exitCode: 0, signal: 0}`.
The teardown intent was known and thrown away. `closeTerminal` knew it was an
operator close and told the orchestration DB nothing; `rememberSyntheticKillExit`
tracked "this exit was synthesized on a requested stop" only to de-duplicate a
late provider exit.
This adds `TerminalExitCause` — `operator_close`, `signaled`, `exited`, or
`unknown` — built from evidence someone actually holds and never inferred from
a bare number. Intent is recorded before the stop, so a requested teardown
stays an operator close even when the provider delivers the exit itself. A
wrapper spawn reports `unknown` rather than a status it cannot vouch for.
The cause is surfaced on `RuntimeTerminalSummary.exitCause` and the wait
result (both additive), and persisted on a new `dispatch_contexts
.termination_reason` column (schema v29). A deliberate close no longer
escalates to the coordinator.
Also fixes an unreported bug found while reproducing this: an explicit
whole-tab close drops the leaf from the renderer graph before the exit lands,
so `failActiveDispatchOnExit`'s leaf-only walk found nothing and left the
dispatch reading `dispatched` forever against a dead process. Exits now settle
from the PTY's own handle and pane key too.
Verified on a live runtime with a real daemon. With the macOS wrapper active:
close -> operator_close, crash -> unknown, clean exit -> unknown. With it
disabled (the Linux/Windows shape): close -> operator_close, SIGKILL ->
signaled/9, `exit 7` -> exited/7. All three cases were identical before.
Three findings from independent review of the previous commit, each of which
would have let the cause model state something it could not establish — the
exact failure this work exists to remove.
1. A stop intent outlived its process. `advancePtyLifecycleGeneration` did not
clear `stopRequestedPtyIds`, despite the comment claiming it did. A stop
whose exit never arrived stayed armed across a same-id respawn, so the NEXT
process's crash was filed as an operator close — and silently not escalated.
2. A bare exit code was read as a clean finish. An exit delivered without a
cause — from a daemon older than the field, or over the SSH relay, which
still forwards only `code` — was reconstructed as `exited(code)`. An OOM
kill on an SSH host arrived as `exited(0)`, indistinguishable from success,
which is the original bug wearing a new name. Unreported exits now resolve
to `unknown: cause_unreported` and say so.
3. Operator intent outranked the evidence that the stop never happened. Intent
is recorded before a stop is known to have succeeded, so a synthetic `-1`
("asked it to die, never saw it") became `operator_close` and suppressed the
escalation. A process that may still be running against a revoked dispatch
is an incident, not a routine teardown. Intent now yields to
`stop_unverified`.
Also strengthens the tests review called out as not covering the shipping
wiring: a close now drives the real `closeTerminal` -> `stopAndWait` path
rather than calling `markPtyStopRequested` directly, the local provider is
asserted to carry node-pty's signal through rather than only some cause, and
the no-escalation assertion first proves the dispatch actually settled instead
of passing vacuously when nothing settled at all. Each new test was confirmed
to fail with its fix reverted.
Not addressed here, and called out in the PR: the SSH relay wire still carries
no cause, so remote exits report `unknown` rather than `signaled`. That needs
the relay protocol and is a separate change; reporting `unknown` is honest in
the meantime.
Rebase onto #15235, which landed the delivery half of this: worker-exit escalations now reach lightweight Run coordinators instead of being gated on a legacy table nothing writes. That PR delivers the message; this one decides what the message says. Both edit the same block, so the resolution keeps every part of the delivery fix — run-aware addressing, the best-effort try/catch, the `notifyMessageArrived` wake, and the prose body — and swaps the exit code it described for the resolved cause. The escalation subject and body now name the cause rather than a code, since "exited with code 0" was the sentence that sent an operator chasing an OOM that never happened. The payload keeps `exitCode` for readers that already parse it and adds `exitCause` beside it. A deliberate close returns before the escalation, so closing a pane no longer pages a coordinator. Also sharpens an over-correction in the previous commit. Refusing to read *any* unreported bare code as a status was too broad: node-pty only ever pairs a signal with 0, and a wrapper spawn only ever returns 0, so a nonzero code really is the child's wait status and stays `exited(code)`. Only 0 is ambiguous, and only 0 now resolves to `unknown: cause_unreported`. That keeps a remote `exit 137` over the SSH relay informative while still refusing to call a possible SIGKILL a clean finish.
Jinwoo-H
force-pushed
the
readiness-exit-provenance-sta-4603-4536-claude
branch
from
August 18, 2026 19:46
4cd2189 to
3fd2ba9
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
paidaxingyo666
pushed a commit
to paidaxingyo666/Manta
that referenced
this pull request
Aug 21, 2026
This was referenced Aug 26, 2026
[Feature]: Recover supervised cycles missing worker_done using attempt-linked result evidence
#14829
Closed
Open
dallascrilley
pushed a commit
to dallascrilley/orca
that referenced
this pull request
Aug 27, 2026
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
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
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.
ELI5
When an agent's terminal dies, Orca wrote down that it died but not why. Closing a pane on purpose, an agent finishing cleanly, and an agent getting OOM-killed all left the identical note:
last_failure = "Agent exited with code 0". A coordinator pollingterminal showordispatch-showcould not tell "lane delivered and went home" from "lane crashed" — one reporter chased an OOM that never happened.This makes Orca record the cause, and — where it genuinely cannot know — say so instead of guessing.
Why it was broken (two separate layers)
1. The exit number was destroyed before Orca could read it.
node-pty reports a signalled death as
{exitCode: 0, signal: N}. Both PTY paths destructuredexitCodealone, so SIGKILL arrived as0.On macOS every PTY is wrapped in
/usr/bin/login -flpqfor TCC attribution (TCC 'access to other apps' data' prompt re-appears on every subprocess launch (e.g. 1Password op CLI) instead of being remembered #6996, [Regression] TCC 'access to other apps' data' prompt re-appears on every subprocess launch (op CLI) — regression of #6996 since ~1.4.139 #8985), andlogin(1)returns its own status. Proved directly with node-pty:exit 42sh{exitCode: 42, signal: 0}{exitCode: 0, signal: 9}{exitCode: 0, signal: 0}{exitCode: 0, signal: 0}The stop paths also synthesize codes (
0on a verified stop,-1on an unconfirmed one), so one number carried three unrelated meanings.2. The teardown intent was known and thrown away.
closeTerminalknew it was an operator close and told the orchestration DB nothing.rememberSyntheticKillExitliterally tracked "this exit was one I synthesized on a requested stop" — only to de-duplicate a late provider exit, then discarded it.What changed
TerminalExitCause—operator_close/signaled/exited/unknown— built only from evidence someone actually holds:signalis now captured in both PTY paths and carried over the daemon wire (additivecausefield on the exit event; a daemon predating it omits it and readers fall back tounknown).unknown, not a status it cannot vouch for. This is the deliberate choice: a confident wrong answer is what caused the original misdiagnosis.RuntimeTerminalSummary.exitCauseand theterminal waitresult (both additive, old clients strip them).dispatch_contexts.termination_reasoncolumn (schema v29), alongside alast_failuresentence that no longer requires decoding a number.Also fixes an unreported bug
An explicit whole-tab close drops the leaf from the renderer graph before the exit lands, so
failActiveDispatchOnExit's leaf-only walk found nothing and left the dispatch readingdispatchedforever against a dead process. Exits now settle from the PTY's own handle and pane key too. Found while building the repro for STA-4603; arguably worse than the ambiguity both issues report, since half the operator-close population was silently invisible.Before / after
Verified on a live runtime with a real daemon (not mocks):
Agent exited with code 0operator_closeoperator_closedispatchedoperator_closeoperator_closeAgent exited with code 0unknown(host cannot report)signaled/ 9exit 7Agent exited with code 0unknown(host cannot report)exited/ 7worker_donecompletedcompleted(unchanged)completed(unchanged)The first four rows were byte-identical before this PR.
Testing
src/shared/terminal-exit-cause.test.ts(7) andsrc/main/runtime/exit-provenance-audit.test.ts(7) — the full case matrix, including the whole-tab strand and the no-escalation-on-close rule.tsc --noEmitclean on node / cli / web;oxlint --deny-warningsclean; max-lines ratchet, reliability gates, and both changed-code gates pass.ORCA_DISABLE_MACOS_LOGIN_SHELL=1to exercise the full-fidelity path.Review notes
exitCauseon the terminal summary and wait result are new optional fields old clients strip. The daemon exit event gains an optionalcause; the daemon is version-locked to the app, and an older one simply omits it. No new stream opcode. The renderer'spty:exitIPC payload is deliberately left unchanged.login(1)status loss is macOS-only; the droppedsignalwas cross-platform. Linux/Windows now get trueexited/signaledcauses. I could only test on macOS — the wrapper-disabled runs are the closest proxy for the Linux/Windows path and they exercise the same code.hasColumn. Migration tests now assert againstSCHEMA_VERSIONinstead of a hardcoded literal so the next bump doesn't break them.migrate-v13-v28.tsrenamed tomigrate-v13-v29.tsto match its contents.login(1)will never forward it. That changes shell startup injection and deserves its own PR. Until then macOS honestly reportsunknownfor process exits, which is a strict improvement over a confidentcode 0.Linked issues
Fixes STA-4603, Fixes STA-4536 (#15048, #14905).
Relates to #14891 (open, unmerged): that PR reports a lane's absence (
dead/unknown) and deliberately does not claim a cause. This supplies the cause it left out. The two do not conflict.AI Disclosure
Written with Claude Opus 5 via Claude Code, from a reproduce-first audit of both issues on latest main.
Review round (3 independent agents, Codex gpt-5.6-luna xhigh)
Three real defects were found in the first commit and fixed in
4cd2189f56. Each new regression test was confirmed to fail with its fix reverted.advancePtyLifecycleGenerationdid not clearstopRequestedPtyIdsdespite a comment claiming it did. A stop whose exit never arrived stayed armed across a same-id respawn, so the next process's crash was filedoperator_closeand silently not escalated.cause— from a daemon older than the field, or over the SSH relay — was reconstructed asexited(code). An OOM kill on an SSH host arrived asexited(0): the original bug wearing a new name. Unreported exits now resolve tounknown: cause_unreported.-1becameoperator_closeand suppressed the escalation. A process that may still be running against a revoked dispatch is an incident. Intent now yields tostop_unverified.Review also judged the tests as not covering the shipping wiring. Fixed: a close now drives the real
closeTerminal->stopAndWaitpath instead of callingmarkPtyStopRequesteddirectly; the local provider is asserted to carry node-pty'ssignalthrough; and the no-escalation assertion first proves the dispatch actually settled rather than passing vacuously.Corrections to this PR description
exit 7did recordcode 7, and the stop paths did producecode -1. The universal collision was between a normal exit-0 finish, a SIGKILL, and a confirmed operator close — allcode 0. The wrapper widened that to swallow every status on macOS. The table above should be read as macOS unless the row says otherwise.preservesAbnormalSshSurfacestill skips dispatch settlement for unconfirmed SSH exits, so the whole-tab fix is not universal. That guard is deliberate and unchanged: a detached relay PTY is designed to outlive the provider addressing it, and an unprovable absence must not settle a dispatch.Known gap, deliberately not in this PR
The SSH relay wire (
src/relay/pty-handler.ts) forwards onlycode, so a remote SIGKILL cannot yet be reported assignaled. With fix 2 above it now reportsunknown: cause_unreportedinstead of a falseexited(0). Carrying the cause across the relay touches an independently-deployed component and belongs in its own change.Verification after the review round
tscclean on node/cli/web,oxlint --deny-warningsclean, max-lines ratchet + reliability gates + both changed-code gates pass.operator_close; SIGKILL ->unknown(wrapper active).