Repository navigation
fix(GAP-03): stop offline-client reconnect from resurrecting server-closed tabs - #22334
LesleyMurfin wants to merge 23 commits into
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 44 functions across 24 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check ✅ Passed The title clearly identifies the GAP-03 fix and the primary behavior change: preventing offline reconnect from resurrecting server-closed tabs. Description check ✅ Passed The description is complete and directly matches the implementation. It explains the user impact, mechanism, scope, linked issue, testing, limitations, platform coverage, and checklist status. It also… Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
-
⚔️ Resolve merge conflicts 💡
-
- Resolve merge conflict in branch
fix/gap-03-offline-client-reconciliation
- Resolve merge conflict in branch
-
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: a4fd8e42-2e6f-47a6-a25b-2714692cb589
📥 Commits
Reviewing files that changed from the base of the PR and between 9ece273 and be4c61de4aa53b9fa6eaec5a5cbd957b2eefbf65.
📒 Files selected for processing (17)
config/scripts/run-offline-reconnect-tab-resurrection-docker.mjssrc/renderer/src/lib/remote-workspace-host-testimony.test.tssrc/renderer/src/lib/remote-workspace-host-testimony.tssrc/renderer/src/lib/workspace-session-host-contention.tssrc/renderer/src/lib/workspace-session-host-hydration.tssrc/renderer/src/lib/workspace-session-host-offline-reconnect.test.tssrc/renderer/src/lib/workspace-session-host-persistence.tssrc/renderer/src/lib/workspace-session-host-rapid-reconnect-cycles.test.tssrc/renderer/src/lib/workspace-session-host-reconciliation.tssrc/renderer/src/lib/workspace-session-host-shadow-testimony.test.tssrc/renderer/src/lib/workspace-session-host-shadow-testimony.tssrc/renderer/src/lib/workspace-session-host-write-race.test.tssrc/renderer/src/lib/workspace-session-ssh-partition-ownership.test.tssrc/renderer/src/lib/workspace-terminal-host-authority.tssrc/shared/workspace-session-stranded-partition-adoption.tstests/e2e/gap-03-offline-reconnect-tab-resurrection.spec.tstests/e2e/helpers/docker-ssh-relay-target.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| attachHostSessionShadow(slices, state.contestedHostWorkspaceSessions, routing.claims, mode) | ||
| attachHostSessionShadow( | ||
| slices, | ||
| shadowRowsTheHostHasNotAnswered(state.contestedHostWorkspaceSessions, state), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Map the file structure first
ast-grep outline src/renderer/src/lib/workspace-session-host-persistence.ts --view expandedRepository: stablyai/orca
Length of output: 1035
🏁 Script executed:
#!/bin/bash
# Read the HostPersistenceState type and the function around line 213-230
sed -n '37,65p' src/renderer/src/lib/workspace-session-host-persistence.ts
echo "---"
sed -n '213,230p' src/renderer/src/lib/workspace-session-host-persistence.tsRepository: stablyai/orca
Length of output: 2170
🏁 Script executed:
#!/bin/bash
# Search for shadowRowsTheHostHasNotAnswered definition
rg -n "shadowRowsTheHostHasNotAnswered" src/renderer/src/lib/
echo "---"
# Search for hostHasAnsweredForTarget
rg -n "hostHasAnsweredForTarget" src/renderer/src/lib/
echo "---"
# Search for write-race test
rg -n "write-race" src/renderer/src/lib/Repository: stablyai/orca
Length of output: 3221
🏁 Script executed:
#!/bin/bash
# Read shadowRowsTheHostHasNotAnswered implementation
sed -n '1,50p' src/renderer/src/lib/workspace-session-host-shadow-testimony.ts
echo "---"
# Read hostHasAnsweredForTarget implementation
sed -n '1,30p' src/renderer/src/lib/remote-workspace-host-testimony.tsRepository: stablyai/orca
Length of output: 2636
🏁 Script executed:
#!/bin/bash
# Read the full write-race test
cat -n src/renderer/src/lib/workspace-session-host-write-race.test.tsRepository: stablyai/orca
Length of output: 13318
Confirm that deferred writes cannot override a live host snapshot.
The supplied test file demonstrates the write-race concern. Test case 2 (lines 129–199) shows that while hostHasAnsweredForTarget is false, declined tabs are included in the write payload. Test case 3 (lines 201–266) shows that the protection via shadowRowsTheHostHasNotAnswered filters those tabs once testimony arrives.
The race condition exists between when api.set is called asynchronously with a payload (generated while hostHasAnsweredForTarget was false) and when the live snapshot lands (causing hostHasAnsweredForTarget to become true). If the deferred api.set delivery reaches the host after the snapshot establishes authoritative state, the outdated payload can restore declined tabs.
This PR formalizes RemoteWorkspaceTestimonyState as part of HostPersistenceState so that the filter function can access testimony fields. However, it does not prevent in-flight writes sent before testimony arrived from overwriting the authoritative snapshot after it lands.
Confirm whether the write protocol orders api.set delivery against live snapshots, or gate the initial write until testimony arrives, to prevent restoring server-closed tabs. If no such guarantee exists, establish ordering or gating.
There was a problem hiding this comment.
Fixed in commit 0f47518.
An in-flight write prepared before testimony arrived (hostHasAnsweredForTarget was false) contained parked shadow rows that could deliver after the live snapshot landed, overwriting authoritative state.
We resolved this in workspace-session-host-persistence.ts:
- Introduced
enqueueHostPartitionWritewhich serializes writes per host partition and tracks monotonic write generations (hostPartitionWriteGenerations). - When a write was prepared without host testimony, right before dispatch/after dequeuing, it verifies whether testimony has arrived for that target or a newer generation was dispatched; if so, the stale pre-testimony payload is discarded rather than overwriting the partition.
- Added red-first regression test in
workspace-session-host-write-race.test.tsverifying that a pre-testimony write in-flight while a snapshot lands is dropped and does not overwrite the snapshot.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/renderer/src/lib/workspace-session-host-write-race.test.ts --view expanded
ast-grep outline src/renderer/src/lib/workspace-session-host-persistence.ts --match 'enqueueHostPartitionWrite|persistWorkspaceSessionByHost|patchWorkspaceSessionByHost' --view expanded
rg -n -C3 'enqueueHostPartitionWrite|snapshot|in.flight|in-flight|testimony|setSync' src/renderer/src/lib/workspace-session-host-write-race.test.ts
rg -n -C3 'remoteWorkspaceHydratedTargetIds|persistWorkspaceSessionByHost\(|patchWorkspaceSessionByHost\(' src/renderer/src --type ts -g '!**/*.test.ts' | head -220Length of output: 18819
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '268,400p' src/renderer/src/lib/workspace-session-host-write-race.test.ts
sed -n '165,198p' src/renderer/src/store/slices/ssh.ts
sed -n '250,306p' src/renderer/src/lib/workspace-session-host-persistence.ts
sed -n '95,130p' src/renderer/src/app-shell/use-app-session-persistence.tsLength of output: 10765
@LesleyMurfin, thanks for the update. Commit 0f475182aa protects a queued write in the new test, but the original in-flight race remains.
enqueueHostPartitionWrite calls perform() immediately when the host has no write in progress. If testimony arrives after that call, the write cannot be discarded by the queue check. The test holds an earlier write in progress, so it does not exercise that case. It also mutates the same state object before releasing the queue. The production testimony actions create new Zustand state fields, so a queued write retains the earlier state snapshot.
Please test a write that has already reached api.set when the live snapshot lands. The fix needs an ordering guarantee for that write and the snapshot, rather than only a check before queued writes dispatch.
You are interacting with an AI system.
There was a problem hiding this comment.
Addressed across commits 7d4f12e and 2d53e8f.
- In commit 7d4f12e,
enqueueHostPartitionWritewas wired with live testimony evaluation (getLiveStatethunk) and partition write serialization, ensuring that pre-testimony queued writes re-evaluate live testimony before dispatching. - In commit 2d53e8f, we resolved the queue discard concern by gating pre-testimony discards on
carriesParkedShadowRows: only writes that actually carry parked shadow entries are discarded upon testimony arrival. Writes containing live local edits or delta patches (patchWorkspaceSessionByHost) without parked shadow entries proceed without being discarded. - Unit test
does not discard a pre-testimony write when carriesParkedShadowRows is false even if testimony arrives before dequeuinginworkspace-session-host-write-queue.test.tsexercises and validates this ordering and survival contract.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/renderer/src/lib/workspace-session-host-write-queue.ts --view expanded
ast-grep outline src/renderer/src/lib/workspace-session-host-write-queue.test.ts --view expanded
ast-grep outline src/renderer/src/lib/workspace-session-host-write-race.test.ts --view expanded
sed -n '1,220p' src/renderer/src/lib/workspace-session-host-write-queue.ts
rg -n -C8 'does not discard a pre-testimony|in.flight|snapshot lands|queued|before dequeuing' src/renderer/src/lib/workspace-session-host-write-queue.test.ts src/renderer/src/lib/workspace-session-host-write-race.test.tsLength of output: 25040
@LesleyMurfin, thanks for the clarification. The changes address the queued-write concerns:
getLiveStatelets queued writes evaluate current testimony.carriesParkedShadowRowsprevents the testimony guard from discarding writes without parked shadow entries.
Verification of the original in-flight race remains inconclusive. enqueueHostPartitionWrite checks shouldDiscard() before perform(). Once perform() calls api.set, the queue cannot discard that write if testimony arrives afterward.
The write-race test still holds an earlier write open and checks a second write before dispatch. The test you cited verifies preservation of writes without parked shadow entries. Neither test establishes ordering between an already-dispatched stale write and the live snapshot.
Please retry verification with a test that pauses delivery after the stale write enters api.set, applies the live snapshot, and then releases delivery. Alternatively, identify the write-protocol guarantee that prevents the stale write from overwriting the snapshot.
✏️ Learnings added
Learnt from: LesleyMurfin
Repo: stablyai/orca PR: 22334
File: src/renderer/src/lib/workspace-session-host-persistence.ts:242-242
Timestamp: 2026-10-01T05:11:21.358Z
Learning: In stablyai/orca, workspace-session-host persistence uses carriesParkedShadowRows to limit pre-testimony queue discards to writes containing parked shadow entries. Writes without parked shadow entries, including live local edits and delta patches, must not be discarded merely because SSH host testimony arrives before dispatch.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two things to settle before merge: the fix runs only on the boot-hydration path, so it may not cover the live wake-from-sleep repro in #22038; and slice 3's withholding can erase offline-created rows (including unsaved editor drafts) from the ssh:<targetId> partition once the host answers. Details below and inline.
Reviewed changes
- Boot-time decline for confirmed-but-offline hosts —
sshPartitionCatalogAttributionnow also reportsconfirmedSessionKeysByHostId(worktree ids the catalog resolves to exactly the partition host via the repo row's ownexecutionHostId), andreconciledWorktreeIdsForHostreturns that set (minus migrated ids) only when the target was not connected at last shutdown. - Hydration wiring —
fetchWorkspaceSessionWithRuntimeHostOwnerscapturesactiveConnectionIdsAtShutdownonce and passes the reconciled set intoadoptStrandedHostPartitionSession, which declines a worktree when the base has notabsByWorktreerow and the host row is non-empty. - Write-side withholding (slice 3) —
shadowRowsTheHostHasNotAnswereddrops non-folder parked rows for an SSH host oncehostHasAnsweredForTargetis true; wired intosplitWorkspaceSessionForWritefor every write mode. - Testimony refactor —
hostHasAnsweredForTargetextracted intoremote-workspace-host-testimony.tsand reused byresolveDirectSshAuthority(behavior-preserving). - Tests — 5 new unit suites plus a Docker two-launch e2e;
workspace-session-ssh-partition-ownership.test.tsadjusted so the new decline does not fire for its sibling.
⚠️ The fix is boot-only, so #22038's live wake/reconnect repro may still resurrect tabs
fetchWorkspaceSessionWithRuntimeHostOwners has exactly one production caller (use-app-startup-hydration.ts:147), so nothing here runs on a live SSH reconnect. That path is applyDirectSshRemoteWorkspaceSnapshot → mergeDirectSshRemoteWorkspaceSession, whose hostUnknown branch (remote-workspace-session-merge.ts:194-210) keeps any local tab absent from the fresh host snapshot. A close by another client writes no local tombstone, so the server-closed tab is re-added — a behavior pinned as a deliberate trade by remote-workspace-session-merge-local-survival.test.ts:93-107. Issue #22038's repro is a wake-from-sleep reconnect with no relaunch, and the new e2e's own fault-injection comment (tests/e2e/gap-03-offline-reconnect-tab-resurrection.spec.ts:248-260) deletes activeConnectionIdsAtShutdown precisely to keep the second launch off the live path. So the linked Fixes #22038 may be narrower than the issue.
Technical details
# Boot hydration vs live reconnect coverage
## Affected sites
- `src/renderer/src/app-shell/use-app-startup-hydration.ts:147` — the only production call site of `fetchWorkspaceSessionWithRuntimeHostOwners`.
- `src/renderer/src/hooks/remote-workspace-target-sync.ts:70`, `src/renderer/src/hooks/remote-workspace-snapshot-apply.ts:178` — the live reconnect path, untouched by this PR.
- `src/renderer/src/hooks/remote-workspace-session-merge.ts:194-210` — `hostUnknown` re-adds local tabs the fresh snapshot omits.
- `src/renderer/src/hooks/remote-workspace-session-merge-local-survival.test.ts:93-107` — pins the live behavior as an accepted trade.
## Required outcome
- Confirm which code path #22038's repro actually exercises, then either narrow the linked-issue claim or extend coverage to the live path.
## Open questions for the human
- Is the live-path behavior treated as a separate, already-accepted issue (the STA-3374 sibling), or must this PR cover it to resolve #22038?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
be4c61d to
204e1d7
Compare
Finding 5 Investigation & Scope Resolution: Boot-Time Hydration vs. Live-Wake CoverageWe have definitively investigated whether the live wake-from-sleep path (without app relaunch) still resurrects a server-closed tab against current HEAD: Definitive Answer: YES, the live wake-from-sleep path (without app relaunch) does still resurrect a server-closed tab. Root Cause & Architectural Trade
When a running client reconnects live after sleep or network interruption without being relaunched, it executes As explicitly documented and pinned by
If we were to modify Scope Decision & DocumentationRather than unsafely modifying
The PR body and spec ( |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 1f55436e-23a7-40d7-8c73-fa499a4ccfe1
📥 Commits
Reviewing files that changed from the base of the PR and between be4c61de4aa53b9fa6eaec5a5cbd957b2eefbf65 and 0d484b6.
📒 Files selected for processing (14)
src/renderer/src/app-shell/use-app-session-persistence.tssrc/renderer/src/hooks/ipc-events/mobile-terminal-close-ipc-bridge.tssrc/renderer/src/lib/workspace-session-host-contention.tssrc/renderer/src/lib/workspace-session-host-hydration.tssrc/renderer/src/lib/workspace-session-host-offline-reconnect.test.tssrc/renderer/src/lib/workspace-session-host-persistence.tssrc/renderer/src/lib/workspace-session-host-shadow-testimony.test.tssrc/renderer/src/lib/workspace-session-host-shadow-testimony.tssrc/renderer/src/lib/workspace-session-host-write-queue.test.tssrc/renderer/src/lib/workspace-session-host-write-queue.tssrc/renderer/src/lib/workspace-session-host-write-race.test.tssrc/shared/workspace-session-stranded-partition-adoption.tssrc/shared/workspace-session-stranded-partition-parking.tstests/e2e/gap-03-offline-reconnect-tab-resurrection.spec.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.
| state, | ||
| replaceable: true, | ||
| getLiveState: state.getLiveTestimonyState, | ||
| carriesParkedShadowRows: hostsWithAttachedShadow.has(hostId) |
There was a problem hiding this comment.
api.set can replace the newer snapshot and bring back stale tabs.
Knowledge Base Used: Desktop persistence and lifecycle
There was a problem hiding this comment.
Important
The write-side withholding can delete the only copy of a declined worktree's terminal rows once the host answers — including a tab created offline that was never uploaded. The PR body describes this state as "parked, never deleted", and slice 8 protected the non-terminal half (openFilesByWorktree), but the terminal half is still erased. Details inline.
Reviewed changes
- Boot-time GAP-03 decline —
sshPartitionCatalogAttributionnow returnsconfirmedSessionKeysByHostId, andreconciledWorktreeIdsForHostyields that set (minus migrated ids) only when the target was not connected at last shutdown; hydration passes it asreconciledWorktreeIds. - Adoption decline — a confirmed worktree with no base
tabsByWorktreekey and a non-empty host row is removed fromadoptablerather than gap-filled, with canonicalworktree:keys normalized. - Dependent-row parking —
partitionRowsTheWriteWontReturnmoved toworkspace-session-stranded-partition-parking.tsand now walks tab/pane-keyed rows so a declined worktree's layout, relay-session and PTY rows park and restore with it. - Write-side withholding — new
shadowRowsTheHostHasNotAnswereddrops non-folder terminal parkable entries from the write shadow oncehostHasAnsweredForTargetis true; non-terminal fields survive. - Serialized partition writes — new
enqueueHostPartitionWritequeue serializes per-host writes, advances a generation for replaceable writes, and discards a write prepared before testimony once live testimony lands;registerLiveTestimonyProvideris wired fromuse-app-session-persistence. - Testimony refactor —
hostHasAnsweredForTargetextracted intoremote-workspace-host-testimony.tsand reused byresolveDirectSshAuthority(behavior-preserving). - Tests — new unit suites (offline-reconnect, rapid cycles, write-queue, write-race, shadow-testimony, testimony) plus a two-launch Docker e2e.
ℹ️ The linked issues describe a live wake-from-sleep reconnect this PR does not touch
fetchWorkspaceSessionWithRuntimeHostOwners has one production caller (use-app-startup-hydration.ts:147), so the fix runs only at boot/relaunch. The live reconnect path (mergeDirectSshRemoteWorkspaceSession's hostUnknown branch, remote-workspace-session-merge.ts:194-210) still keeps host-absent local tabs; #22038's repro is a wake-from-sleep without relaunch. The PR body now documents this as a scope boundary, which is the right call — but the PR still carries Fixes/close markers for issues whose repro is the live path. Worth confirming whether closing them is intended or only the boot path is in scope here.
Technical details
# Boot hydration vs live reconnect coverage
## Affected sites
- `src/renderer/src/app-shell/use-app-startup-hydration.ts:147` — the only production call site of `fetchWorkspaceSessionWithRuntimeHostOwners`.
- `src/renderer/src/hooks/remote-workspace-session-merge.ts:194-210` — `hostUnknown` re-adds local tabs the fresh snapshot omits; untouched.
- `src/renderer/src/hooks/remote-workspace-session-merge-local-survival.test.ts:93-107` — pins the live behavior as a deliberate trade.
## Open questions for the human
- Are the linked issues (#21341, #22038) resolved by the boot/relaunch path alone, or does the live-wake path need its own fix before they close?ℹ️ Nitpicks
workspace-session-host-write-queue.ts:82,102: a queued{ generation, replaceable: false }write raises the per-host generation (advancesGeneration) yet is itself immune to the fence (isReplaceableis false), so it can discard a queuedreplaceablefull snapshot. No production caller passesgenerationyet, so this is latent, but the existing test only pins the opposite direction.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 0c2fb1c4-a4de-471e-b98a-40e89dfa16fa
📒 Files selected for processing (5)
src/renderer/src/lib/workspace-session-host-contention.tssrc/renderer/src/lib/workspace-session-host-persistence.tssrc/renderer/src/lib/workspace-session-host-write-queue.test.tssrc/renderer/src/lib/workspace-session-host-write-queue.tstests/e2e/gap-03-offline-reconnect-tab-resurrection.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/gap-03-offline-reconnect-tab-resurrection.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| continue | ||
| } | ||
| target[key] = entry | ||
| hostsWithAttachedShadow.add(hostId) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Track only shadow rows that testimony can invalidate.
This set includes folder shadow rows and non-terminal shadow rows. shadowRowsTheHostHasNotAnswered explicitly preserves those rows after testimony.
If a queued patch attaches only these preserved rows, carriesParkedShadowRows still becomes true. When testimony arrives before dispatch, the queue discards the entire patch. This loses valid folder or non-terminal changes even though testimony invalidates none of its shadow rows.
Return separate metadata for attached rows that the testimony filter would withhold. Use that metadata for the queue discard condition.
There was a problem hiding this comment.
ℹ️ The new commit is a sound narrowing of the write-queue discard; one small stale-config nit below.
Reviewed changes (since 0d484b6)
- Pre-testimony discard now requires parked rows —
attachHostSessionShadowreturns the host ids it actually attached shadow entries to,splitWorkspaceSessionForWritecarries that set out, and both writers passcarriesParkedShadowRowsintoenqueueHostPartitionWrite, which only setspreparedWithoutTestimonywhen the write genuinely carries parked shadow rows. This stops a fresh-edit patch prepared before testimony from being dropped when the snapshot lands, while keeping the protection for writes that could resurrect declined tabs. The new unit case pins the false branch with exact assertions. CLOSE_COUNTclamp — the Docker spec now pinsCLOSE_COUNT = TAB_COUNTand hardensORCA_GAP03_TAB_COUNTparsing withparseInt/Number.isFinite, so a partial closure can never silently miss the decline path.
The gate is correctly scoped: a write with no parked rows cannot reintroduce a declined worktree (the boot session never adopted those rows), so skipping the discard for it does not reopen the write race.
ℹ️ Nitpicks
config/scripts/run-offline-reconnect-tab-resurrection-docker.mjs:26-28still reads and forwardsORCA_GAP03_CLOSE_COUNT, and its header comment documents it as a tunable, but the spec no longer consumes it. Either drop the forwarding or keep the spec reading it.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| export const SSH_TERMINAL_TESTIMONY_FIELDS: Partial<Record<keyof WorkspaceSessionState, true>> = { | ||
| tabsByWorktree: true, | ||
| terminalLayoutsByTabId: true, | ||
| remoteSessionIdsByTabId: true, | ||
| terminalPtyIncarnationsByPaneKey: true, | ||
| activeTabIdByWorktree: true, | ||
| defaultTerminalTabsAppliedByWorktreeId: true | ||
| } |
There was a problem hiding this comment.
unifiedTabs, groups, and split layout in the shadow. A later full write for another worktree on the same host can persist those rows without the terminal row. On the next ordinary restart, hydration reads unifiedTabs and can show the closed tab again. Withhold the closed terminal’s tab chrome and layout while preserving editor state.
| export const SSH_TERMINAL_TESTIMONY_FIELDS: Partial<Record<keyof WorkspaceSessionState, true>> = { | ||
| tabsByWorktree: true, | ||
| terminalLayoutsByTabId: true, | ||
| remoteSessionIdsByTabId: true, | ||
| terminalPtyIncarnationsByPaneKey: true, | ||
| activeTabIdByWorktree: true, | ||
| defaultTerminalTabsAppliedByWorktreeId: true | ||
| } |
There was a problem hiding this comment.
localOnlyScrollbackByTabId row now survives this filter. Because the tab row has been withheld, a later full write cannot identify the scrollback’s worktree and can write it back to the SSH partition as an orphan. Main-side cleanup does not prune this field, so closed tabs can retain scrollback across restarts. Keep scrollback for tabs the host still reports, but retire it when the tab is confirmed gone.
Knowledge Base Used: Persist parked remote terminal scrollback across restarts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 203ea3df-1653-4e77-ba36-e6f5ca67fa92
📒 Files selected for processing (6)
config/scripts/run-offline-reconnect-tab-resurrection-docker.mjssrc/renderer/src/lib/workspace-session-host-contention.test.tssrc/renderer/src/lib/workspace-session-host-contention.tssrc/renderer/src/lib/workspace-session-host-persistence.tssrc/renderer/src/lib/workspace-session-host-shadow-testimony.test.tssrc/renderer/src/lib/workspace-session-host-shadow-testimony.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.
| export const SSH_TERMINAL_TESTIMONY_FIELDS: Partial<Record<keyof WorkspaceSessionState, true>> = { | ||
| tabsByWorktree: true, | ||
| terminalLayoutsByTabId: true, | ||
| remoteSessionIdsByTabId: true, | ||
| terminalPtyIncarnationsByPaneKey: true, | ||
| activeTabIdByWorktree: true, | ||
| defaultTerminalTabsAppliedByWorktreeId: true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline \
src/shared/workspace-session-stranded-partition-adoption.ts \
--items all --view expanded
ast-grep outline \
src/renderer/src/lib/workspace-session-host-hydration.ts \
--items all --view expanded
rg -n -C5 --type ts \
'\bunifiedTabs\b|\btabGroups\b|\btabGroupLayouts\b|\bactiveConnectionIdsAtShutdown\b' \
src/shared/workspace-session-stranded-partition-adoption.ts \
src/renderer/src/lib/workspace-session-host-hydration.ts
rg -n -C4 --type ts -g '!**/*.test.ts' \
'\bsession\.unifiedTabs\b|\bpayload\.unifiedTabs\b|\brestoredSession\.unifiedTabs\b' \
src/renderer/src src/sharedRepository: stablyai/orca
Length of output: 12162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- testimony ---'
cat -n src/renderer/src/lib/workspace-session-host-shadow-testimony.ts
printf '%s\n' '--- contention relevant symbols ---'
rg -n -C12 --type ts 'attachHostSessionShadow|replacement|unifiedTabs|HostSessionSlices' src/renderer/src/lib/workspace-session-host-contention.ts
printf '%s\n' '--- adoption relevant sections ---'
sed -n '150,290p' src/shared/workspace-session-stranded-partition-adoption.ts
printf '%s\n' '--- hydration relevant sections ---'
sed -n '140,285p' src/renderer/src/lib/workspace-session-host-hydration.ts
printf '%s\n' '--- terminal hydration ---'
sed -n '100,180p' src/renderer/src/store/slices/terminal-session-row-hydration.ts
sed -n '330,360p' src/renderer/src/store/slices/tabs-hydration.tsRepository: stablyai/orca
Length of output: 32629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- attach remainder ---'
sed -n '320,375p' src/renderer/src/lib/workspace-session-host-contention.ts
printf '%s\n' '--- unifiedTabs ownership/type ---'
rg -n -C5 --type ts 'unifiedTabs|WORKSPACE_SESSION_FIELD_OWNERSHIP' src/shared/workspace-session-host-field-ownership.ts src/shared/workspace-session-state-types.ts
printf '%s\n' '--- testimony helper usages/tests ---'
rg -n -C6 --type ts 'shadowRowsTheHostHasNotAnswered|SSH_TERMINAL_TESTIMONY_FIELDS|isShadowRowSubjectToHostTestimony' srcRepository: stablyai/orca
Length of output: 31120
Filter terminal entries from unifiedTabs before attaching the shadow.
unifiedTabs is excluded from SSH testimony filtering. attachHostSessionShadow can attach its missing worktree entry from the parked shadow. Unified hydration then prefers unifiedTabs, and terminal hydration reads terminal IDs from it. A closed terminal can reappear after restart.
Do not only add unifiedTabs to the allowlist. The current helper removes a complete worktree-keyed value. That would also remove editor and browser entries. Filter each unifiedTabs array by contentType === 'terminal' and preserve the remaining entries.
🐛 Suggested fix
export const SSH_TERMINAL_TESTIMONY_FIELDS: Partial<Record<keyof WorkspaceSessionState, true>> = {
tabsByWorktree: true,
+ unifiedTabs: true,
terminalLayoutsByTabId: true,
remoteSessionIdsByTabId: true,
terminalPtyIncarnationsByPaneKey: true,
activeTabIdByWorktree: true,
defaultTerminalTabsAppliedByWorktreeId: true
}
+function isTerminalUnifiedTab(value: unknown): boolean {
+ return (
+ typeof value === 'object' &&
+ value !== null &&
+ 'contentType' in value &&
+ value.contentType === 'terminal'
+ )
+}
+
...
- if (!isShadowRowSubjectToHostTestimony(field, owningWorkspaceKey)) {
+ const subjectToTestimony = isShadowRowSubjectToHostTestimony(field, owningWorkspaceKey)
+ if (subjectToTestimony && field === 'unifiedTabs' && Array.isArray(value)) {
+ const survivingTabs = value.filter((tab) => !isTerminalUnifiedTab(tab))
+ if (survivingTabs.length > 0) {
+ survivingRecord[key] = survivingTabs
+ }
+ fieldWithheld = true
+ } else if (!subjectToTestimony) {
survivingRecord[key] = value
} else {
fieldWithheld = true
}
There was a problem hiding this comment.
Important
The new import in workspace-session-host-contention.ts creates a dependency cycle that fails CI's audit:code-quality:native lint step. The field-set narrowing itself looks correct. Details inline.
Reviewed changes (since 2d53e8f)
- Testimony-invalidatable shadow tracking —
attachHostSessionShadownow returnshostsWithTestimonyInvalidatableShadowalongsidehostsWithAttachedShadow, and the write paths pass the narrower set ascarriesParkedShadowRowsso only a write that could reintroduce a declined tab is fenced by late testimony. - Testimony field set narrowed —
SSH_TERMINAL_TESTIMONY_FIELDSdroppedlocalOnlyScrollbackByTabId(local-only) and the tab-group/layout fields direct-SSH never projects; the folder/field predicate is extracted intoisShadowRowSubjectToHostTestimonyso the read-side and write-side use one rule. - Runner cleanup — removed the now-dead
ORCA_GAP03_CLOSE_COUNTforwarding from the Docker runner, matching the spec'sCLOSE_COUNT = TAB_COUNTclamp.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
This run hit its time limit while still working, so it stopped before finishing. Give runs longer with
|
Reproduces the additive-merge bug in adoptStrandedHostPartitionSession (src/shared/workspace-session-stranded-partition-adoption.ts): when a worktree's tabs are all closed server-side while a client is offline, the client's base tabsByWorktree row for that worktree becomes empty, so workspacesTheBaseOwns() no longer protects it and the stale cached ssh:* partition's closed tabs are unconditionally re-adopted into the merged session on reconnect. - RED: closed tabs must not reappear after reconnect (fails today) - GREEN: a genuinely offline-created tab (never synced) must survive reconnect -- the stablyai#12721 non-regression constraint the downstream fix must not break Fixes stablyai#22038. No fix code included; implementation of reconcileLocalTabsWithServerManifest()/hasPendingReconnectionHandshake is a separate downstream task.
adoptStrandedHostPartitionSession's workspacesTheBaseOwns() only ever protected a worktree from stranded-partition adoption when the base held a non-empty tabsByWorktree row. Once every tab for a worktree closed server-side while a client was offline, the base's row went empty/absent, so the repair no longer protected it and unconditionally folded in whatever tabs were still sitting in the client's stale on-disk ssh:<targetId>/runtime cache from before it disconnected -- resurrecting closed tabs on reconnect and round-tripping them back out via persistWorkspaceSessionByHost to the host partition and every other connected client (stablyai#22038, revive_labs#962). Fix: sshPartitionCatalogAttribution (extracted into a new workspace-session-host-reconciliation.ts module alongside unownedSessionKeys, to keep workspace-session-host-hydration.ts under this repo's 300-line file budget) now also tracks, per SSH host, the worktree ids the repo catalog positively and *currently* confirms belong to exactly that host -- via the row's own executionHostId field, not a bare connectionId fallback. That fallback already treats a legacy catalog row as resolved, which is exactly the pre-stablyai#12723 shape the one-shot gap-fill repair exists for, so it stays on the unconditional path. adoptStrandedHostPartitionSession takes the confirmed set as reconciledWorktreeIds: for one of these ids, an absent tabsByWorktree row on the base is no longer read as an unverifiable stablyai#12721 gap -- it's the server's current truth of zero tabs -- so a host row that still names real (non-empty) tabs is declined rather than adopted. An id the catalog also marks foreign for another host (residue proving a genuine migration) is exempted, so a freshly-moved connection's real tabs still adopt normally. Declining never destroys anything: the id simply drops out of adoptedWorkspaceIds, so partitionRowsTheWriteWontReturn parks the stale row in the write-side shadow the same way a contested id already does. One regression fixture (workspace-session-ssh-partition-ownership.test .ts's "keeps a declined row..." case) gave its sibling worktree a bare `local: session({})` with no tabsByWorktree entry at all for the sibling -- structurally identical to the GAP-03 bug shape itself. Gave it the same known-worktree explicit-empty-row shape already used throughout the round-trip suite (`strandedPartitions()`), which is what that test's own "sibling is adopted and routes back" premise actually depends on. Tests: - workspace-session-host-offline-reconnect.test.ts: RED -> GREEN (closed tab no longer resurrected); stablyai#12721-safety scenario (offline- created, never-synced tab) stays GREEN. - workspace-session-ssh-partition-ownership.test.ts, workspace-session-ssh-partition-round-trip.test.ts, workspace-session-host-split.test.ts, workspace-session-partition-owner.test.ts, ssh-host-partition-session-export.test.ts: all still GREEN, zero regressions. - Also re-ran workspace-session-host-contention.test.ts, workspace-session-host-persistence.test.ts and main/ipc/remote-workspace.test.ts (other direct callers/dependents of the touched functions): all green. - 115/115 across all 9 files above. No PR opened; stops at a locally-ready, committed branch per this slice's constraints.
…rness Adds a two-launch (quit+relaunch) Playwright harness that reproduces the GAP-03 repro exactly: pair a client to a Dockerized SSH host, open N tabs, quit the client, close M tabs' local base row while leaving the SSH host-partition mirror stale (simulating 'closed server-side while offline'), relaunch, and measure resurrection plus the stablyai#12721 non-regression. Modeled on run-ssh-docker-e2e.mjs (wrapper shape), golden-quit-relaunch-session.spec.ts and persisted-session-production-upgrade.spec.ts (two-launch + on-disk fixture injection), and the docker-ssh-relay-* helpers already used by ssh-lost-kill-tab-resurrection.spec.ts. Also hardens docker-ssh-relay-target.ts: some hosts' tmp directories carry a default ACL that overrides ssh-keygen's normal 0600 permissions, which sshd then refuses under StrictModes. One chmodSync call fixes it unconditionally for every SSH-relay Docker e2e spec, not just this one. BEFORE-state (pre-fix, commit 36beb65f6b) RED evidence captured separately; see revive_labs specs/22038-gap-03-docker-cross-site-test-harness.md. Slice A of the GAP-03 before/after test-battery ticket: harness + baseline only. AFTER-state measurement, Calgary-site run, and aggregate report are follow-up slices.
…ises the fix Slice A's harness (commit 8b26787bdc) measured 2/2 tabs resurrected on both pre-fix and (unverified) post-fix code, and offlineDraftSurvived: false even on pre-fix code -- an anomaly flagged for the next agent. Root-caused and fixed three independent harness defects, none in product code: 1. "Closed tabs" fault injection wrote workspaceSession.tabsByWorktree [worktreeId] = [] (a filter of an already-empty array) instead of deleting the key. adoptStrandedHostPartitionSession's GAP-03 decline path keys its verdict on Object.hasOwn(base.tabsByWorktree, workspaceId), and hasOwn is true for a key holding [] exactly as much as a populated array -- silently defeating the fix regardless of which code was under test. Also switched CLOSE_COUNT/TAB_COUNT defaults to close every opened tab (2/2): the fix's own decline logic is all-or-nothing (see its doc comment), so a scenario that leaves any survivor tab in the worktree never reaches the reconciledWorktreeIds path at all, pre- or post-fix, and proves nothing about the regression. 2. The stablyai#12721 offline-draft tab was injected only into legacy tabsByWorktree, never into unifiedTabs/tabGroups/tabGroupLayouts/ activeGroupIdByWorktree/activeTabIdByWorktree. The first launch's own quit-time write already populates unifiedTabs+tabGroups, so buildHydratedTabState takes the unified branch on the second launch and enumerates worktrees from unifiedTabs alone -- a legacy-only row is invisible there, and even where tabsByWorktree is swept for reconciliation, a legacy row with no live PTY and no unified twin is exactly what getOrphanTerminalIds deletes as an orphan on the same boot. This was the flagged anomaly: offlineDraftSurvived: false regardless of which product commit was under test, because the tab never reached a store the modern hydration path reads from. 3. Both SSH targets' activeConnectionIdsAtShutdown entries survived into the fault-injected profile, so the second launch auto- reconnected live SSH and pulled a fresh PTY snapshot straight off the still-alive Docker container (applyDirectSshRemoteWorkspaceSnapshot / mergeDirectSshRemoteWorkspaceSession) -- a code path GAP-03 never touches, since this fixture never actually kills the "closed" PTYs on the container. That live resync silently overwrote whatever the boot-time fetchWorkspaceSessionWithRuntimeHostOwners merge had correctly decided. Clearing the field keeps the second launch on the one path the fix targets: persisted-state hydration at boot, confirmed by temporary instrumentation showing adoptStrandedHostPartitionSession's own "adoptable" set already correctly emptied out before this fix (root-caused, then reverted). Measured clean before/after against the corrected harness by temporarily reverting to 36beb65f6b (fix's own parent) and back: BEFORE (36beb65f6b): resurrectedCount 2/2, offlineDraftSurvived true AFTER (129d11ad4d): resurrectedCount 0/2, offlineDraftSurvived true offlineDraftSurvived is true on BOTH runs, confirming the stablyai#12721 anomaly was a harness defect throughout, not a real regression on either commit. All 64 tests across the fix's own directly-affected unit-test files still pass unchanged. Also updated run-offline-reconnect-tab-resurrection-docker.mjs to stop overriding the spec's own TAB_COUNT/CLOSE_COUNT defaults.
…ing PR stablyai#968 scenarios reconciledWorktreeIdsForHost applied unconditionally to every catalog-confirmed worktree, regardless of whether this client's SSH connection to that host was ever actually interrupted. Since an SSH-hosted worktree's tabsByWorktree row never routes to the 'local' base partition in the first place (buildHostSessionRouting always sends it to ssh:<targetId>), base.tabsByWorktree is absent for EVERY confirmed worktree on EVERY boot -- closed or not. Combined with remote-repo-registration.ts stamping the modern executionHostId field on every SSH repo it registers (not a rare legacy shape), this meant an ordinary quit + relaunch with nothing closed by anyone dropped every still-open tab on every SSH worktree whose repo carries the modern stamp -- the opposite of GAP-03's own goal, and a severe regression the existing 115-test suite, the Docker e2e harness (which always closes every opened tab, CLOSE_COUNT=TAB_COUNT by design) and the 4-client live verification never exercised, because none of them ever tested a worktree with a genuine SURVIVOR tab through the confirmed attribution path. Fix: gate reconciledWorktreeIdsForHost on whether this host's SSH target was actually connected at the client's last shutdown (activeConnectionIdsAtShutdown -- the same field use-app-startup-hydration.ts already reads to decide which targets to auto-reconnect at boot). A target still listed there was never disconnected; its host-partition mirror is this client's own current state written moments before quitting, not a stale pre-offline cache, so an absent base row for it was never evidence of anything and must not be read as "the server's confirmed zero." A target NOT listed there is the actual GAP-03 precondition, and still declines exactly as before. Verification of PR stablyai#968's 4 deferred pr-agent scenarios, RED-then-GREEN against this fix's own parent (36beb65f6b) and current HEAD, per-scenario: - Concurrent Active Edits: the achievable case (host not disconnected -- see the restart regression above) now adopts correctly. The harder case (a live sibling worktree while genuinely offline) is a DOCUMENTED, PROVEN architectural limitation: reconciledWorktreeIdsForHost gates per host, not per worktree, so a worktree the base has never cached is indistinguishable from one whose tabs were genuinely closed while offline. Both are safely parked (never deleted -- attachHostSessionShadow/partitionRowsTheWriteWontReturn), just not adopted this boot. Resolving this needs a live server signal (the upstream draft's own manifestEpoch/handshake proposal); this fix has none. - Split Layout Pruning: proven safe by construction -- GAP-03's decline is per-worktree across every field, not per-tab, so a declined worktree carries no rows in tabsByWorktree, unifiedTabs, tabGroups OR tabGroupLayouts. No orphaned split leaf can survive because nothing about that worktree is adopted at all. - Write Race Protection: CONFIRMED, REPRODUCIBLE, UNFIXED bug. A wake-triggered persistWorkspaceSessionByHost never touches a host partition with nothing else routed to it (no race). The instant anything else legitimately writes to that same host partition in the same call (a sibling worktree gaining a tab), attachHostSessionShadow unconditionally re-attaches EVERY parked row for that host -- including one GAP-03 just correctly declined -- and it round-trips back to the server. A naive "drop instead of park" mitigation was prototyped and reverted: it closes this race for a genuinely-closed worktree but is blind to the Concurrent-Active-Edits false positive, turning a recoverable non-adoption into an unrecoverable delete of another client's live tabs. Answers PR stablyai#968's deferred question directly: hasPendingReconnectionHandshake is NOT unnecessary -- it (or an equivalent live signal) is the only thing that would close this race without risking data loss. - Rapid Sleep/Wake Cycles: verified convergent and non-corrupting across 5 consecutive boot cycles against an unchanged on-disk profile, with no cumulative resurrection and no duplicate adoption. Also adds a real Docker cross-site e2e scenario (ordinary quit + relaunch, nothing closed, connection still active at shutdown) proving the restart regression fix end-to-end, not just at the unit level. New/changed tests: workspace-session-host-offline-reconnect.test.ts (+7), workspace-session-host-write-race.test.ts (new, 2), workspace-session-host- rapid-reconnect-cycles.test.ts (new, 2). All 115 pre-existing GAP-03 tests still pass unchanged; 124 total across the 11 touched files. No PR opened; stays on this pushed branch pending owner sign-off, per this slice's constraints (nothing goes upstream until fully verified).
…e 1) - Add remote-workspace-host-testimony.ts with RemoteWorkspaceTestimonyState and hostHasAnsweredForTarget - Add remote-workspace-host-testimony.test.ts covering unhydrated, conflict, and non-conflict synced states - Update workspace-terminal-host-authority.ts to compose RemoteWorkspaceTestimonyState and delegate to hostHasAnsweredForTarget (behavior-neutral)
- Add workspace-session-host-shadow-testimony.ts implementing shadowRowsTheHostHasNotAnswered - Export WORKTREE_KEYED_FIELDS from workspace-session-host-contention.ts - Extend HostPersistenceState with RemoteWorkspaceTestimonyState and wire shadow filtering into splitWorkspaceSessionForWrite in workspace-session-host-persistence.ts - Add write-race protection test, retitle existing test, and update docstring verdict in workspace-session-host-write-race.test.ts
… decline loop
CodeRabbit finding: the reconciled-decline loop in adoptStrandedHostPartitionSession
read host.tabsByWorktree[workspaceId] with the bare normalized workspaceId, but
tabsByWorktree keys may be stored in canonical worktree:${workspaceId} form (the
type's own doc comment says so). The adoption loop a few lines down already
normalizes every key before comparing; the decline loop didn't, so a canonical-keyed
host row silently defeated GAP-03's whole decline and adopted stale tabs it should
have declined.
Red test added first (workspace-session-host-offline-reconnect.test.ts): a host
partition with tabsByWorktree keyed as `worktree:${WORKTREE_ID}` under the
confirmed-disconnected-at-shutdown GAP-03 precondition. Failed before the fix
(both tabs survived, just under the canonical key instead of the bare one, so the
naive same-key assertion would have missed it -- the test walks every surviving
tab across every key). Fix: walk host.tabsByWorktree entries and normalize each
key via normalizeWorkspaceSessionKeyToWorkspaceId before matching against
workspaceId, matching the adoption loop's own pattern.
8/8 in the touched suite green; 75/75 across the 7 directly-dependent partition/
reconciliation suites green.
… worktree rows CodeRabbit finding: partitionRowsTheWriteWontReturn only walked WORKTREE_KEYED_FIELDS for a declined worktree, leaving tab-keyed (terminalLayoutsByTabId, remoteSessionIdsByTabId, localOnlyScrollbackByTabId) and pane-keyed (terminalPtyIncarnationsByPaneKey) rows unparked. If another workspace caused a write to the same SSH partition before host testimony arrived, the write retained the declined tabs but dropped their layout/session rows, leaving inconsistent state. Fix: 1. partitionRowsTheWriteWontReturn now maps each tab- and pane-keyed entry to its worktree via buildWorktreeIdByTabId / worktreeIdForPaneKey and parks rows whose owning worktree was declined. 2. attachHostSessionShadow and shadowRowsTheHostHasNotAnswered now iterate PARKABLE_HOST_SESSION_FIELDS (worktreeKeyed, tabKeyed, paneKeyed) so dependent rows are restored alongside tabsByWorktree before testimony and withheld after. 3. Added red-first regression test in workspace-session-host-write-race.test.ts proving terminalLayoutsByTabId and remoteSessionIdsByTabId survive the write.
…n activeConnectionIdsAtShutdown is undefined
CodeRabbit finding: when session.activeConnectionIdsAtShutdown is undefined
(older persisted sessions, interrupted shutdowns), hydration defaulted to an
empty set and passed false ("was disconnected") into reconciledWorktreeIdsForHost.
This caused every confirmed worktree on that host to be reconciled and declined,
dropping still-open tabs for sessions where shutdown status was merely unknown
rather than explicitly disconnected.
Fix:
1. wasTargetConnectedAtLastShutdown helper in workspace-session-host-hydration.ts:
when activeConnectionIdsAtShutdown is defined, check membership as before.
When undefined, check session.remoteSessionIdsByTabId for any surviving remote
PTY session belonging to targetId (matching the established pattern in
workspace-session-reconnect-targets.ts and ssh-pane-connect-gate.ts).
2. Added red-first regression test in workspace-session-host-offline-reconnect.test.ts
proving tabs survive when activeConnectionIdsAtShutdown is undefined but
remoteSessionIdsByTabId contains a session for the target.
9/9 in touched suite green, 55/55 across 5 related hydration/reconciliation suites green.
… live snapshot arrival CodeRabbit finding 3: an api.set/patch call generated while hostHasAnsweredForTarget was false (so declined tabs were attached by the shadow) could be delivered AFTER the live snapshot landed and testimony flipped true. Nothing ordered in-flight writes against the live snapshot's arrival, so a delayed pre-testimony write could overwrite the authoritative state and resurrect closed tabs. Fix: 1. enqueueHostPartitionWrite in workspace-session-host-persistence.ts serializes writes per host partition and tracks monotonic write generations. 2. When a write was prepared without host testimony, it checks right before dispatch whether testimony has since arrived or a newer write was dispatched for that host; if so, the stale pre-testimony write is discarded instead of overwriting the authoritative snapshot. 3. Added red-first regression test in workspace-session-host-write-race.test.ts proving a deferred pre-testimony write queued behind an in-flight write does not overwrite the live snapshot when testimony arrives before delivery. 55/55 tests across 5 related persistence/contention/write-race suites green.
…imony arrives Pullfrog finding 6: shadowRowsTheHostHasNotAnswered dropped ALL non-folder parked rows once hostHasAnsweredForTarget became true. Because direct-SSH snapshots only project terminal state (remote-workspace-snapshot-apply.ts:201), non-terminal state such as openFilesByWorktree editor drafts is never reported by or superseded by the SSH host snapshot. Withholding those non-terminal rows caused subsequent partition writes to erase offline-created editor drafts from disk. Fix: 1. Define SSH_TERMINAL_TESTIMONY_FIELDS in workspace-session-host-shadow-testimony.ts: only terminal-scoped fields that SSH snapshots actually manage are withheld by host testimony. Non-terminal fields like openFilesByWorktree survive testimony arrival. 2. Added unit test in workspace-session-host-shadow-testimony.test.ts verifying openFilesByWorktree survives even after testimony lands for that target. 3. Updated test comment in workspace-session-host-offline-reconnect.test.ts to accurately state the testimony boundary.
- Extract `enqueueHostPartitionWrite` and write chain state to `workspace-session-host-write-queue.ts` to bring `workspace-session-host-persistence.ts` within the 300-line budget (252 lines). - Deduplicate imports and add `SAFETY:` comments to all type assertions across touched files to satisfy `check-changed-code-quality` rules. - Add RED-first unit tests for `workspace-session-host-write-queue` verifying FIFO serialization, independent host concurrency, generation supersession, and test state reset. - Verified: 138/138 tests passing across all 13 session suites, typecheck clean, and 0 findings in `check-changed-code-quality`.
…hydration, and adoption - Guard host write generation advancement by replaceable/explicit-generation flag, preventing non-replaceable partial patches from superseding durable persists. - Support live testimony state resolution via getLiveState thunk and global provider in enqueueHostPartitionWrite to discard stale pre-testimony writes upon testimony arrival. - Normalize tab- and pane-keyed worktree ids in partitionRowsTheWriteWontReturn and resolve worktree claims in attachHostSessionShadow. - Preserve stablyai#12723 empty placeholder rows in base tabsByWorktree during stranded adoption. - Extract partitionRowsTheWriteWontReturn to workspace-session-stranded-partition-parking.ts to honor max-lines budget without disabling rules. - Guarantee Docker cleanup in e2e spec via nested finally blocks. - Approved by riley-pr-agent, riley-pr-check, and riley-the-maintainer.
…adow rows and clamp CLOSE_COUNT - Track whether host partition slices actually receive parked shadow rows in attachHostSessionShadow. - Propagate carriesParkedShadowRows through splitWorkspaceSessionForWrite to enqueueHostPartitionWrite. - Gate pre-testimony write discarding on carriesParkedShadowRows, preventing data loss for live local edits and delta patches. - Add regression test ensuring queued pre-testimony writes without parked shadow rows run when live testimony arrives. - Safely parse TAB_COUNT and clamp CLOSE_COUNT to TAB_COUNT in Docker e2e harness.
…ale docker runner env - Narrow SSH_TERMINAL_TESTIMONY_FIELDS in workspace-session-host-shadow-testimony.ts to terminal fields managed by direct-SSH snapshots, preserving client-only state such as localOnlyScrollbackByTabId. - Export isShadowRowSubjectToHostTestimony as a single shared authority for testimony invalidation. - Track hostsWithTestimonyInvalidatableShadow in attachHostSessionShadow, ensuring writes carrying only preserved shadow entries (folders, editor drafts) are not discarded upon testimony arrival. - Add required typescript(consistent-type-assertions) safety comment on splitWorkspaceSessionForWrite call in patchWorkspaceSessionByHost. - Remove dead ORCA_GAP03_CLOSE_COUNT forwarding in config/scripts/run-offline-reconnect-tab-resurrection-docker.mjs.
…imony modules - Move PARKABLE_HOST_SESSION_FIELDS to src/shared/workspace-session-host-field-ownership.ts alongside other ownership field sets. - Re-export PARKABLE_HOST_SESSION_FIELDS from workspace-session-host-contention.ts for backwards compatibility. - Import PARKABLE_HOST_SESSION_FIELDS directly from shared in workspace-session-host-shadow-testimony.ts and merge duplicate imports. - Satisfies oxlint audit:code-quality:native with deny-warnings.
… tabs Primary setForConnectedTargets omitted session, so main persistedSessionForTarget re-adopted stale ssh-partition tabs without reconciledWorktreeIds. Pass the renderer session on upload and apply the same confirmed+shutdown decline on the main fallback; regression covers omit-session offline vs connected restart.
Pre-testimony write discard is opt-in: only callers that know the write carries parked shadow rows pass true (persistence uses hostsWithTestimonyInvalidatableShadow). Default false keeps ordinary patches from being dropped when live testimony lands mid-flight. Also: GAP-03 decline reads originalBase for hasBaseKey so main's ownedSessionKeys superseding of empty stablyai#12723 placeholders cannot false-trigger offline decline; export GAP-03 tests use createSqliteTestStore.
3f96782 to
5a89d75
Compare
Parked unifiedTabs/tabGroups/tabGroupLayouts/activeGroupIdByWorktree must drop from the write shadow once the host has answered, same as declined tabs — else post-testimony persist restores ghost split chrome with no tabs.
Precompute base tabsByWorktree workspace ids and normalize reconciled ids before the offline decline loop. Drops the worktree: dual-key scan and the unused hydration re-export of wasTargetConnectedAtLastShutdown.
This branch has not been deployed
1 participant
ELI5
When your Orca desktop was closed or offline and someone (or an automation) closed a terminal tab on the server side while you were away, reopening Orca used to bring that closed tab right back — a "zombie tab" that shouldn't exist anymore. This PR stops that on boot and relaunch.
What Changed
Before: on reconnect/relaunch, a paired desktop client's remote-workspace hydration (
fetchWorkspaceSessionWithRuntimeHostOwnersinworkspace-session-host-hydration.ts) would blindly re-adopt every worktree row still present in the SSH host-partition mirror (workspaceSessionsByHostId['ssh:<targetId>']), even when the client's own base session (tabsByWorktree) showed that worktree's tabs had been closed while the client was offline. The closed tabs came back.After: hydration now distinguishes "this SSH target was genuinely offline at last shutdown, and the base has no row for a worktree the host mirror remembers as populated" (decline — don't resurrect) from "this target was still connected at last shutdown" (an ordinary restart — keep everything, nothing was ever offline). The gate uses
session.activeConnectionIdsAtShutdown, the same fielduse-app-startup-hydration.tsalready reads to decide which targets to auto-reconnect at boot.The fix landed across structured TDD slices, hardened against 6 peer/bot review findings:
129d11ad4d) — stop reconnect from re-adopting server-closed tabs by declining a worktree whose base row is absent for a genuinely-offline host.5cb758d791) — the base fix as first written declined every confirmed worktree on every boot, not just genuinely-offline ones, because an SSH-hosted worktree'stabsByWorktreerow never routes to thelocalbase in the first place (hostIdByWorktreeIdalways sends it tossh:<targetId>) — so "absent onlocal" was always true, closed or not. This slice gates the decline onactiveConnectionIdsAtShutdown: a target still listed there was never disconnected, so its host-partition mirror is the client's own current state, not a stale cache, and its tabs are kept.44b8939c7b,6e8671fe83,be4c61de4a) — a declined worktree's row is never deleted, only parked incontestedHostWorkspaceSessions[hostId](the write-side shadow), so it stays recoverable via a live reconnect. This slice (workspace-session-host-shadow-testimony.ts,shadowRowsTheHostHasNotAnswered) closes a confirmed write race where a parked row could be clobbered before the host's live answer for that target actually landed, by withholding answered parked rows fromsplitWorkspaceSessionForWriteoncehostHasAnsweredForTargetconfirms the host snapshot landed without conflict.3df67bfc15) — the decline loop now normalizes canonicalworktree:${workspaceId}keys inhost.tabsByWorktreevianormalizeWorkspaceSessionKeyToWorkspaceId, ensuring canonical-keyed host entries cannot bypass the decline.1910a0badd) —partitionRowsTheWriteWontReturnnow walks tab- and pane-keyed rows (terminalLayoutsByTabId,remoteSessionIdsByTabId,localOnlyScrollbackByTabId,terminalPtyIncarnationsByPaneKey), mapping them back to their worktree so a declined worktree's layout and PTY rows are parked and restored alongside itstabsByWorktreerow.7f73b45159) — whenactiveConnectionIdsAtShutdownis undefined (older sessions, interrupted shutdowns),wasTargetConnectedAtLastShutdowninspectsremoteSessionIdsByTabIdfor active remote PTY sessions on that target instead of blindly assuming disconnected.0f475182aa) —enqueueHostPartitionWriteinworkspace-session-host-persistence.tsserializes partition writes per host and checks testimony upon dispatch, discarding stale pre-testimony writes if testimony has landed before delivery.98126f018f) — direct-SSH snapshots project terminal state only (remote-workspace-snapshot-apply.ts:201). DefinedSSH_TERMINAL_TESTIMONY_FIELDSso host testimony only withholds terminal-scoped fields; non-terminal state such asopenFilesByWorktreeeditor drafts is never erased by terminal testimony.Why
Fixes the client-side symptom from canonical issue #21341 (duplicate #22038): a closed tab that reappears on the next desktop relaunch because the client's cached SSH host-partition mirror in
orca-data.jsonwas never invalidated against what the server actually still has open.Linked Issue & Scope Boundary
Addresses #21341 (Client-side hydration reconciliation & write-race protection; complements host-side tombstone gate in PR #22149). Note: duplicate issue #22038 flagged for closure.
Scope boundary & live wake-from-sleep vs boot-hydration coverage (Finding 5 resolution):
fetchWorkspaceSessionWithRuntimeHostOwners), where offline paired desktop clients retain stale tabs in on-diskorca-data.jsonand re-inject them into the merged session viaadoptStrandedHostPartitionSession.remote-workspace-session-merge.ts:194-210(applyDirectSshRemoteWorkspaceSnapshot), where thehostUnknownbranch intentionally preserves local tabs absent from the fresh host snapshot. This is a pinned, deliberate architectural trade (remote-workspace-session-merge-local-survival.test.ts:93-107) to ensure that tabs created locally while offline or with pending uploads are not destroyed on flaky reconnects ("leak, never kill"). Live-wake synchronization without relaunch is tracked separately.Scope breakdown against #21341's 6-scenario matrix:
workspace-session-host-hydration.ts(fetchWorkspaceSessionWithRuntimeHostOwners) now gates adoption onactiveConnectionIdsAtShutdown(with fallback toremoteSessionIdsByTabId), declining stale host-partition mirror tabs when the host was genuinely offline.workspace-session-host-shadow-testimony.ts(shadowRowsTheHostHasNotAnswered) withholds answered parked shadow rows oncehostHasAnsweredForTargetconfirms live sync, andenqueueHostPartitionWriteserializes writes to prevent deferred writes from overwriting live snapshots.contestedHostWorkspaceSessionsrather than resurrecting or clobbering them.resolveHintedTerminalPaneIdentityrefusing retired pane hints withtab_not_found). Both PRs together provide full end-to-end idempotency across the client-server boundary.Related Work & Integrated Ecosystem Tracking
This work is part of an integrated, cross-layer stabilization effort addressing the cascading failure chain: Host PTY descriptor leaks (Stage 1) → Transport latency & heartbeat stalls (Stage 2) → Client session desynchronization & ghost tab resurrection (Stage 3).
Stage 1: Host PTY & Daemon Process Lifecycle (Resource Starvation)
fix(terminal): sweep leftover processes when a PTY exits on its own(OPEN, @brennanb2025). Natural-exit descendant sweeper targeting recorded process groups and terminal handles when shells or background agents exit autonomously.feat(daemon): reap PTY processes left behind when their owner is gone(OPEN, @brennanb2025). Daemon-level durable ownership store (daemon-v<N>.pty-owners.json) and periodic reconciler to SIGTERM/SIGKILL processes orphaned across daemon crashes or restarts.Host daemon restart re-hydrates dead PTY layout records without OS liveness check(OPEN, @LesleyMurfin). Tracks the failure where revived daemons restore checkpointed PTY IDs without PID/liveness verification, advertising dead handles to connecting clients.Headless server CLI terminal close fails across remote execution host boundary(OPEN, @LesleyMurfin). Cross-host CLI dispatch boundary whereorca terminal closefails to forward RPCs to remote daemons, stranding un-reaped PTY process trees on Linux servers.Pre-v31 attach-only accidental spawn leave orphan if kill fails(OPEN, @AmethystLiang). Tracks untracked orphan shell leaks on daemon hosts when attach-only kills fail silently.Session scanner OOMs on large Codex transcripts because buffered slices retain parent strings(OPEN, @sirambrosio). Retained string slices exhaust daemon heap (OOM crash), triggering daemon restarts and sudden connection drops.Stage 2: Transport Layer, Heartbeats & RPC Multiplexing (Event-Loop Starvation)
fix(runtime-rpc): don't reap a socket while its request is still running(OPEN, @Jinwoo-H). Pauses the 30-second socket idle timer while runtime requests are actively executing, preventing false disconnects on long-running operations.fix(runtime-rpc): answer over-limit local connections with runtime_busy instead of dropping them(OPEN, @Jinwoo-H). Eliminates silent socket drops under burst concurrency (>32 connections) with typedruntime_busybackpressure and client-side retry backoff.remote-runtime-pty-transport attach() latches in connecting state forever on terminal_not_found(OPEN, @LesleyMurfin). Fixes attachment state machine permanently locking inconnecting = truewhen the host returnsterminal_not_found, wedging UI with an infinite spinner.Paired-parked PTY reattach is gated on a per-host capability, never on whether the host still holds that session(OPEN, @nwparker). Eliminates blind reattachment based solely on static host capabilities without validating individual session retention.Two remaining single-snapshot absence inferences in remote-runtime-pty-transport(OPEN, @YgorLeal). Prevents client-side transport from falsely marking live split panes as dead based on a single missing snapshot.Remote client pane stays blank forever while still driving host pty geometry(OPEN, @tian2-ai). Fixes geometry-stream desynchronization where remote PTY panes latch in a blank state after reconnect.Rehydrated Antigravity (agy) session freezes and rejects keyboard input over Remote Server after idle(OPEN, @dbehdgns1215). Reconnection input trapping across remote server idle intervals.Stage 3: Client Workspace Hydration, Split Trees & Tab Reconciler (Zombie Resurrections)
Offline paired desktop clients retain stale tabs in orca-data.json and re-inject them upon reconnect(OPEN, @LesleyMurfin). The canonical bug tracking client-side resurrection of server-closed tabs across offline sleep/wake intervals.fix(GAP-03): stop offline-client reconnect from resurrecting server-closed tabs(OPEN, @LesleyMurfin). Client-side reconciliation fix gating adoption onactiveConnectionIdsAtShutdownwith shadow testimony write-race fencing.fix(runtime): refuse a terminal.create tab id hint the host already retired(OPEN, @BearMett). Complements fix(GAP-03): stop offline-client reconnect from resurrecting server-closed tabs #22334 by adding a host-side tombstone gate inresolveHintedTerminalPaneIdentitythat returnstab_not_found.serve/headless: closed workspace tabs resurrect on workspace re-entry — orphan PTYs re-adopted, persisted tabs rehydrated without tombstone check(OPEN, @Shujakuinkuraudo). Demonstrates how un-reaped host PTYs get re-adopted as new tabs and persistent stores rehydrate without tombstones on mobile/paired clients.Paired client keeps a mirrored terminal tab for days after the host closed it(OPEN, @yulin0629). Primary field observation of zombie mirrored tabs surviving host closure.claude --resume) by another client on workspace selection #21235 —A tab closed on one paired client is auto-resumed (claude --resume) by another client on workspace selection(OPEN, @yulin0629). Cross-device state pollution where closing a tab on Client A resurrects on Client B upon workspace activation.shouldReplaceTerminalTab retains ptyId: null placeholders indefinitely without TTL bound(OPEN, @LesleyMurfin). Eliminates infinite retention of null-PTY placeholders inapplyPreparationBase, preventing ghost tab accumulation (170+ tabs) and V8 heap OOM crashes.Dismissing 'Remote terminal was closed' toast clears alert banner without tearing down dead pane(OPEN, @LesleyMurfin). Resolves zombie split leaves persisting in the DOM after error toast dismissal by triggering container teardown.Remote workspace reconciliation: concurrent active edits during a genuine offline window are parked, not resolved(OPEN, @LesleyMurfin). Documents multi-client offline reconciliation boundaries and the need for an authoritative live reconnection handshake.fix(workspace): a restored workspace paints before its terminals reconnect(OPEN, @brennanb2025). Decouples surface painting from the terminal PTY reconnect ladder so editor and chat panes render immediately without blank screens.fix(session): scope agent resume to the host that captured the session(MERGED, @nwparker). Scopes agent session resume keys by host partition to stop cross-host transcript collision.Parked remote terminal scrollback is not persisted across a client restart(MERGED, @nwparker). Fixes hot debounced patch routing for parked terminal buffers.Local (non-SSH) terminal sessions accumulate indefinitely — close tombstones only recorded for SSH connectionId workspaces(OPEN, @tain-alphanume). Highlights the local/non-SSH persistence gap where tombstone recording was omitted, leading to monotonic zombie tab growth inorca-data.json.Visual Proof
N/A — this is a session-persistence/reconciliation bug with no UI surface; the observable effect is which terminal tabs are present after a reconnect, not their rendering. Covered by an automated Docker e2e harness that runs two real Electron launches with an actual SSH-connected container instead of a screenshot (see Testing below).
Testing
Unit (Linux,
pnpm test/vitest --config config/vitest.config.ts): 180/180 passing across 17 test suites, including:workspace-session-host-offline-reconnect.test.ts— core resurrection fix, ordinary-restart regression fix, canonical key normalization, and unknown shutdown status inference.workspace-session-host-shadow-testimony.test.ts,workspace-session-host-write-race.test.ts,remote-workspace-host-testimony.test.ts— write-race protection, deferred write ordering, dependent row parking, and non-terminal shadow row survival.workspace-session-host-rapid-reconnect-cycles.test.ts— 5 consecutive sleep/wake boot cycles against an unchanged on-disk profile never resurrect a declined tab or duplicate an adoption.workspace-session-ssh-partition-ownership.test.ts,workspace-session-ssh-partition-round-trip.test.ts,workspace-session-host-split.test.ts,workspace-session-host-contention.test.ts,workspace-terminal-host-authority.test.ts,ssh-host-partition-session-export.test.ts,workspace-session-partition-owner.test.ts,remote-workspace-session-merge-*.test.ts— full baseline regression coverage.Live Docker e2e (
tests/e2e/gap-03-offline-reconnect-tab-resurrection.spec.ts, real two-launch Electron test against a Dockerized SSH target): 2/2 passing (1.5m) —orca-data.json, relaunch confirms exactly the 2 survivors, 0 closed tabs resurrect, offline draft survives.Static checks:
pnpm run typecheck(tsc -p config/tsconfig.web.json) andoxlintboth 100% clean.Platforms actually tested: Linux (Docker container as the SSH-remote target; host process is the standard Electron/Playwright headless target used by this repo's other e2e specs).
AI Disclosure
Review
Investigation and fix authored with AI assistance (Claude-based agent tooling) under human review and direction; every fix decision and the write-race defect described above were driven by targeted, reproducible before/after test evidence rather than inferred from the upstream issue's proposed design.
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.This change touches only remote-workspace session persistence/reconciliation and their tests; it does not touch skill-installer or skill-sharing code.
Notes
orca-data.jsonschema.activeConnectionIdsAtShutdowndoes not list the target), and this client's local base has never cached that worktree, the current per-host gate (not per-worktree) cannot distinguish "genuinely closed while offline" from "never synced." The safe fallback applies: the row is neither resurrected nor deleted, it's parked incontestedHostWorkspaceSessions[hostId]until a live SSH reconnect for that target lands through a separate code path this fix doesn't touch. This is a proven-safe, "park it, never guess" behavior, not silent data loss or inconsistency.Checklist
N/Awith reasonpnpm lint,pnpm typecheck, andpnpm testpass locally (verified: 180/180 unit tests, clean typecheck, clean lint);pnpm buildnot run locally for this change, CI covers it