Repository navigation
fix(session): remember closed and initialized runtime tabs - #21630
Conversation
Host persist snapshots wrote tabs but omitted this write-once map, and full session replaces / hydration treated omission as "never applied". Union the marker across persist and hydrate so default terminals are not re-spawned on every launch or re-attach. Fixes stablyai#18117
Focusing a workspace owned by a remote runtime created a terminal every time, and sometimes two. The mirror could never record the closed-last-terminal state. A host snapshot with no terminals produced `nextTerminalTabs === null`, which `withWorktreeEntry` turns into a deleted key -- and a missing row is exactly how every seeder spells "never initialized" (initial-terminal.ts). Keep an explicit empty row instead, so the remote path reads the same tombstone the local one already honours. A worktree that never had a terminal still gets no row, because `sameTerminalTabs` treats a missing row and an empty one as equal; removal frames and synthesized unpublished frames keep deleting, since neither is evidence the user emptied anything. The duplicate had a second cause. `requestedInitialTerminal` was a `let` inside the session-tabs subscription closure, so "one focus creates at most one terminal" held only for as long as that closure lived. Its effect re-runs whenever the environment, connection generation, pairing revision, or session-ready flag settles -- all of which move during a workspace switch -- so a second closure re-armed the flag while the first create was still in flight. That is the asymmetry in the report: one terminal when arriving from the landing screen, two when arriving from another workspace. Latch the bootstrap per worktree in a module-scoped set instead, modelled on web-runtime-wake-terminal-respawn.ts, released when the create settles. The closure flag stays alongside it so a failed create still does not retry on every later frame of the same subscription. Fixes STA-6173.
Follow-up on the STA-6173 fix, addressing restore-time safety gaps found in review. - Decline the bootstrap on a synthesized unpublished frame (`UNPUBLISHED_WORKTREE_PUBLICATION_EPOCH` at version 0). That frame is the runtime saying "ask me later", not a host with zero terminals; seeding on it can duplicate a pane the host is about to republish after a restart. This is the same "ask me later" frame the tombstone write already refuses to treat as the user emptying the workspace. - Release the module-scoped bootstrap latch on worktree tracking teardown and environment teardown, mirroring web-runtime-wake-terminal-respawn. A create RPC that never settles during a disconnect would otherwise leave the per-worktree key set and suppress the next bootstrap after reconnect. Tests: - New per-worktree and per-environment latch-release cases. - New "unpublished frame declines" bootstrap case. - New hook-level regression pinning the second defect end to end: a forced active-subscription re-run while the first create is in flight seeds exactly one terminal (two on the pre-fix tree).
Addresses review on the STA-6173 hardening. - Key the bootstrap latch by (environment, worktree), not worktree alone. A worktree id is `repoId::path` with no host component, so the same id can be live on two paired runtimes at once. The latch was cleared wholesale on any environment teardown, so tearing down environment A released environment B's in-flight key and a fresh B subscription could seed a duplicate — the STA-6173 defect through another door. Environment teardown now clears only its own keys; worktree teardown clears only that (environment, worktree). - Hold the latch after a successful create until a mirrored `tabsByWorktree` row exists. The snapshot refresh the create awaits can resolve on an empty, unconfirmed frame that leaves no row; releasing then let a later effect re-run seed a second terminal. A failed create still releases for retry. The latch claim/create/release now lives in web-runtime-initial-terminal-bootstrap-dispatch.ts, keeping active-session-subscription within its line budget. Tests (each mutation-tested against its own regression): - Cross-environment: a create for env B in flight, env A torn down, a fresh B closure must still decline. Fails when env teardown sweeps all environments. - Hold-until-row: a create that resolved without mirroring a row must not let an effect re-run seed again. Fails on unconditional release.
createWebRuntimeSessionTerminal never throws: the operation catches RPC and
network failures and returns `{ status: 'failed' }`. The dispatch helper
released the latch only from `catch` (dead for that path) or once a
tabsByWorktree row existed (false after a failure), so a failed create left
the latch held and suppressed every later auto-seed for that environment's
worktree until teardown -- the opposite of its own doc comment.
Capture the outcome and release on a returned failure as well, so the next
focus can retry.
Regression: a create that resolves `{ status: 'failed' }` followed by an
effect re-run must create again. Fails on the previous release condition.
The previous commit released the latch on a returned failure, but a create that *succeeded* with no mirrored row yet had no release at all: the row-conditional check was the only exit for the success path, so a host that accepted the tab while the mirror never got a frame held the latch until environment teardown and suppressed every later auto-seed for the worktree. Give the latch two phases. `creating` blocks other closures while the RPC is in flight. A success with no row is parked as `awaiting-mirror` instead of held, and the next frame the mirror accepts for that worktree releases it -- that frame is the mirror's answer either way (a row now exists and the predicate declines on its own, or the host genuinely has no terminal and a retry is right). A create still in flight keeps its claim: releasing it on a frame would reopen the re-armed-closure race the latch exists to close. Also correct the closure-flag comment: `requestedInitialTerminal` is set only after the dispatch resolves, so a thrown create never sets it and a later frame may retry. The flag records that this subscription already owned a create; it never described a failed one. Regressions, each mutation-tested against its own term: - success with no row, then the mirror's empty answer, then a fresh closure -> must create again (fails when success-with-no-row is held instead of parked, and when the subscription does not call the frame release) - a mirror frame releases an awaiting-mirror claim but never a pending create (fails when the release ignores the phase)
The returned-failure release freed the module latch but the caller still
latched its closure-local requestedInitialTerminal whenever the dispatch
reported it owned the create, including a create that returned
{ status: 'failed' }. A thrown failure never set it and retried on the
next frame; since every RPC and network failure is reported as a return,
the live path was the one that suppressed the whole subscription. The
dispatch now reports false for a failure whichever way it arrives.
ensureWebRuntimeWorktreeTerminalAfterWake read tabsByWorktree through `?? []`, and the only guard that could have seen presence was &&-gated on the value that erased it, so it could never fire for a tombstone. It runs on every activateAndRevealWorktree, not only after a wake, and a tombstoned workspace routes exclusively here: the stream-frame path returns at localTerminalCount === 0 before reaching its own guard. So the workspace was re-seeded on every focus regardless of the mirror fix. Two states shared one line and now do not. With no rows the workspace is being seeded for the first time and the decision goes to shouldAutoCreateInitialTerminal with presence read by Object.hasOwn; with rows present the question is whether a woke workspace's chrome outlived its PTYs, which the tombstone says nothing about. Both arms are pinned, because reverting only the respawn arm broke no existing test.
canonicalizeTerminalSessionWorktreeId read the source row through `?? []` and always wrote the target one, so a workspace with NO tabsByWorktree row came out the other side with an explicit empty one. That is the closed-last-terminal tombstone, and this PR's new activation-path reader honours it, so the re-keyed workspace never gets its initial terminal. Guarded on the source row's presence, which is the guard the sibling keyed maps six lines below already use. Mutating the guard to test emptiness instead of presence breaks the assertion that a real tombstone still survives the re-key, which is the distinction that matters.
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe PR preserves Priority: ⬆️ High Severity of issue fixed: High 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 775b7099-8d2d-4854-8867-5087f9b5d069
📒 Files selected for processing (23)
src/main/persistence-host-admitted-terminal-membership.test.tssrc/main/persistence/loading-store/pty-binding-persistence.tssrc/main/persistence/runtime-authored-workspace-session-fields.test.tssrc/main/persistence/runtime-authored-workspace-session-fields.tssrc/main/runtime/workspace-session-worktree-id.test.tssrc/main/runtime/workspace-session-worktree-id.tssrc/renderer/src/lib/default-terminal-tabs-applied-persist.test.tssrc/renderer/src/lib/session-write-subscriber.test.tssrc/renderer/src/lib/web-runtime-worktree-terminal-after-wake.tssrc/renderer/src/lib/worktree-activation-empty-remote.test.tssrc/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.tssrc/renderer/src/runtime/web-runtime-initial-terminal-bootstrap.tssrc/renderer/src/runtime/web-session-tabs-sync-emptied-workspace-tombstone.test.tssrc/renderer/src/runtime/web-session-tabs-sync-initial-terminal-relaunch.test.tsxsrc/renderer/src/runtime/web-session-tabs-sync-terminal-bootstrap.test.tssrc/renderer/src/runtime/web-session-tabs-sync-test-harness.tssrc/renderer/src/runtime/web-session-tabs-sync/active-session-subscription.tssrc/renderer/src/runtime/web-session-tabs-sync/apply-preparation-base.tssrc/renderer/src/runtime/web-session-tabs-sync/tracking-decisions.tssrc/renderer/src/runtime/web-session-tabs-sync/tracking-lifecycle.tssrc/renderer/src/runtime/web-session-tabs-sync/tracking.tssrc/renderer/src/store/terminals/workspace-terminal-hydration-patch.tssrc/renderer/src/store/terminals/workspace-terminal-hydration.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
| session.defaultTerminalTabsAppliedByWorktreeId = { | ||
| ...session.defaultTerminalTabsAppliedByWorktreeId, | ||
| [bindingWorktreeId]: true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 15 '\bevaluatePtyBindingFastLane\s*\(' src/main/persistenceRepository: stablyai/orca
Length of output: 14046
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fast-lane implementation ---'
cat -n src/main/persistence/loading-store/pty-binding-fast-lane.ts
printf '%s\n' '--- persistence marker references ---'
rg -n -C 12 'defaultTerminalTabsAppliedByWorktreeId|applyPtyBinding|persistPtyBinding|writePtyBinding' src/main/persistence/loading-store/pty-binding-persistence.ts src/main/persistence/loading-store src/main | head -n 320Repository: stablyai/orca
Length of output: 35737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 'defaultTerminalTabsAppliedByWorktreeId|default terminal|defaultTerminal' src shared tests 2>/dev/null | head -n 360Repository: stablyai/orca
Length of output: 31497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 18 'defaultTerminalTabsAppliedByWorktreeId|defaultTerminalTabsAppliedByWorktreePath' src
printf '%s\n' '--- default terminal application symbols ---'
rg -n -C 12 'apply.*default|default.*terminal|createDefault.*Terminal|defaultTerminalTabs' src/main src/renderer src/sharedRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 --glob '!**/*.test.*' --glob '!**/i18n/**' --glob '!**/locales/**' 'defaultTerminalTabsAppliedByWorktreeId' src
printf '%s\n' '--- likely activation guards ---'
rg -n -C 8 --glob '!**/*.test.*' --glob '!**/i18n/**' --glob '!**/locales/**' 'defaultTabs|defaultTerminal|initialTerminal|applyDefault' src/renderer/src/lib src/renderer/src/hooks src/renderer/src/components src/mainRepository: stablyai/orca
Length of output: 50370
Ensure the fast lane does not bypass the marker write.
evaluatePtyBindingFastLane does not check defaultTerminalTabsAppliedByWorktreeId. A durable binding with matching state can take the fast lane while its marker is absent. persistPtyBinding then returns before applyPtyBinding writes the marker, so a later activation can apply the default terminals again.
Make a missing marker force the durable write path.
| if (Object.hasOwn(session.tabsByWorktree, sourceWorktreeId)) { | ||
| const tabs = session.tabsByWorktree[sourceWorktreeId] ?? [] | ||
| delete session.tabsByWorktree[sourceWorktreeId] | ||
| session.tabsByWorktree[targetWorktreeId] = tabs.map((tab) => ({ | ||
| ...tab, | ||
| worktreeId: targetWorktreeId | ||
| })) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,140p' src/main/runtime/workspace-session-worktree-id.ts
rg -n 'canonicalizeTerminalSessionWorktreeId|defaultTerminalTabsAppliedByWorktreeId' src/main/runtime src/main/persistence src/renderer/src | head -200Repository: stablyai/orca
Length of output: 15008
🏁 Script executed:
set -eu
printf '%s\n' '--- runtime caller ---'
cat -n src/main/runtime/runtime-terminal-orphan-session-adoption.ts
printf '%s\n' '--- canonicalization tests ---'
cat -n src/main/runtime/workspace-session-worktree-id.test.ts
printf '%s\n' '--- identity migration ---'
sed -n '150,245p' src/main/persistence/tracking-repos/worktree-identity-migration.ts
printf '%s\n' '--- identity migration coverage tests ---'
sed -n '120,210p' src/main/persistence/tracking-repos/worktree-identity-migration-field-coverage.test.ts
printf '%s\n' '--- migration callers ---'
rg -n -C 5 'canonicalizeTerminalSessionWorktreeId|worktree-identity-migration|moveSessionKey' src/main src/renderer/src | head -260
printf '%s\n' '--- default terminal application ---'
cat -n src/renderer/src/lib/worktree-default-terminal-tabs.ts
printf '%s\n' '--- marker writes and relevant activation flow ---'
rg -n -C 8 'markDefaultTerminalTabsApplied|applyDefaultTerminalTabs|defaultTerminalTabsAppliedByWorktreeId' src/renderer/src/lib src/renderer/src/store | head -320Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
set -eu
printf '%s\n' '--- moveSessionKey definition ---'
sed -n '95,150p' src/main/persistence/tracking-repos/worktree-identity-migration.ts
printf '%s\n' '--- migration test references to default marker and collisions ---'
rg -n -C 10 'defaultTerminalTabsAppliedByWorktreeId|target|collision|existing' src/main/persistence/tracking-repos/*worktree-identity-migration*.test.ts
printf '%s\n' '--- relevant workspace session state defaults/types ---'
rg -n -C 5 'defaultTerminalTabsAppliedByWorktreeId' src/shared src/main/runtime src/main/persistence/tracking-repos/worktree-identity-migration.tsRepository: stablyai/orca
Length of output: 19933
Re-key the default-terminal marker with the terminal row.
This runtime migration moves tabsByWorktree[sourceWorktreeId] but does not move defaultTerminalTabsAppliedByWorktreeId[sourceWorktreeId]. The separate persistence migration does not run on this caller path.
If the source marker is true, the target ID remains unmarked. applyDefaultTerminalTabs can then create the default terminals again. Move the marker to targetWorktreeId, preserve an existing target marker, and delete the source marker. Add this case to workspace-session-worktree-id.test.ts.
There was a problem hiding this comment.
Important
Two regressions worth addressing before merge: the wake-respawn/bootstrap race reopened by dropping the wake-latch guard, and the write-once union resurrecting markers the bulk purge intentionally removed.
Reviewed changes — read all 23 files / 14 commits at c50aff0.
- Write-once marker persistence.
runtime-authored-workspace-session-fields.tsunions priordefaultTerminalTabsAppliedByWorktreeIdinto every session write;pty-binding-persistence.tsstamps it on any host PTY binding; hydration and the scoped hydration patch union disk with live store (local marks win). - Emptied-workspace tombstone.
apply-preparation-base.tskeeps an explicit emptytabsByWorktreerow for an affirmed-empty host frame while still deleting it for removal/unpublished frames;shouldBootstrapInitialWebRuntimeTerminalnow gates onhostSnapshotAffirmsWorktreeContentsand the sharedshouldAutoCreateInitialTerminalpredicate. - Per-environment bootstrap latch. New
web-runtime-initial-terminal-bootstrap.ts/-dispatch.tsreplace the closure-localrequestedInitialTerminal, withcreating/awaiting-mirrorphases released on failure, a mirrored row, or tracking teardown. - Re-key guard.
canonicalizeTerminalSessionWorktreeIdonly writes the target row when the source row exists, so a re-key cannot fabricate a tombstone.
ℹ️ Runtime-id fence on the post-adoption read was dropped
expectedRuntimeId: runtimeId was removed from the recoverWebSessionTerminalOrphansBeforeApply call in active-session-subscription.ts (it existed at the base, and the other four recovery call sites still pass it). Without it the fence falls back to getSessionTabsRuntimeIdFromResponse(adoptionResponse), which anchors to whichever runtime served terminal.adoptOrphans rather than to the triggering frame's publisher. With frame runtime R1 and adoption/list served by R2, the old fence rejected and the new one admits the R2 snapshot while R1 is still the current runtime. The removal is not mentioned in the PR body or the commit that made it — please confirm it is intentional.
Technical details
# Post-adoption read no longer fenced to the frame's runtime
## Affected sites
- `src/renderer/src/runtime/web-session-tabs-sync/active-session-subscription.ts:118-123` — options no longer pass `expectedRuntimeId`.
- `src/renderer/src/runtime/web-session-terminal-orphan-recovery.ts:221` — fallback `expectedRuntimeId ?? getSessionTabsRuntimeIdFromResponse(adoptionResponse)`.
- `src/renderer/src/runtime/web-session-terminal-orphan-recovery-adoption.ts:93-100` — an `undefined` expected id short-circuits the runtime comparison entirely.
## Required outcome
- The post-adoption `session.tabs.list` read should be fenced to the runtime that published the triggering frame, or the change should be recorded as deliberate.
## Open questions for the human
- If the fallback is intentional, what guarantees the adoption runtime equals the frame publisher across a runtime restart, when `getRuntimeEnvironmentRevision` is stable across restarts?DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
| snapshotIsFresh: decision.apply, | ||
| localTerminalCount | ||
| }) | ||
| const bootstrap = shouldBootstrapInitialWebRuntimeTerminal({ |
There was a problem hiding this comment.
skipAutomaticTerminal guard reopens a duplicate-terminal window on the wake path. The bootstrap decision no longer consults shouldSkipWebRuntimeWakeTerminalRespawn, and the two latches are independent: ensureWebRuntimeWorktreeTerminalAfterWake claims only the wake latch while its session.tabs.createTerminal is in flight, so a subscription frame for the same affirmed-empty, row-less worktree can claim the bootstrap latch and issue a second create. No test covers the overlap.
Technical details
# Wake respawn and initial bootstrap are no longer mutually exclusive
## Affected sites
- `src/renderer/src/runtime/web-session-tabs-sync/active-session-subscription.ts:148` — bootstrap no longer checks `shouldSkipWebRuntimeWakeTerminalRespawn`.
- `src/renderer/src/lib/web-runtime-worktree-terminal-after-wake.ts:92` — claims only `beginWebRuntimeWakeTerminalRespawn`.
- `src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.ts:30` — claims only `beginWebRuntimeInitialTerminalBootstrap`.
- `src/renderer/src/runtime/web-runtime-terminal-create-operation.ts:210,255` — the wake create writes no `tabsByWorktree` row until after the awaited create + `session.tabs.list` RPCs, so `hasPersistedTerminalState` stays false for the whole window.
## Required outcome
- At most one `session.tabs.createTerminal` is issued per empty workspace: a wake respawn in flight must suppress the subscription bootstrap, or both paths must share one latch.
## Suggested approach (optional)
- Re-include `shouldSkipWebRuntimeWakeTerminalRespawn(activeWorktreeId)` in the `requestedInitialTerminal` predicate, or have `ensureWebRuntimeWorktreeTerminalAfterWake` claim `beginWebRuntimeInitialTerminalBootstrap` for its no-row arm.
## Open questions for the human
- Was removing the wake guard intentional? It is not mentioned in the commit message, and the tombstone does not close the window because `bootstrap` is computed from pre-apply state.| return next | ||
| } | ||
| const nextMarks = next.defaultTerminalTabsAppliedByWorktreeId | ||
| const merged = { ...priorMarks, ...nextMarks } |
There was a problem hiding this comment.
{B:true}), so merged re-adds A; because the field is owner-keyed, the resurrected key then pins the removed worktree's metadata against pruning.
Technical details
# Write-once union resurrects explicitly removed markers
## Affected sites
- `src/main/persistence/runtime-authored-workspace-session-fields.ts:26` — unconditional union over prior marks.
- `src/renderer/src/store/slices/worktrees/teardown/worktree-purge-state.ts:243-245` — bulk purge drops the key (callers `fetch-all-worktrees.ts`, `fetched-worktree-merge.ts`, `worktree-event-runtime.ts`).
- `src/renderer/src/lib/workspace-session-patch.ts:189-195` — sends the whole reduced map when non-empty.
- `src/main/persistence/loading-store/session-snapshot-operations.ts:65-75` — a patch containing `tabsByWorktree` routes through `setLocalWorkspaceSession`/`setHostWorkspaceSession`, where `preserveRuntimeAuthoredWorkspaceSessionFields` runs.
- `src/main/persistence/restoring-sessions/session-worktree-ownership.ts:51` — field is `owner-keyed`, so the stale key counts as a live owner.
## Required outcome
- An explicit key removal by a writer must survive the union, while a writer that omits the field entirely still inherits prior marks.
## Suggested approach (optional)
- Only union when `next.defaultTerminalTabsAppliedByWorktreeId === undefined` (the clientHostedBrowserPages precedent), or have the purge path also clear the key through the owner-keyed prune that the explicit single-worktree removal already uses.
## Open questions for the human
- Is the permanent-write-once behaviour intended even across worktree removal, given the marker's `owner-keyed` role in metadata pruning?
ELI5
Orca now remembers when it has already created a workspace's default terminal and when a user deliberately closed the last terminal in a runtime-owned workspace. Reopening the app or returning to that workspace no longer creates unwanted terminals.
What Changed
defaultTerminalTabsAppliedByWorktreeIdthrough host writes, session replacement, and hydration.Why
The previous persistence path omitted the write-once marker, so default terminals could be recreated after restart or reattach. Runtime-owned workspaces also collapsed “never initialized” and “the user closed the last terminal” into the same state. The fix carries the existing state through the persistence and mirror paths and keeps the bootstrap decision scoped to the runtime/workspace pair.
Linked Issue
Fixes #18117
Fixes #17681
Related PRs: #18223, #18513
Credit: #18223 author Wooseong Kim (
innocarpe) is preserved as a co-author through the original authored commit included here; #18513 author Neil Parker (nwparker) supplied the runtime fix series.Visual Proof
N/A — this changes session persistence and terminal creation decisions; there is no intended visual redesign.
Testing
Focused Vitest suite: 9 files, 71 tests passed.
pnpm tcpassed.Oxlint and formatter hooks passed on changed files.
Readiness review: no changed mobile-facing RPC, pairing, relay, protocol, or persisted mobile key; no new timers, polling loops, subprocesses, dependencies, shell commands, or unbounded collections. The runtime latch is bounded by environment/worktree keys and is released on success, failure, mirror settlement, and teardown. Existing SSH/remote and cross-platform path abstractions remain in use.
AI Disclosure
Codex (GPT-6) assisted with implementation and validation.
Review
Please review the persistence merge semantics and the runtime/workspace-scoped bootstrap latch, especially mixed-version and reconnect behavior.
Agent skill upstream boundary
Notes
The changes preserve macOS, Linux, Windows, SSH, and folder-workspace behavior by using existing runtime and path abstractions. No wire opcode or public protocol shape was added.
Checklist
pnpm lint,pnpm test, andpnpm buildwere not run in full; CI should cover them.