Repository navigation
Conversation
…blishes `session.tabs.listAll` (and the inventory census behind `session.tabs.subscribe`) withholds its answer until the renderer publishes the current graph, because a client without the authoritative-inventory capability would prune tabs from an unpublished list. On a host whose graph never publishes (orcad before its headless placeholder graph, or a stalled renderer) that wait had no bound, so a pre-capability client hung forever. The wait now gives up after 30s with `runtime_unavailable`, which every client already treats as retryable, and drops its waiter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe inventory publication wait now accepts an optional timeout override and uses a 30-second default. If the inventory is not authoritative before the timeout, the wait rejects with Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Inventory requests now have a bounded wait and return a retryable error when publication does not arrive. No merge-blocking issue was found. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Requests no longer wait indefinitely when the host has no published window graph. They receive an existing retryable error, and the review found no new route to unpublished tab data or broader authority. Surrounding security and deployment coverage remains incomplete. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — bounded wait for the session-tab inventory publication, plus the two tests that pin it.
SESSION_TABS_INVENTORY_PUBLICATION_WAIT_MS— new exported30_000constant insrc/main/runtime/orca-runtime-wait-for-session-tabs-inventory-publication.ts.- Timer in
waitForSessionTabsInventoryPublication— asetTimeout(added with a defaultabletimeoutMsparam,unref'd) races the existing publish/abort listeners through the sharedcleanup; the losing listeners are still removed exactly once, and a lateonPublishedafter a timeout is a harmless no-op since the promise already settled. - Error mapping — the thrown
new Error('runtime_unavailable')flows throughmapRuntimeError'sRUNTIME_PASSTHROUGH_CODES, the same pathcreateTerminalalready uses in this state, so no new wire shape or code is introduced. - Two fake-timer tests — one asserts the direct runtime call rejects
runtime_unavailableand leaves no waiter; the other drives the realRpcDispatcherwith no client capabilities and gets{ ok: false, error: { code: 'runtime_unavailable' } }.
I verified the coverage is real, not theatre: the file passes (19 tests) with the fix, and both new tests time out (30s) when I removed the timer while keeping the constant exported, so they cannot pass with the bug present. Call-path check confirms only the no-signal / never-published case times out — an aborted request still rejects client_disconnected, and a graph published within the bound still resolves normally.
Two things worth recording, neither blocking. The timeoutMs parameter is currently never overridden by any caller, so it is a seam kept for testability. And the bound is per wait call inside the while (true) retry loop rather than a whole-request budget, so epoch churn could in principle exceed 30s in aggregate — not reachable in the never-publishes scenario this PR targets.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
|
Hi @Jinwoo-H, gentle ping on this PR — all checks have passed and it's ready for merge. Whenever you have time, could you please take a look? Thank you! |

ELI5
When a client asks an Orca host for its list of tabs, the host waits until it has an up-to-date picture of its windows before it answers. On some headless hosts that picture never arrives, so the host kept the client waiting forever. Now it waits at most 30 seconds, then says "not available right now, try again" instead of never answering.
What Changed
orca serveafter its hidden renderer died, as in [Bug]: headless serve — renderer process gone leaves graph 'unavailable' forever; terminal create/close from paired clients silently fail with runtime_unavailable #18604),session.tabs.listAllnever answered: no error, no log. The inventory census behindsession.tabs.subscribehung the same way, so that stream never sent its first snapshot. Each stuck request also left a waiter registered on the host for as long as the connection lived.runtime_unavailableand its waiter is removed. That is the same error code the same host already returns forsession.tabs.createTerminalin this state. When the graph does get published within the 30 seconds, nothing changes.waitForSessionTabsInventoryPublicationnow starts a timer (SESSION_TABS_INVENTORY_PUBLICATION_WAIT_MS, 30s) next to its existing publish and abort listeners. Whichever fires first runs the shared cleanup, which clears the timer, drops the waiter and removes the abort listener. The timer isunref'd so it never keeps the process alive. Nothing else changes: the caller's retry loop, the authoritative labeling and the capability projection are untouched.Why
The wait exists for a good reason. A client could prune tabs from a list the renderer hasn't published yet, so the host holds the answer until that list is real. What was missing was an upper bound. Alternatives considered:
30s is deliberately generous compared with a normal publish, so a slow but healthy startup still answers normally.
Linked Issue
Related to #17846 and #18604. This PR doesn't fully resolve either one:
session.tabs.listAllnever answers" symptom, but orcad still cannot create a terminal until its graph is published. fix(orcad): publish the headless graph and start the agent hook server #17735 fixes that.Visual Proof
No visual change. This is host-side RPC timing: a request that used to hang now fails after 30s with an error clients already handle. Nothing is drawn differently.
Testing
Two new tests in
src/main/runtime/session-tabs-inventory-publication.test.ts, using fake timers:listAllMobileSessionTabsInventoryrejects withruntime_unavailableafter the wait, and no waiter is left behind.RpcDispatcherwith no client capabilities (the shape an older client sends),session.tabs.listAllanswers{ ok: false, error: { code: 'runtime_unavailable' } }.Both new tests fail without the fix. I reverted the production file and kept only the exported constant so the test still imports. Both then time out because the call never settles. With the fix restored, they pass.
Existing tests: the inventory publication file (19 tests) plus every test under
src/main/runtime/rpc/methodspass, 221 files and 2,220 tests.pnpm tc,check:code-quality:changed,audit:anti-slop, oxlint and oxfmt on the changed files all pass.Tested on macOS only. The change has no platform-specific code.
I manually tested these changes locally (covered by unit tests. I did not run orcad against a live client.)
Automated tests added/updated, or explained why not below
AI Disclosure
Claude Opus 5.5 (via Claude Code) wrote the fix and the tests, directed by a human.
Review
AI code review summary:
setTimeout/clearTimeoutare used, with no platform branches.unref?.()is optional-chained, so it still works where a timer has nounref.live/unverifiable/exitedverdicts fromdocs/reference/ssh-execution-boundary.md.runtime_unavailablehas been in the shared recoverable set (RECOVERABLE_CODESinsrc/shared/remote-runtime-client-error-classification.ts) since fix(remote-runtime): recover terminals after network loss #9774, and this host already returns it forcreateTerminalin this state. The error carries no tab list, so an old client cannot prune anything from it, which is the hazard the wait protects against. For example, the desktop's local inventory refresh throws on!response.okand applies no snapshots. When the desktop loads tabs from a paired remote host, it already stops waiting forlistAllafter its own 15s timeout, so its visible behavior doesn't change. What changes is that the host no longer holds a waiter for it forever. Callers with no client-side timeout, and the census behindsession.tabs.subscribe, now get an ending instead of an endless wait.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
The 30s bound is a judgment call. It is longer than the 15s
listAlltimeout the desktop uses for paired remote hosts, on purpose, because the host-side bound is meant to catch a graph that never gets published, not to race the client's own timeout. I'm happy to change the value if you'd prefer something else.Author
X: @mea_pri
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (typecheck and the related tests pass locally, along with the changed-code lint gates. I left fullpnpm lint, the full test suite andpnpm buildto CI.)🤖 Generated with Claude Code