Repository navigation
fix(agent-status): lift the closed-tab blackhole when a retracted tab comes back (STA-5679) - #16985
brennanb2025 wants to merge 2 commits into
Conversation
|
Warning Review limit reachedNext included review available in 7 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 4bb3377 and 573eea774fefbb665685e000375dc2c1ec3f19ef. 📒 Files selected for processing (7)
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 |
Retracting this — the fix is a no-op, and making it work would regress STA-4114A history audit found this PR is wrong in a way my own repro hid. Posting the reasoning rather than silently force-pushing. 1. The lift can never fire. Main's 2. The ids I pass can't match anything main holds. 3. If it were "fixed" to actually fire, it would regress STA-4114. #14624 lifted the pane fence and explicitly refused the tab one: "Closed-tab tombstones are a separate, stronger claim and are deliberately left standing… a live process is never routed back into a closed tab." Main's set also gates 4. My tests were vacuous against production. Both used On my repro: the blackhole I demonstrated was real, but I induced it by calling What survives as a genuine gap, for a future PR: nothing pins STA-4114's guarantee against a lift — adding The Claude/Cursor status outage this was chasing is explained and fixed by #16980 (a green test run was deleting the developer's real |
… comes back (STA-5679) `getAgentStatusDisposition` suppresses every hook event whose tab id sits in `closedAgentStatusTabIds`, and that check runs FIRST — before the pane-key gate and before any new-turn revive. Unlike `closedAgentStatusPaneKeys`, which a `UserPromptSubmit`/`SessionStart` lifts, the tab set was add-only: nothing but a 1024-entry LRU eviction or a full server reset ever cleared it. A tab retracted by a transient or subset snapshot frame — a host restart, a cross-host collision replay — is therefore blackholed for the rest of the session even though it is still open and its agent is still working. The POSTs keep returning 204, so nothing anywhere reports a problem. The renderer already knew: `buildRemirroredClosedTabMarkerLiftPatch` lifts its own marker when a mirrored id is republished, commented "a lingering closed-tab marker would blackhole its byte-derived status for the whole session". But main's set is checked upstream of the renderer, so lifting only the renderer half could never reach it. Give main the other half of that lift: `liftClosedAgentStatusTabs()` behind an `agentStatus:liftClosedTabs` IPC, called with exactly the ids the renderer just proved alive. This does not weaken the `re-attach does not lift a closed-tab tombstone` rule — a re-attach is an inference, whereas a republished mirrored id is the host reporting the tab exists. Reproduced before the fix on a live pane: UserPromptSubmit, SessionStart and PreToolUse all returned 204 with no row, while the tab stayed open.
…igger The lift's only production trigger — the renderer re-mirror call — and the IPC handler that carries it had no test: reverting either left every suite green. The server-side method was the sole covered hunk. Also adds liftClosedAgentStatusTabs to the agentHookServer double; the handler swallows its own errors, so the omitted field would have passed vacuously.
573eea7 to
22685e4
Compare
Review r1 — rebased, validated, and given the coverage its wiring was missingHead is now What the bug actually is
Rendered proof (real hook events through the live main process)Posted genuine
Phase 2 is the important frame: the row does not vanish, it goes stale, which is exactly the "no loading status" symptom users report. The gap I found and fixedAblating each production hunk individually: only the server method was covered. Reverting the IPC handler, or the renderer re-mirror call that is the fix's only production trigger, left every suite green. The two new server tests exercise a method called directly on the object — they would still pass if nothing ever called it. Added in
Mutation-tested the original two tests: making the method a no-op kills one, making it clear the whole set kills the other. They are non-redundant. Pre-fix both fail with Scope — worth stating plainlyThe lift fires only for worktrees bound to an explicit runtime environment: every snapshot path in Follow-up worth filing (not blocking)That same retraction sweep tears down five things under a documented "retired tab ids never recur" assumption. On the mirror path that assumption is false, and this PR restores only one of the five — the parked-terminal watcher, pane-foreground-agent map, hibernation output epoch, and agent-startup delivery guards stay torn down for a tab that comes back. Gates
Not verified: dev-mode CDP is blind to packaged-build differences, so the rendered evidence above does not cover a packaged build. I also did not exercise a real paired remote host end to end — the phase-2/3 transition was driven through the real IPC and hook path, but the natural trigger is the snapshot re-mirror. Not approving or merging — that is Brennan's call. |
Rendered QA (adversarial) — #16985 @
|
| step | subject tab | control tab |
|---|---|---|
| A baseline, neither closed | working / "baseline-subject" |
working / "baseline-control" |
B both retracted (dropByTabPrefix) |
NULL — blackholed | NULL — blackholed |
C real applyFreshWebSessionTabsSnapshot re-mirrors only the subject |
renderer marker cleared, liftClosedTabs fired |
(not re-mirrored) |
| D POST a hook to both again | working / "AFTER-REMIRROR-subject" ✅ |
NULL — still suppressed ✅ |
Step A matters: without a green baseline the whole differential would be vacuous. It's green, so B is a genuine reproduction of STA-5679 and D is a genuine behavioural change.
The chain exercised end to end is the entire PR: web-session-tabs-sync.ts → preload liftClosedTabs → agentStatus:liftClosedTabs ipcMain handler → agentHookServer.liftClosedAgentStatusTabs → getAgentStatusDisposition gate → hook ingest. Step C called the real production entry point the paired path uses (applyFreshWebSessionTabsSnapshot), not the IPC directly, so the renderer-side trigger is covered too.
The control arm is the important half: the tab that was never re-mirrored stays suppressed. The lift is scoped to the named tab and is not a global reset of the closed-tab set.
I also confirmed the preload surface is live in the running app — liftClosedTabs is present on window.api.agentStatus.
Scope, stated plainly
The only production caller of applyWebSessionTabsSnapshot is web-runtime-session.ts:1145, keyed by a runtime environmentId. So this fixes the paired/remote variant and nothing else — consistent with what the earlier review concluded. My step B→D control arm is effectively the plain-local shape: a tab retracted with no snapshot re-mirror stays blackholed for the whole session. A green on the local path would have proved nothing about this PR, and I did not claim one. That variant remains #16980's job.
What I could not cover
- A genuine remote host. There was no real paired Orca server in this rig; the snapshot in step C was synthesized to the shape the host publishes and fed to the real production function. The renderer→main→hook-server half is fully real; the host→renderer wire hop is not exercised here.
- Packaged builds — dev-mode CDP is structurally blind to them. No packaged coverage claimed.
- Retraction via a real transient/subset host frame. I retracted with
dropByTabPrefix, which is the same main-side state the retraction path produces, but not the frame-driven route that creates it in the wild.
No production code was changed and nothing was merged, approved, or closed.
ClosingRetracted above with the full reasoning: this lift can never fire (main's The real fix for the status problems this was chasing is #17619. Closing so nobody picks this up as merge-ready. |
ELI5
When you close a terminal tab, Orca stops accepting agent-status updates for it. Sensible — except
Orca could believe a tab was closed when it wasn't. A host restart, or a refresh that only carries
part of the picture, can briefly retract a tab that is still sitting there with an agent working in
it. Once that happened, the tab was on the "closed" list for the rest of the session with no way
off it. Every status update for it was accepted and thrown away, silently, so the tab showed no agent
status at all and nothing anywhere reported a problem. Restarting Orca was the only cure.
Even starting a brand-new turn in that tab — the strongest possible "I am alive" signal there is —
did not bring it back.
User-facing before / after
When it bites
Anyone whose Orca host restarts, or replays a partial snapshot, while agents are working. The symptom
is a tab whose agent status stops updating and never recovers until you restart the app.
The bug
getAgentStatusDispositionsuppresses every hook event whose tab id is inclosedAgentStatusTabIds(server.ts:1185), and that check runs first — ahead of the pane-key gate and ahead of any new-turn revive.Unlike
closedAgentStatusPaneKeys, which aUserPromptSubmit/SessionStartlifts a few lines below, the tab set is add-only. Only a 1024-entry LRU eviction or a full server reset ever clears it.So a tab retracted by a transient or subset snapshot frame — a host restart, a cross-host collision replay — is blackholed for the rest of the session even though it is still open and its agent is still working. The POSTs keep returning
204, so nothing reports a problem anywhere.Reproduced on a live pane
Real hook events over the real HTTP path, against a tab confirmed still present in
tabsByWorktree:claude / workingUserPromptSubmit204, no rowSessionStart204, no rowPreToolUse204, no rowUserPromptSubmitandSessionStartare the strongest revive signals in the system. The tab-level set ignores all of them.The fix
The renderer already recognized this hazard and fixed its half —
buildRemirroredClosedTabMarkerLiftPatch, commented:But main's set is checked upstream of the renderer, so lifting only the renderer marker could never reach it. This adds the other half:
liftClosedAgentStatusTabs()behind anagentStatus:liftClosedTabsIPC, called with exactly the ids the renderer just proved alive.This does not weaken the existing
re-attach does not lift a closed-tab tombstonerule — a re-attach is an inference about a pane, whereas a republished mirrored id is the host reporting that the tab exists.Tests
Two in
server-closed-tab-suppression.test.ts, verified non-vacuous: stubbing the lift to a no-op fails withexpected "vi.fn()" to be called at least once. The first test also asserts the suppressed state before lifting, so it cannot pass by accident. Second test pins that only the named tabs lift. 22/22 pass;tc:node,tc:web, oxlint and oxfmt clean.Scope
Deliberately narrow. Not touched: the 1024 LRU cap, the launch-token fence, the pane-key revive rules, or the retraction logic that decides a tab is gone in the first place. Whether
web-session-tabs-syncshould retract a live tab is a separate question — this makes the consequence recoverable rather than permanent.