Repository navigation
fix(orca): adapter readback, wait bound, stream faults, version gate, and doctor resilience - #2829
Conversation
…dling
Four defects in the Orca orchestration adapter, each reproduced before the fix.
C1 — a status-only `task-update` never survived its readback. `result` is
optional on the operation, but the readback demanded result equality, comparing
`JSON.stringify(undefined)` (the value `undefined`) against the stored value:
`null` on a fresh task, the prior JSON string on a task that already carried
one. Every status-only update failed with `readback_mismatch` after the mutation
had already committed. The result is now compared only when the mutation
actually carried one; the mismatch check is unchanged when it did.
H4 — `worker-start`, `check --wait`, and `ask` accept a `--timeout-ms` of up to
600_000 ms, but the child process was pinned to the 30s adapter default and
SIGKILLed every long wait before Orca could answer, turning a supported wait
into a `timeout` / `ambiguous_after_possible_commit`. The process bound now
follows the requested wait plus a 5s grace, never shrinks below the adapter
default, and is clamped at 605_000 ms so no caller can unbound the child.
H5 — `run-current` legitimately answers `{ run: null }` when nothing is bound.
The run-use readback dereferenced it and threw a raw `TypeError` out of
`execute`, bypassing the adapter's typed error contract entirely. `recordOf`
now collapses a non-object to an empty record (killing that class across every
readback plan) and the run-use plan rejects an unbound answer explicitly, so it
surfaces as the documented `readback_mismatch`.
L4 — `child.stdout` / `child.stderr` had no `error` listener, so a stream fault
became an unhandled `error` event: the process died and the promise never
settled. Both now settle through the same ambiguous-transport-loss path as a
post-spawn child `error`, reached through a new `spawnChild` seam that makes the
post-spawn failure paths testable.
M6 — `bounds timeout termination through the kill escalation path` asserted
SIGKILL but read SIGTERM on bun 1.3.14. The child installed its SIGTERM handler
in JS and the 20 ms timeout fired before the interpreter finished booting, so
the child died of the plain SIGTERM and escalation never happened. `/bin/sh`
sets the disposition to SIG_IGN before anything else runs and SIG_IGN survives
`exec`, so the child is unkillable by SIGTERM from its first instruction; the
timeout is 250 ms for margin. Verified 5/5 locally on bun 1.3.14.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013gGxGgKskzyzr1HRUB6cV3
`versionParts` matched `(?:[-+].*)?` and discarded it, so `1.4.192-rc.1` parsed as `[1, 4, 192]`, compared equal to the released `1.4.192`, and satisfied `>=1.4.192`. The plugin then ran against a runtime whose orchestration contract is still in flux — the exact case the version gate exists to reject. Parse the prerelease identifiers instead and compare them per semver §11: a release outranks any prerelease of the same triple, numeric identifiers compare numerically and rank below alphanumeric ones, and a longer identifier list wins when every preceding field is equal. Build metadata is still discarded, since it never affects precedence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013gGxGgKskzyzr1HRUB6cV3
…t of doctor `inspectOrcaPluginLifecycle` called `expectedOwnership()` unguarded, and that digests two real files. On a payload the caller cannot read — EACCES, a path that is no longer a regular file, a mid-update rename — the throw escaped the inspection and therefore escaped `genie doctor` entirely, killing the one command an operator runs to find out what is broken. Both doctor call sites (`checkDatabase` and `checkOrcaLifecycle`) inherited the failure. Catch it and return a new `unreadable` payload state carrying the underlying reason as recovery text. `checkOrcaLifecycle` already fails any payload that is not `owned-clean`, so doctor now renders `payload=unreadable` as a check line instead of aborting. Nothing here mutates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013gGxGgKskzyzr1HRUB6cV3
… source `bun scripts/orca-bundle-parity.ts --write` after the adapter and runtime fixes in this branch. `bun run lint:orca-bundle` is green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013gGxGgKskzyzr1HRUB6cV3
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ab62fc7fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| child.stdout.on('error', (error: Error) => { | ||
| settleTransportLoss(`stdout stream error: ${error.message}`); | ||
| }); | ||
| child.stderr.on('error', (error: Error) => { | ||
| settleTransportLoss(`stderr stream error: ${error.message}`); |
There was a problem hiding this comment.
Terminate the child after a stream fault
When either pipe emits error while Orca continues running, these handlers call settleTransportLoss(), whose finish() clears the process timeout without calling stop() or removing the data listeners. The adapter therefore reports completion while leaving an unbounded child process alive, potentially leaking Orca processes and continuing a mutation after the caller has handled the ambiguous failure; terminate the child through the existing bounded kill path before settling.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
* ci(commitlint): pin the four #2817 review-wave squash subjects Three squash-merge headers (#2828, #2829, #2837) crossed 100 characters only because GitHub appended the PR suffix, and #2830's subject is capitalised ("Orca-mode"). They are already on shared dev, so — matching every prior exception in this file — pin the exact full subjects instead of rewriting history. The rolling promotion #2817 fails Commit Messages on that range until this lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013gGxGgKskzyzr1HRUB6cV3 * ci(commitlint): also pin the #2840 squash subject Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013gGxGgKskzyzr1HRUB6cV3 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Six verified findings from the independent review of the dev→main promotion PR #2817, all in the Orca lane. Each was reproduced by execution against
origin/devbefore being fixed; the repro is what the new test asserts.C1 (CRITICAL) — status-only
task-updatenever survived its readbackProblem.
src/lib/orca-orchestration-adapter.ts—resultis optional ontask-update, but the readback demanded result equality:With
operation.resultomitted,JSON.stringify(operation.result)is the valueundefined, while the readback entity holdsnull(public projection, fresh task) or the prior JSON string. Neither comparison can hold, so every status-only update failedreadback_mismatchwithretrySafety: 'unsafe'— after the mutation had already committed.Fix. A new
taskResultMatcheshelper: when the mutation carried no result it asserts nothing about the stored one; when it did, the comparison is exactly as before.Tests.
accepts a status-only task-update readback whatever result the entity already storescovers the public projection withresult: null, the private projection with no result, and the private projection with a prior stored result.still rejects a task-update readback whose stored result contradicts the mutationkeeps the negative case honest.H4 (HIGH) — the child process ignored the operation's own wait
Problem.
worker-start,check --wait, andaskaccept a--timeout-msof up to 600 000 ms and legitimately block for it, butexecuteProcessalways passed the fixed 30 s adapter default as the process wall clock. Reproduced:askwithtimeoutMs: 120_000spawned withtimeoutMs: 30000. Any supported long wait was SIGTERM/SIGKILLed at 30 s and surfaced astimeout— orambiguous_after_possible_commitfor a mutation.Fix.
resolveProcessTimeoutMshonours the requested wait plus a 5 s grace (so the CLI can render its own timeout response instead of being killed mid-write), never shrinks below the adapter default, and clamps atMAX_ORCA_TIMEOUT_MS = 605_000— the schema maximum plus the grace, so no caller can unbound the child and a later schema widening cannot either.Tests.
bounds the child on the operation wait, not the fixed adapter defaultasserts the capturedrequest.timeoutMsforask/check/worker-start, for a sub-default wait (must not shrink), and for an operation with no wait.never lets a caller push the child bound past the adapter ceilingpins the clamp.H5 (HIGH) —
run-usethrew a rawTypeErroron an unboundrun-currentProblem.
run-currentlegitimately answers{ run: null }(the response schema ispublicRunEntity.nullable()), and the run-use readback didrecordOf(current.run).id. Reproduced:TypeError: null is not an object (evaluating 'run.id')escapingadapter.execute— outside the adapter's typed error contract, and thrown fromplan.matches(...), whichfinalizeMutationdoes not wrap.Fix. Two layers.
recordOfnow collapses any non-object to an empty record, killing the whole class across every readback plan (a field read yieldsundefinedand the caller's own comparison decides). The run-use plan additionally rejects an unbound answer explicitly, so it surfaces as the documentedreadback_mismatch/phase: 'readback'/retrySafety: 'unsafe'.Tests.
reports an unbound run-current readback as a typed run-use mismatchasserts anOrcaAdapterError(explicitly not aTypeError) with the full error shape, and that both calls were made.M6 (MEDIUM) —
bounds timeout termination through the kill escalation pathwas not version-robustProblem. The test expected SIGKILL and read SIGTERM, deterministically 5/5 on bun 1.3.14 (CI pins 1.3.11;
enginesallows>=1.3.10). Root cause is a startup race, not the adapter: the child installed its handler in JS (bun -e "process.on('SIGTERM', …)") and the 20 ms timeout fired before the interpreter finished booting, so the child died of the plain SIGTERM at ~26 ms and escalation never ran. The adapter's escalation path is correct — measured directly, it reaches SIGKILL at ~1258 ms once the child actually ignores SIGTERM.Fix. The child is now
/bin/sh -c 'trap "" TERM; exec sleep 30':shsets the disposition toSIG_IGNbefore anything else runs,SIG_IGNsurvivesexec, andexecleaves no grandchild holding the stdout pipe open. Timeout raised to 250 ms for margin over shell startup; the< 2500 msoverall bound is unchanged.Verification. 5/5 passes on the installed bun 1.3.14, both before and after the rebase.
L4 (LOW) — a stream fault crashed the process and left the promise pending
Problem.
child.stdout/child.stderrhad no'error'listener. Node/Bun re-throw an unhandled'error'event as an uncaught exception, so a stream fault killed the process andspawnOrcaProcessnever settled.Fix. Both streams settle through the same ambiguous-transport-loss path as the post-spawn child
'error'handler, now factored into onesettleTransportLoss(detail)used by all three call sites. The pre-spawnrejectpath is untouched.Tests. A real stream
'error'is not portably reproducible from a live child, socreateOrcaProcessExecutorgained aspawnChildseam (defaultnode:child_process.spawn, exposed only through__orcaAdapterTestOnly). Three tests drive it: the stdout and stderr faults, the post-spawn child transport error (regression lock on the refactor), and the pre-spawn error still rejecting rather than settling.L1 (LOW) — a prerelease satisfied the minimum runtime version
Problem.
plugins/genie/orca-runtime.tsmatched(?:[-+].*)?and discarded it, so1.4.192-rc.1parsed as[1, 4, 192]and satisfied>=1.4.192. The plugin would run against a runtime whose orchestration contract is still in flux — the exact case the gate exists to reject.Fix.
parseVersionkeeps the prerelease identifiers andcomparePrereleaseapplies semver §11: a release outranks any prerelease of the same triple, numeric identifiers compare numerically and rank below alphanumeric ones, and a longer identifier list wins on otherwise-equal fields. Build metadata is still discarded (it never affects precedence).Tests.
rejects a prerelease of the minimum runtime version and accepts real successors— accepts1.4.192,1.4.193,1.5.0,2.0.0,1.4.193-rc.1,1.4.192+build.7; rejects1.4.192-rc.1,1.4.192-0,1.4.192-alpha,1.4.191,1.3.999,0.9.9, and unparseable input.L3 (LOW) — an unreadable payload threw
genie doctorout entirelyProblem.
inspectOrcaPluginLifecyclecalledexpectedOwnership()unguarded, and that digests two real files. On a payload the caller cannot read (EACCES, a path that is no longer a regular file, a mid-update rename) the throw escaped the inspection and therefore escaped doctor — the one command an operator runs to find out what is broken. Both call sites (checkDatabaseat doctor.ts:268 andcheckOrcaLifecycle) inherited it.Fix. Catch it and return a new
unreadablepayload state carrying the underlying reason as recovery text.checkOrcaLifecyclealready fails anything that is notowned-clean, so doctor renderspayload=unreadableas a check line instead of aborting. Nothing in this path mutates.Tests.
reports an unreadable payload instead of throwing out of doctorreplaces the manifest with a directory (uid-independent, so it behaves the same for a root CI container and a normal user) and asserts both the inspection state and thatcheckOrcaLifecyclestill renders its line.reports an EACCES payload as unreadablecovers the literal mode-bit case, skipped under uid 0.Also in this branch
plugins/genie/orca-entrypoint.min.jsregenerated viabun scripts/orca-bundle-parity.ts --write, since the adapter and runtime are both bundled into it.bun run lint:orca-bundleis green.Validation
The full suite was not run locally (OOM on this box under concurrent load); CI runs it.
🤖 Generated with Claude Code
https://claude.ai/code/session_013gGxGgKskzyzr1HRUB6cV3