Repository navigation
Conversation
…issing A terminal tab present in the live mobile-session snapshot but absent from the persisted workspace session could never be closed. Every close path routes through closeHeadlessMobileTerminalTab, whose de-persist step threw tab_not_found before the PTY kill and the snapshot prune, so the runtime republished the unchanged snapshot and the tab resurrected on every client. The drift arises from never-persisted background tabs (failed reveal) and from a prior close that de-persisted the row and then failed on a later step. The caller resolves the tab from the live snapshot before this code runs, so a missing persisted row always means drift, never a bad tab id. Treat it as already de-persisted: continue with the PTY kill and the snapshot prune. This also makes the interrupted-close case self-healing — the next close completes the transaction. Fixes stablyai#10747 Fixes stablyai#12920 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe persisted headless terminal removal path now returns an empty result when the tab is missing. The headless terminal close flow no longer accepts or forwards 🚥 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 |
|
@coderabbitai review |
|
|
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. |
|
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. |
|
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. |
|
@OrcaWin A fresh Orca 1.4.184 reproduction today shows that this now also affects supervised worker cleanup: both PTYs were killed, but both close operations returned This PR is currently mergeable and CodeRabbit has no actionable findings. The exact-head Could you please approve the workflow run and review the current head? This is blocking reliable terminal and supervised-worker cleanup on Orca 1.4.184. |
|
Confirming this on v1.4.184 in headless SymptomTerminal tabs could not be closed at all. Evidence 1 — the throw, exactly as this PR describesFrom the host's Span The ordering consequence this PR names is what produced the orphaned processes: the throw precedes the PTY kill, so a failed close loses both the record and the process. 53 live shells, zero of them reachable from the UI. Evidence 2 — why the persisted row is missing:
|
| Where | Tabs |
|---|---|
workspaceSession (the local partition) |
81 (51 + 30 across two repos) |
workspaceSessionsByHostId["runtime:<envId>"] |
0 |
repos[].executionHostId (both repos) |
runtime:<envId> |
folderWorkspaces was empty, and 103 of the 111 failing closes used the plain repo-scoped worktree key (identical to the stored key) — so this is not a worktree-key mismatch and not the folder-scope path. tryGetWorkspaceSessionHostIdForWorktree takes its non-folder branch, returns getRepoExecutionHostId(repo) = runtime:<envId>, and getWorkspaceSessionForWorktree reads that one partition, which is empty. Meanwhile getWorkspaceSessionHydrationTargets sweeps every partition, finds the 81 rows in local, and republishes them. Read scans everything; delete targets exactly one partition.
Evidence 3 — the drift regenerates (this is the part that matters for this PR)
We purged all persisted state (81 tab records and 81 terminal-history dirs removed, service restarted), then opened one fresh terminal and closed it:
13:28:23 session-created serve-a3b556c3-...
13:28:33 runtime.session-tabs.close -> Failure: tab_not_found
13:28:52 bash pid 377643 still alive
Store afterwards: local partition = 2 tabs, runtime:<envId> partition = 0 tabs, executionHostId unchanged. So the tab-create path writes to local on a freshly purged store while the delete path resolves to runtime:<envId>. This is not legacy data — it reproduces on every new terminal.
Consequence for this PR
This change is necessary and we are shipping it locally: it guarantees a close can no longer leave an orphaned PTY, which is the worst symptom. But on its own it does not end the resurrection in serve mode — closeTerminalTabInWorkspaceSession still returns closed:false against the empty partition, so the row in local is never deleted and the hydrate sweep can re-create the tab on the next reconnect. The tab dies for the session and comes back on reattach.
The remaining half looks like it belongs in tryGetWorkspaceSessionHostIdForWorktree, which is the single resolver behind both getWorkspaceSessionForWorktree and setWorkspaceSessionForWorktree — so a fallback placed there keeps read and write on the same partition by construction. (Fixing only the getter would write the mutated local session into the runtime: partition, which is worse than the current bug.) The rule we are using locally, in the read-side-only spirit of #12722: populated candidate always wins; otherwise, if exactly one partition holds tabs for that worktree, resolve to it; under zero or 2+ matches, keep today's behaviour rather than guess.
Worth noting that in headless serve mode the runtime is the local machine, so the writer choosing local is arguably correct and the deleter consulting getRepoExecutionHostId is the actual defect — which is why we did not touch the writer.
Related: #9585, #11803, #12721, #14719 all appear to be facets of the same read/write partition asymmetry.
Happy to supply the raw trace spans or the anonymised orca-data.json if that would help.
Summary
A terminal tab that exists in the live mobile-session snapshot but has no persisted workspace-session record can never be closed:
terminal close/terminal closeTabfail withtab_not_found(CLIterminal closecan never close a background tab that failed reveal — headless close path throwstab_not_foundfor tabs that were never persisted #10747).Every close path funnels into
closeHeadlessMobileTerminalTab, which runs in this order:removePersistedHeadlessTerminalTab— throwstab_not_foundwhen the persisted row is missing.The drift has two observed origins: a background tab that failed reveal and was never persisted (#10747), and a prior close that removed the persisted row and then failed on a later step (daemon logs show
session-kill-failed), leaving the live snapshot holding a tab whose persisted row is already gone (#12920).Fix: by the time
closeHeadlessMobileTerminalTabruns,closeMobileSessionTabhas already resolved the tab from the live snapshot — the tab provably exists. A missing persisted row at that point can only mean drift, and refusing the close can never repair it. So treat a missing row as "already de-persisted" and continue with the PTY kill and snapshot prune. This is exactly what the existingallowMissingPersistedTabescape hatch already did for the relay-recovery caller; this PR makes that behavior unconditional and removes the option. It also makes the interrupted-close origin self-healing: the next close attempt completes the transaction instead of failing forever. Theterminal_tab_pinnedandworkspace_session_unavailableguards are unchanged.Screenshots
No visual change.
Testing
pnpm lintpnpm typecheckpnpm test— 46,990 passed, 11 failed. The 11 failures reproduce identically on cleanmain(f713739) in the same environment (Linux, Node 22, checkout on /tmp): they are host-environment-sensitive suites (shell prompt-hook builders, GPU flag config, git filesystem-boundary discovery), none of which import the changed module.pnpm buildNew regression test in
orca-runtime.test.ts: warms the live snapshot vialistMobileSessionTabs, drops the persisted row out-of-band, then closes. It fails withtab_not_foundbefore the fix (verified by stashing the fix and re-running); with the fix it verifies both PTYs are killed and a re-list shows no resurrected tab.AI Review Report
Reviewed with Claude Code (Claude Fable 5). Main risks checked:
removePersistedHeadlessTerminalTabhas exactly one caller (closeHeadlessMobileTerminalTab), and all of its call sites are insidecloseMobileSessionTab, which resolves the tab from the live snapshot before any of them run. A repo-wide search fortab_not_foundconsumers found one functional matcher (remote-runtime-pty-transport.tsisMissingHostSessionSurfaceError), which guards thesession.tabs.activatepath — activation still throwstab_not_foundfor tabs absent from the snapshot (closeMobileSessionTabline ~7749 is unchanged); only the close-side de-persist step stops throwing.live-host-ptyecho protection,retirement-ownerboundary, unattributed-close intent check) all run before the modified code; this change only affects post-authorization cleanup.projectedPtyIdsis empty; PTYs to kill are collected from the live snapshot with the existing other-owner exclusion, so no PTY owned by a surviving tab can be killed.terminal_tab_pinnedstill refuses when the persisted row exists and is pinned. A pinned tab whose persisted row is gone becomes closable — that state is drift, and the pre-fix alternative was an unclosable immortal tab.snapshotVersionand republishes, which is what paired clients key on.Cross-platform: explicitly reviewed for macOS, Linux, and Windows impact. The change is pure control flow in the Electron main process — no paths, shell invocations, shortcuts, labels, or platform-conditional branches are touched. PTY kills go through the existing
ptyControllerabstraction, which already encapsulates platform differences. No Electron API surface is added or changed.Security Audit
parseAppSshPtyIdfilter) is preserved; the missing-row case contributes no persisted ids at all, so kill targets come only from the live snapshot with the other-owner exclusion.terminal listuntil restart (noted in CLIterminal closecan never close a background tab that failed reveal — headless close path throwstab_not_foundfor tabs that were never persisted #10747).Notes
closed: trueinstead of erroring withtab_not_found. Clients that surfaced that error to users will simply stop seeing it; no client relied on it programmatically (see AI Review Report).Fixes #10747
Fixes #12920
🤖 Generated with Claude Code