Repository navigation
Conversation
…ly unreachable A worker reports worker_done once and ends its turn, so a few-minute app outage silently stranded finished work at dispatched. The CLI now retries worker_done on runtime_unavailable for about two minutes with backoff, reusing one request id so the host's mutation ledger replays rather than double-applies it, then prints the existing recovery command. The contract probe no longer caches a failed status.get, which otherwise made every retry fail without reaching the app. Part of STA-8833.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!unavailable || Date.now() + delayMs > deadline) { | ||
| throw orchestrationMutationRecoveryError(error) |
There was a problem hiding this comment.
If runtime metadata is missing or the status.get contract probe stays unreachable throughout the retry window, its error has no request ID. This code passes that error to recovery handling, which then prints neither a query command nor a retry command. The CLI has already minted an ID, but the worker cannot use it to recover the completion after giving up.
| it('reaches a runtime that comes back after the first attempt found it down', async () => { | ||
| const userDataPath = mkdtempSync(join(tmpdir(), 'orca-worker-done-retry-')) | ||
| const endpoint = join(userDataPath, 'runtime.sock') |
There was a problem hiding this comment.
This test runs on Windows but tries to listen on a Unix-style runtime.sock path and advertises a Unix transport. Existing Unix-socket transport tests skip Windows. Without a Windows guard or a named-pipe fixture, this test blocks the Windows test suite.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe CLI now retries selected orchestration mutations after Priority: ⚪ Not assessed Merge Risk: 🟡 Moderate · up to Retries improve worker completion during temporary outages, but prolonged probe failures still omit the promised recovery command, and the new socket test breaks Windows test execution. Address both before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Completion retries preserve their identity while running, but a later outage can erase the information needed for safe manual recovery after an uncertain completion. Existing access and duplicate-prevention controls limit the exposure; no new privilege expansion was established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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}
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: 9fabb749-3bf9-40a5-ad63-29692599cce8
📒 Files selected for processing (6)
src/cli/handlers/orchestration-migration.test.tssrc/cli/handlers/orchestration.test.tssrc/cli/handlers/orchestration/message-send-handler.tssrc/cli/handlers/orchestration/mutation-request.test.tssrc/cli/handlers/orchestration/mutation-request.tssrc/cli/runtime/client.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| }) | ||
| }) | ||
|
|
||
| it('reaches a runtime that comes back after the first attempt found it down', async () => { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,250p' src/cli/handlers/orchestration/mutation-request.test.ts
cat package.json
rg -n 'win32|exclude|include|pool' vitest.config* .github/workflowsRepository: stablyai/orca
Length of output: 40968
Skip this Unix-socket test on Windows or use a named pipe.
This test passes a filesystem path to server.listen and records it as a Unix transport. Windows requires a named-pipe endpoint for this IPC path, so the test can fail before it exercises retry recovery.
Minimal platform guard
- it('reaches a runtime that comes back after the first attempt found it down', async () => {
+ it.skipIf(process.platform === 'win32')('reaches a runtime that comes back after the first attempt found it down', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('reaches a runtime that comes back after the first attempt found it down', async () => { | |
| it.skipIf(process.platform === 'win32')('reaches a runtime that comes back after the first attempt found it down', async () => { |
| const unavailable = | ||
| error instanceof RuntimeClientError && error.code === 'runtime_unavailable' | ||
| if (!unavailable || Date.now() + delayMs > deadline) { | ||
| throw orchestrationMutationRecoveryError(error) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,110p' src/cli/handlers/orchestration/mutation-request.ts
sed -n '70,275p' src/cli/runtime/client.ts
sed -n '1,120p' src/cli/orchestration-mutation-recovery.tsRepository: stablyai/orca
Length of output: 15077
🏁 Script executed:
#!/bin/bash
set -o pipefail
base=a0abcb6818bfb2fcb1adf4bdecf097d81ea9c9be
head=4306ac428eab8387eca431c376d6d772783f3731
printf '%s\n' '--- diff stat ---'
git diff --stat "$base" "$head"
printf '%s\n' '--- focused diff: mutation helper ---'
git diff --unified=80 "$base" "$head" -- src/cli/handlers/orchestration/mutation-request.ts
printf '%s\n' '--- focused diff: runtime client ---'
git diff --unified=35 "$base" "$head" -- src/cli/runtime/client.ts
printf '%s\n' '--- focused diff: recovery helper ---'
git diff --unified=35 "$base" "$head" -- src/cli/orchestration-mutation-recovery.ts
printf '%s\n' '--- baseline focused source ---'
git show "$base:src/cli/handlers/orchestration/mutation-request.ts" | sed -n '1,120p'
git show "$base:src/cli/runtime/client.ts" | sed -n '70,190p'
printf '%s\n' '--- relevant tests and call sites ---'
rg -n --glob '*.{ts,tsx}' 'callOrchestrationMutation|ensureOrchestrationContractCompatible|orchestrationMutationRecoveryError|unavailableRetryMs|retry-request' src test tests 2>/dev/null | head -240Repository: stablyai/orca
Length of output: 41530
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- worker terminal handler ---'
cat -n src/cli/handlers/orchestration/worker-terminal-handlers.ts
printf '%s\n' '--- orchestration mutation call sites with argument context ---'
rg -n -C 8 --glob '*.ts' 'callOrchestrationMutation' src/cli/handlers/orchestration
printf '%s\n' '--- focused retry helper tests ---'
cat -n src/cli/handlers/orchestration/mutation-request.test.ts
printf '%s\n' '--- RuntimeClientError and transport error construction ---'
rg -n -C 8 --glob '*.ts' 'class RuntimeClientError|new RuntimeClientError|function sendRequest|const sendRequest|sendRequest<' src/cli src/sharedRepository: stablyai/orca
Length of output: 45661
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- retry-window references ---'
rg -n -C 12 --glob '*.ts' 'unavailableRetryMs|worker_done|workerDone|mutation_worker_done' src/cli src/shared
printf '%s\n' '--- exact changed orchestration handlers ---'
git diff --unified=25 a0abcb6818bfb2fcb1adf4bdecf097d81ea9c9be 4306ac428eab8387eca431c376d6d772783f3731 -- src/cli/handlers/orchestration/message-send-handler.ts src/cli/handlers/orchestration.test.ts src/cli/handlers/orchestration-migration.test.ts
printf '%s\n' '--- runtime-client files and error definition ---'
rg --files src | rg 'runtime-client|runtime/.*transport|request'
rg -n -C 12 --glob '*.ts' 'class RuntimeClientError|runtime_unavailable' src/cli/runtime-client.ts src/cli/runtime src/main/runtime src/shared 2>/dev/null
printf '%s\n' '--- focused new tests around recovery ---'
sed -n '1,240p' src/cli/handlers/orchestration/mutation-request.test.tsRepository: stablyai/orca
Length of output: 45627
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- message-send handler ---'
cat -n src/cli/handlers/orchestration/message-send-handler.ts | sed -n '90,135p'
printf '%s\n' '--- runtime-client error definitions and sendRequest ---'
rg -n --glob '*.ts' 'class RuntimeClientError|function sendRequest|sendRequest<' src/cli/runtime-client.ts src/cli/runtime
sed -n '1,180p' src/cli/runtime-client.ts
printf '%s\n' '--- client recovery attachment definitions ---'
rg -n -C 18 --glob '*.ts' 'attachDurableMutationRecovery|function attachDurableMutationRecovery|orchestrationRequestId' src/cli/runtime/client.ts src/cli/runtime
printf '%s\n' '--- recovery helper entry and unchanged behavior ---'
sed -n '1,95p' src/cli/orchestration-mutation-recovery.tsRepository: stablyai/orca
Length of output: 42141
Preserve recovery information when the compatibility probe fails.
When worker_done uses the retry window and the runtime remains unavailable, RuntimeClient.call runs ensureOrchestrationContractCompatible before it creates the request ID, recovery command, or recovery closure. The nested status.get call therefore returns an unavailable error without the outer request ID. orchestrationMutationRecoveryError leaves that error unchanged, so the exhausted command emits neither the stable ID nor a keyed retry command.
Establish the request ID and recovery command before the compatibility probe, and route probe failures through the same recovery path as mutation transport failures. Add a real-client exhaustion test for this probe-failure path. This branch has not sent worker_done, but it still loses the stable retry command needed to resume the one-shot worker completion safely.
![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 — worker_done now survives a brief Orca outage instead of being dropped after one failed send.
- Bounded retry for
worker_done(mutation-request.ts,message-send-handler.ts): onlyworker_doneopts into a 120s budget; the loop retriesruntime_unavailablewith 1s→15s backoff and reuses one request id (explicit--retry-requestor a CLI-minted UUID) so the host replays rather than re-applies. Other errors surface immediately as before. - Failed contract probe no longer memoized (
client.ts): a rejected probe clearsorchestrationContractCheck, so a retry actually reaches the app; a successful probe is still cached. Metadata is re-read per attempt, picking up a restarted app. - Tests: new
mutation-request.test.tscovers fake-timer retries, explicit-id preservation, non-retry of timeouts/non-opted-in verbs, budget exhaustion + recovery command, and a real Unix-socket round trip that fails without the probe-cache fix. Existingworker_donecall assertions updated for the new thirdcallarg.
I verified the retry is genuinely idempotent: the host's OrchestrationMutationExecutor keys inFlight by ${callerFingerprint}:${requestId} and joins an in-flight duplicate, backed by the persisted mutation-receipt ledger (including the effect-free worker_done checkpoint and operation_unknown handling after a crash). The recovery command does not double-emit --retry-request (recoverableOrchestrationArgs strips it first), and the probe-cache .catch cannot drop a concurrent success since only one probe exists at a time. All src/cli tests pass locally.
ℹ️ Retry is scoped to runtime_unavailable, not remote_runtime_unavailable
The predicate matches exactly runtime_unavailable. The CLI remote-pairing transport rewraps RemoteRuntimeClientError as RuntimeClientError with code remote_runtime_unavailable (websocket-transport.ts), so that path is not retried. The PR notes SSH workers run this CLI on the desktop and therefore hit the local code path, but it is worth confirming no worker_done can flow through remote pairing.
Technical details
# remote_runtime_unavailable is excluded from the retry
## Affected sites
- `src/cli/handlers/orchestration/mutation-request.ts` — predicate is `error.code === 'runtime_unavailable'`.
- `src/cli/runtime/websocket-transport.ts` — converts `RemoteRuntimeClientError` to `RuntimeClientError(error.code, ...)`, preserving `remote_runtime_unavailable`.
- `src/shared/remote-runtime-client-error-classification.ts` — `RECOVERABLE_CODES` already groups `runtime_unavailable` and `remote_runtime_unavailable` as recoverable connection errors.
## Open questions for the human
- Is a `worker_done` ever sent by a CLI that connects via remote pairing (rather than from the desktop/local transport)? If so, a brief desktop outage would still drop that report, since only `runtime_unavailable` is retried.
- If the gap is real, should the predicate reuse `isRecoverableRemoteRuntimeConnectionError` (while still excluding `runtime_timeout`), or is the local-only scope deliberate?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Superseded by #23984: same commit, moved to a branch name without the Linear id so merging it does not auto-close STA-8833 (this PR covers only part of that ticket). |
ELI5
When a worker agent finishes, it sends one "I'm done" message (
worker_done) and then stops. If the Orca app happened to be unreachable at that moment (a restart, an update, a short hang), that single message failed and the finished work stayed marked "in progress" forever. Now the CLI quietly keeps trying for about two minutes before giving up, and it re-sends under the same identity so Orca can never count the completion twice.What Changed
Before:
orca orchestration send --type worker_donemade one attempt. Onruntime_unavailable("Could not connect to the running Orca app", "closed the connection before responding", "runtime changed while the request was in flight", missing runtime metadata) it printed an error and exited. The worker had usually already ended its turn, so the Task stayeddispatchedand the coordinator'scheck --waitnever saw the report.After: for
worker_doneonly, the CLI retries onruntime_unavailablewith backoff (1 s, 2 s, 4 s, 8 s, then every 15 s) for up to 2 minutes. Every attempt carries the same orchestration request id: the one from--retry-requestif given, otherwise one the CLI mints before the first attempt. If the runtime is still unreachable after the budget, the CLI prints exactly the recovery output it prints today (read-only query command plus a retry command that reuses that request id).Mechanism:
callOrchestrationMutation(src/cli/handlers/orchestration/mutation-request.ts) takes an optional retry budget. When set, it owns the request id and loops onruntime_unavailableonly; any other error (timeouts, rejections, invalid responses) surfaces immediately as before. Onlyworker_donepasses a budget.RuntimeClientno longer caches a failed orchestration-contract probe. Before, the firststatus.getfailing during an outage was memoised as a rejected promise, so every later call in the same process failed instantly without contacting the app — retries would have been no-ops. A successful probe is still cached.No host change, no new wire field, no disk state.
Why
A worker's report is its only first-hand evidence of completion, and the report path is where an outage turns into lost work (#22156). Alternatives considered:
worker_doneis the one whose loss strands work, and its host-side dedupe is well exercised.runtime_timeouttoo: a timeout means the host may still be processing; the existing recovery flow (query first, then keyed retry) is the right answer there, so it is unchanged.The host already dedupes mutations by request id in its persisted mutation-receipt ledger (added with the orchestration primitives in #9925, 2026-07-27), so a retry after a lost response replays the original outcome instead of settling twice.
Linked Issue
Part of STA-8833 (Decision 8 of the dispatch-capability removal design).
Refs #22156 — brief outages now delay instead of drop a completion; outages longer than ~2 minutes still need the printed recovery command, so this does not fully resolve it.
Refs #12986 — answers the "queued or lost?" question in behavior; the orchestration guide is not updated here.
Visual Proof
N/A — CLI transport retry; no UI change.
Testing
New
src/cli/handlers/orchestration/mutation-request.test.ts:--retry-requestid across retries;runtime_timeout;statusmail);retryCommandcarries the same id;RuntimeClient: runtime down on the first attempt (no metadata, so the contract probe fails), then comes up and drops the firstsendmid-request, then answers — the call succeeds and bothsendattempts carried the same request id. This test fails without the probe-cache fix (verified by reverting it).Updated assertions in
orchestration.test.ts/orchestration-migration.test.ts:worker_donesends now carry a request id as the thirdcallargument.Commands (gate base a0abcb6):
./node_modules/.bin/vitest run --config config/vitest.config.ts src/cli src/main/ssh— all pass exceptssh-posix-command-wrapper.test.ts > survives the real /opt/homebrew/bin/fish parser, which fails identically without this change (local fish install; unrelated file).pnpm tc— pass.oxlinton changed files — clean.pnpm run check:code-quality:changed— pass.Not tested: a full end-to-end run against a real Orca app being restarted mid-report, and Windows named pipes (the transport code path is shared and unchanged; only the loop around it is new).
Review
Mixed versions:
orchestrationRequestId, and the CLI already sent a (randomly minted) id for every durable mutation; it now mints it one step earlier so retries share it. Nothing new on the wire.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
Cross-platform: pure Node timers around the existing transport (Unix socket / named pipe). Mobile: unaffected (CLI only). Performance: at most ~12 extra connection attempts over 2 minutes, only for a failing
worker_done.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)