Repository navigation
fix(session): read ssh host partitions so stranded tabs stop round-tripping as deletions - #12722
robertnisipeanu wants to merge 23 commits into
Conversation
|
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⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds Direct SSH authority tracking to remote workspace caching, notifications, reads, writes, retries, and explicit session export. It adds SSH workspace-session partition adoption with collision detection, provenance reconciliation, durable persistence ordering, rollback, and IPC/preload APIs. Renderer synchronization now validates worktree, tab, PTY, layout, and remote-session ownership before hydration. Tests and reliability gates cover authority rotation, partition recovery, duplicate ownership, persistence failures, PTY recovery, and application restarts. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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: 1
🧹 Nitpick comments (1)
src/main/ipc/remote-workspace.ts (1)
427-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the new rationale comment.
Lines 427-437 explain the full recovery flow in eleven lines. Keep only the ownership rationale. The adoption helper already documents tombstone behavior.
As per coding guidelines, “Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through how the code works.”
Proposed change
- // Why: the local-partition fallback cannot see session state the - // runtime persisted into the target's ssh partition; exporting - // without it round-trips those worktrees as empty, and the next - // remote-authoritative pull deletes their tabs on every client. - // Adoption here is deliberately read-only: the renderer owns the - // local partition and replaces its worktree map wholesale on every - // debounced write, so a store-side move would be silently undone — - // with the only durable copy already pruned. The move happens at - // boot hydration, where the adopted tabs enter the renderer's own - // state; deliberate closes are fenced by surface tombstones, which - // adoption honors. + // Adopt SSH state for export only; renderer hydration owns persistence.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 59f14b44-f70a-4f8c-b66a-3bfa6c4a19a1
📥 Commits
Reviewing files that changed from the base of the PR and between eea0bb6 and a7907687c4659b616b41c16deb3f5f8e231bb142.
📒 Files selected for processing (7)
src/main/ipc/remote-workspace.test.tssrc/main/ipc/remote-workspace.tssrc/renderer/src/lib/workspace-session-host-persistence.test.tssrc/renderer/src/lib/workspace-session-host-persistence.tssrc/renderer/src/lib/workspace-session-ssh-partition-hydration.tssrc/shared/workspace-session-partition-adoption.test.tssrc/shared/workspace-session-partition-adoption.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/lib/workspace-session-ssh-partition-hydration.ts (1)
91-99: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRetry source cleanup after a successful owner write.
If the source reread or source patch fails after Line 91, the owner keeps the adopted tabs and the SSH partition keeps its copy. On later runs, the populated owner makes
ownerPatchnull, so Lines 92-99 never retry cleanup. If the owner later becomes empty without a corresponding tombstone, the retained SSH tabs can be adopted again.Persist pending cleanup state, or reconcile and prune only source tab IDs already present in the owner on later reads. Add coverage for a failed source prune followed by a successful later recovery.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d918b4b8-fa93-4fc0-9983-5ae56ca43b86
📥 Commits
Reviewing files that changed from the base of the PR and between a7907687c4659b616b41c16deb3f5f8e231bb142 and bfe2e1279c97b1c0b73bf93dab74ae9974fe7ae6.
📒 Files selected for processing (4)
src/renderer/src/lib/workspace-session-host-persistence.test.tssrc/renderer/src/lib/workspace-session-ssh-partition-hydration.tssrc/shared/workspace-session-partition-adoption.test.tssrc/shared/workspace-session-partition-adoption.ts
…ipping as deletions Main-process runtime flows persist SSH-owned worktree session state into ssh:<targetId> partitions (workspaceSessionsByHostId), while renderer persistence keeps SSH worktrees in the local partition. Neither boot hydration (local + runtime:* partitions only) nor the remote-workspace export fallback (local partition only) ever reads an ssh: partition, so a worktree whose only populated tab list lives there renders empty at boot and is uploaded to the target's remote workspace snapshot as an empty tab list. The remote-authoritative pull merge then deletes those tabs on every client — permanently, on every launch, since the emptied state is pushed right back. Fix, on the read side only (no writer changes): - shared/workspace-session-partition-adoption.ts: adoption of orphaned partition entries — a worktree whose base session holds no tabs takes the partition's populated entry plus the tab-scoped records of exactly the adopted tabs. Populated base entries always win; tabs fenced by a surface tombstone on either side stay retired; nothing is ever removed from the base. - Renderer boot hydration reads the ssh partitions of known SSH repos and completes each adoption as a move: the local partition durably gains every adopted field (patch built from a fresh read, awaited) before the source partition sheds the adopted entries, so tabs the user later closes cannot be re-adopted. - The remoteWorkspace:setForConnectedTargets fallback adopts per target before exporting, read-only: the renderer owns the local partition and replaces its worktree map wholesale on every debounced write, so a store-side move here would be silently undone with the only durable copy already pruned. The move belongs to boot hydration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ll owns Session patches replace whole record fields, so the adoption move has to decide what it may overwrite from the partitions' current contents rather than from the boot snapshot it started with. - Boot hydration reads the owner partition before adopting and folds it into the base, taking both its entries and its surface tombstones. Adoption's base-wins rule then leaves a worktree the owner repopulated alone, and a pane retired since the snapshot still fences its tab out. - buildAdoptedWorkspaceSessionOwnerPatch drops every adopted worktree the owner already populates and reports the tab ids it does land, so the hydrated session and the owner partition agree on what moved. - The source partition sheds exactly those landed tabs plus the ones a tombstone retired. A tab written to an adopted worktree after the adoption read stays behind instead of going down with the whole entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfe2e12 to
b0785b0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/reliability-gates.jsonc (1)
3-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBump
updatedAtto match the newest evidence date.The file's top-level
updatedAtis"2026-08-09"(Line 3). The new evidence run added in this diff (Lines 6409-6417) is dated"2026-08-10", which is later thanupdatedAt. UpdateupdatedAtto"2026-08-10"(or later) so the manifest's own timestamp stays truthful relative to the evidence it records.🗓️ Proposed fix
- "updatedAt": "2026-08-09", + "updatedAt": "2026-08-10",Also applies to: 6409-6417
🧹 Nitpick comments (12)
src/renderer/src/hooks/remote-workspace-target-sync.test.ts (1)
215-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
sessionAuthorityin the upload expectation.
syncAfterConnectnow sendssessionAuthorityin addition tosessionTargetId.expect.objectContainingignores the missing field, so a regression that drops the authority from the upload would still pass this test. The authority is the field the main process uses to reject foreign or stale writes.♻️ Proposed test change
expect.objectContaining({ sessionTargetId: 'target-a', + sessionAuthority: owner, hydratedTargetIds: ['target-a'] })src/renderer/src/hooks/remote-workspace-session-merge.ts (1)
138-147: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for dead-tab generation precedence. A retry generation has a matching exact-authority recovery entry, so the branch preserves it while recovery is active. A dead tab without recovery authority intentionally adopts
remoteTab, even when its generation is lower. Test both cases and name the condition predicates.src/main/ipc/remote-workspace.ts (1)
110-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider delegating the test helpers to the production cache functions.
_rememberRemoteWorkspaceSnapshotForTestsrepeats the body ofrememberRemoteWorkspaceSnapshot, including the eviction loop._getRemoteWorkspaceSnapshotForTestsrepeatsgetCachedRemoteWorkspaceSnapshot, but it omits the invalidation delete on authority mismatch. The cache tests therefore assert against a second implementation that can drift from the production one.Extract the shared insert and lookup logic, then let both the production functions and the test helpers call it with the currency check as a parameter.
src/shared/workspace-session-partition-authority.ts (2)
18-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a code-unit comparison for the canonical key sort.
localeComparedepends on the ICU collation of the running runtime. The key order determines the serialized signature. A plain code-unit comparison keeps the signature stable across runtimes and avoids locale-dependent ordering for keys that differ only by case or accent.♻️ Proposed change
return Object.fromEntries( - Object.entries(entry as Record<string, unknown>).sort(([left], [right]) => - left.localeCompare(right) - ) + Object.entries(entry as Record<string, unknown>).sort(([left], [right]) => + left < right ? -1 : left > right ? 1 : 0 + ) )
56-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
emptyWorkspaceBundle.The function returns a bundle that already contains
workspaceRecordsfor the given key. Only the tab-scoped fields start empty. A name such asworkspaceRecordBundledescribes the return value.src/preload/api-types.ts (1)
2746-2746: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the omitted-
hostIdbehavior.
adoptSshPartitionaccepts an optionalhostId. The signature does not state what main does when the caller omits it. Neighbouring members in this block carry a short comment for the same reason (see thehostIdnote aboveget). Add one line that states whether main adopts every SSH partition or none.src/renderer/src/web/web-preload-api.ts (1)
793-793: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a one-line reason for the no-op.
The browser fallback has no SSH partitions, so returning the local session is correct. Neighbouring members in this object state the reason for a divergence from the desktop bridge (lines 779 and 794). Add the same for this member.
♻️ Proposed change
+ // No SSH partitions exist in the browser fallback, so adoption returns the local session unchanged. adoptSshPartition: async () => getStoredWorkspaceSession(),src/renderer/src/lib/workspace-session-ssh-partition-hydration.ts (1)
37-44: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRead the host partitions in parallel.
Each
api.get(hostId)is a separate IPC round trip awaited in sequence. This adds latency to the boot path in proportion to the number of SSH targets.Promise.allpreserves the array order, so the adoption fold order stays the same, and the per-host catch keeps the skip-on-error behavior.♻️ Proposed change
- const sources: WorkspaceSessionState[] = [] - for (const hostId of listKnownSshHostIds(repos)) { - try { - sources.push(await api.get(hostId)) - } catch (err) { - console.warn(`[session] skipping unreadable host partition ${hostId}:`, err) - } - } + const settled = await Promise.all( + listKnownSshHostIds(repos).map(async (hostId) => { + try { + return await api.get(hostId) + } catch (err) { + console.warn(`[session] skipping unreadable host partition ${hostId}:`, err) + return null + } + }) + ) + const sources = settled.filter((source): source is WorkspaceSessionState => source !== null)src/main/persistence.ts (1)
6298-6306: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a locale-independent comparison to order the SSH partitions.
localeCompareresolves through ICU collation and can order host ids differently on macOS, Linux, and Windows, or under a different default locale. Adoption applies the sources in this order, so the ordering must be stable across platforms.♻️ Proposed deterministic ordering
- .sort(([left], [right]) => left.localeCompare(right)) as [string, WorkspaceSessionState][] + .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0)) as [ + string, + WorkspaceSessionState + ][]As per coding guidelines: "commands and scripts must work on macOS, Linux, and Windows."
Source: Coding guidelines
src/renderer/src/lib/workspace-session-host-persistence.ts (1)
318-342: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSkip the redundant local read when
adoptSshPartitionis available.Line 319 awaits
api.get()and stores it as the local slice. Line 341 then overwrites that slice with the adoption result. Whenapi.adoptSshPartitionexists, the first value is never read, so boot performs one extra IPC round-trip that returns the full session payload.♻️ Proposed change
- const slices: HostSessionSlices = { - [LOCAL_EXECUTION_HOST_ID]: await api.get() - } + // Main adoption returns the local owner, so skip the plain read on that path. + const slices: HostSessionSlices = { + [LOCAL_EXECUTION_HOST_ID]: await (api.adoptSshPartition + ? api.adoptSshPartition() + : api.get()) + }Then drop the
if (api.adoptSshPartition)block at lines 339-342.src/renderer/src/lib/workspace-session-host-persistence.test.ts (2)
194-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the new
sessionTabhelper for these tab literals.
strandedTabandliveTabrepeat the same nine fields the helper at lines 16-27 already produces. Onlyid,title, andcreatedAtdiffer.Line 208 also casts the layout snapshot with
as never. That cast accepts any shape, so a future change toTerminalLayoutSnapshotwill not surface here.♻️ Proposed change
- const strandedTab = { - id: 'stranded-tab', - ptyId: null, - worktreeId, - title: 'Stranded', - customTitle: null, - color: null, - sortOrder: 0, - createdAt: 1 - } + const strandedTab = sessionTab('stranded-tab', worktreeId)Apply the same substitution for
liveTabat lines 258-267, and replaceas neverwith an explicitTerminalLayoutSnapshotliteral.Also applies to: 258-267
326-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
hostIdbranch from the mock.The assertion at line 342 proves
adoptSshPartitionreceives no arguments, sohostIdis alwaysundefinedand thehostId === 'ssh:conn-1'branch never runs. The branch suggests the test exercises per-host adoption, which it does not.♻️ Proposed change
- const adoptSshPartition = vi.fn( - async (hostId?: `ssh:${string}`): Promise<WorkspaceSessionState> => { - return { - ...getDefaultWorkspaceSession(), - tabsByWorktree: - hostId === 'ssh:conn-1' - ? { [worktreeA]: [tabA] } - : { [worktreeA]: [tabA], [worktreeB]: [tabB] } - } - } - ) + // Main enumerates every persisted SSH partition in one call. + const adoptSshPartition = vi.fn().mockResolvedValue({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: { [worktreeA]: [tabA], [worktreeB]: [tabB] } + })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e61b276-25bf-4e9f-9a32-16cf6852967c
📒 Files selected for processing (31)
config/reliability-gates.jsoncsrc/main/ipc/remote-workspace-cache.test.tssrc/main/ipc/remote-workspace-events.tssrc/main/ipc/remote-workspace-explicit-session-authority.tssrc/main/ipc/remote-workspace-patch-authority.test.tssrc/main/ipc/remote-workspace-patch-queue.test.tssrc/main/ipc/remote-workspace.test.tssrc/main/ipc/remote-workspace.tssrc/main/ipc/session.test.tssrc/main/ipc/session.tssrc/main/persistence.test.tssrc/main/persistence.tssrc/main/ssh/ssh-relay-session.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/hooks/remote-workspace-session-merge.test.tssrc/renderer/src/hooks/remote-workspace-session-merge.tssrc/renderer/src/hooks/remote-workspace-snapshot-apply.tssrc/renderer/src/hooks/remote-workspace-snapshot-duplicate-tab-repair.test.tssrc/renderer/src/hooks/remote-workspace-target-sync.test.tssrc/renderer/src/hooks/remote-workspace-target-sync.tssrc/renderer/src/lib/workspace-session-host-persistence.test.tssrc/renderer/src/lib/workspace-session-host-persistence.tssrc/renderer/src/lib/workspace-session-ssh-partition-hydration.tssrc/renderer/src/web/web-preload-api.tssrc/shared/workspace-session-partition-adoption.test.tssrc/shared/workspace-session-partition-adoption.tssrc/shared/workspace-session-partition-authority.tssrc/shared/workspace-session-partition-provenance.tssrc/shared/workspace-session-partition-state-merge.tstests/e2e/ssh-session-partition-adoption.spec.ts
| if (!merged) { | ||
| state.setRemoteWorkspaceSyncStatus(authority.targetId, { | ||
| phase: 'conflict', | ||
| direction: 'pull', | ||
| revision: snapshot.revision, | ||
| updatedAt: snapshot.updatedAt, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Re-check arrival and token currency before you write the conflict status.
waitForWorkspaceSessionReady() at Line 103 is awaited. After it resolves, this path writes the conflict status without re-checking isArrivalCurrent or isPreparationTokenCurrent. The success path re-checks at Line 143, and the timeout path re-checks at Line 104. A superseded snapshot can therefore overwrite the status of a newer arrival with a stale conflict entry.
🛡️ Proposed fix
if (!merged) {
+ if (!isArrivalCurrent(authority.targetId, arrival) || !isPreparationTokenCurrent(token)) {
+ return
+ }
state.setRemoteWorkspaceSyncStatus(authority.targetId, {
phase: 'conflict',
direction: 'pull',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!merged) { | |
| state.setRemoteWorkspaceSyncStatus(authority.targetId, { | |
| phase: 'conflict', | |
| direction: 'pull', | |
| revision: snapshot.revision, | |
| updatedAt: snapshot.updatedAt, | |
| if (!merged) { | |
| if (!isArrivalCurrent(authority.targetId, arrival) || !isPreparationTokenCurrent(token)) { | |
| return | |
| } | |
| state.setRemoteWorkspaceSyncStatus(authority.targetId, { | |
| phase: 'conflict', | |
| direction: 'pull', | |
| revision: snapshot.revision, | |
| updatedAt: snapshot.updatedAt, |
| /** Recover SSH tabs through the main process's atomic partition move. */ | ||
| export async function adoptStrandedSshHostPartitions( | ||
| api: SshPartitionHydrationApi, | ||
| repos: readonly Pick<Repo, 'connectionId' | 'executionHostId'>[], | ||
| merged: WorkspaceSessionState | ||
| ): Promise<WorkspaceSessionState> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the doc comment.
The comment states that recovery happens "through the main process's atomic partition move". This function does not perform a move. It reads each SSH partition with api.get and merges the result in the renderer. SshPartitionHydrationApi exposes only get. The durable move lives behind session.adoptSshPartition. The current wording implies that this call prunes the source partition, which it does not.
📝 Proposed change
-/** Recover SSH tabs through the main process's atomic partition move. */
+/** Fold stranded SSH host partitions into the merged boot session. Read-only; pruning happens in main. */
export async function adoptStrandedSshHostPartitions(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Recover SSH tabs through the main process's atomic partition move. */ | |
| export async function adoptStrandedSshHostPartitions( | |
| api: SshPartitionHydrationApi, | |
| repos: readonly Pick<Repo, 'connectionId' | 'executionHostId'>[], | |
| merged: WorkspaceSessionState | |
| ): Promise<WorkspaceSessionState> { | |
| /** Fold stranded SSH host partitions into the merged boot session. Read-only; pruning happens in main. */ | |
| export async function adoptStrandedSshHostPartitions( | |
| api: SshPartitionHydrationApi, | |
| repos: readonly Pick<Repo, 'connectionId' | 'executionHostId'>[], | |
| merged: WorkspaceSessionState | |
| ): Promise<WorkspaceSessionState> { |
…anes A topology revision is only recorded in a partition by the process that runs the topology-authority code path, so a partition can hold live terminal state without ever receiving a bump. workspaceTerminalAuthority treated any recorded revision as newer than an unrecorded one, which let a partition of dormant tab rows win authority over the partition holding the running terminals: the adopted view then dropped the live tabs, their agent sessions failed every ownership check at the next cold start, and cold restore relaunched them as duplicates while the original processes kept running unreachable. The one-sided comparison now yields to the partition holding pty-bound panes when the recorded side has none, and a pty-bound tiebreak resolves the no-revision case before bundle equivalence. Two-sided comparisons, presence, and fail-closed ambiguity are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqNqAZQTwmAkwGcHj5TYXk
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqNqAZQTwmAkwGcHj5TYXk
…ption-liveness # Conflicts: # src/main/ipc/remote-workspace.test.ts # src/main/ipc/remote-workspace.ts # src/preload/api-types.ts # src/renderer/src/hooks/remote-workspace-session-merge.ts # src/renderer/src/lib/workspace-session-host-persistence.test.ts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqNqAZQTwmAkwGcHj5TYXk
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqNqAZQTwmAkwGcHj5TYXk
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqNqAZQTwmAkwGcHj5TYXk
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqNqAZQTwmAkwGcHj5TYXk
|
Pushed three additions on top of the rework:
Known limitation, pinned in the tests: a closed tab whose stale copy in the losing partition is still pty-bound is indistinguishable in the persisted data from a live stray, and grafts back. Telling them apart needs a per-tab close signal; the tab-mutation ledger in #13013 looks like the natural future fence. Field data behind the veto: a local partition with ten dormant rows and revision 162 against an ssh host partition holding the workspace's only two live, pty-bound tabs and no revision map — every cold boot of that store duplicated both running agent sessions. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/shared/workspace-session-partition-adoption.ts (1)
93-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting the reconciliation phases into named functions.
adoptOrphanedWorkspaceSessionPartitionruns eleven sequential phases over shared mutable locals: tab merge, graft, pty grafting, layouts and remote sessions, incarnations, tombstones, retirement, and sleeping agents. Each phase reads state that a previous phase mutated, so the ordering is an implicit contract.Extract each phase into a named function that takes and returns explicit state. This makes the ordering contract visible and lets tests target one phase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cfa2e5f0-e75f-43d3-a7d6-2f36c9d213ef
📒 Files selected for processing (32)
config/reliability-gates.jsoncsrc/main/ipc/remote-workspace-cache.test.tssrc/main/ipc/remote-workspace-events.tssrc/main/ipc/remote-workspace-explicit-session-authority.tssrc/main/ipc/remote-workspace-patch-authority.test.tssrc/main/ipc/remote-workspace-patch-queue.test.tssrc/main/ipc/remote-workspace.test.tssrc/main/ipc/remote-workspace.tssrc/main/ipc/session.test.tssrc/main/ipc/session.tssrc/main/persistence.test.tssrc/main/persistence.tssrc/main/ssh/ssh-relay-session.tssrc/preload/api/workspace-session-api.tssrc/preload/index.tssrc/renderer/src/hooks/remote-workspace-session-merge.test.tssrc/renderer/src/hooks/remote-workspace-session-merge.tssrc/renderer/src/hooks/remote-workspace-snapshot-apply.tssrc/renderer/src/hooks/remote-workspace-snapshot-duplicate-tab-repair.test.tssrc/renderer/src/hooks/remote-workspace-target-sync.test.tssrc/renderer/src/hooks/remote-workspace-target-sync.tssrc/renderer/src/lib/workspace-session-host-persistence.test.tssrc/renderer/src/lib/workspace-session-host-persistence.tssrc/renderer/src/lib/workspace-session-ssh-partition-hydration.tssrc/renderer/src/web/web-preload-api.tssrc/shared/workspace-session-partition-adoption.test.tssrc/shared/workspace-session-partition-adoption.tssrc/shared/workspace-session-partition-authority.test.tssrc/shared/workspace-session-partition-authority.tssrc/shared/workspace-session-partition-provenance.tssrc/shared/workspace-session-partition-state-merge.tstests/e2e/ssh-session-partition-adoption.spec.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- src/renderer/src/web/web-preload-api.ts
- src/preload/index.ts
- src/main/ipc/session.test.ts
- src/renderer/src/hooks/remote-workspace-target-sync.ts
- src/main/ipc/session.ts
- src/renderer/src/lib/workspace-session-host-persistence.ts
- src/main/ipc/remote-workspace-events.ts
- src/main/ipc/remote-workspace-patch-authority.test.ts
- src/main/ssh/ssh-relay-session.ts
- config/reliability-gates.jsonc
- src/main/ipc/remote-workspace-patch-queue.test.ts
- tests/e2e/ssh-session-partition-adoption.spec.ts
- src/renderer/src/lib/workspace-session-host-persistence.test.ts
- src/renderer/src/hooks/remote-workspace-snapshot-apply.ts
- src/main/ipc/remote-workspace-cache.test.ts
- src/renderer/src/lib/workspace-session-ssh-partition-hydration.ts
- src/renderer/src/hooks/remote-workspace-target-sync.test.ts
- src/main/ipc/remote-workspace-explicit-session-authority.ts
- src/renderer/src/hooks/remote-workspace-snapshot-duplicate-tab-repair.test.ts
- src/main/ipc/remote-workspace.test.ts
- src/renderer/src/hooks/remote-workspace-session-merge.ts
The remote workspace snapshot's tab list is the last state some client exported, not a close ledger. A local tab bound to a live pane on the current connection whose id the snapshot does not contain is appended to the merged list instead of dropped, keeping its exact-target layout and remote session mapping. Unbound local rows still defer to the snapshot, ids the snapshot owns elsewhere follow its placement, and rows owned by a workspace outside the replace scope never graft.
|
Pushed a companion fix for the other half of this bug class: The box-side snapshot's tab list is the last state some client exported, not a close ledger. The merge built the result list only from the snapshot's entries, so a local tab absent from that list was dropped even while pty-bound to a live terminal on the very connection being applied — the adopted-partition recovery this PR performs at boot was then undone at connect, detaching the running terminals and duplicating their agent sessions through cold restore. The graft appends local rows the snapshot does not know when they are pty-bound to the current connection, hold recovery authority, or carry a pending activation spawn, keeping their exact-target layouts and remote session mappings. Snapshot-known ids follow the snapshot's placement, ids owned by a worktree outside the replace scope never graft, an id graft-eligible under two replace-scope worktrees fails the merge closed (mirroring Field data: a snapshot carrying ten dormant rows exported weeks earlier replaced a workspace's two live, pty-bound tabs at connect; both running agent sessions were orphaned and relaunched as duplicates. |
…ft superseded by stablyai#14844 The snapshot-merge half (grafting live local tabs over a stale snapshot list) is covered on main by stablyai#14844's host-unknown branch and worktree union, so this branch no longer touches remote-workspace-session-merge.ts. What remains is the partition-adoption mechanism: the export path reads the ssh host partitions, setForConnectedTargets carries the session's target authority, and the Store adopts orphaned ssh partitions into the local session. Re-seated across upstream refactors: the remote-workspace module split (snapshot cache, relay sync), the persistence barrel split (persistence/loading-store/store.ts), and the persistence test split (persistence-host-partitioned-sessions.test.ts). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqNqAZQTwmAkwGcHj5TYXk
…te manifest persistence-ssh-partition-adoption.test.ts holds the 19 adoption tests (the host-partitioned file stays under the test max-lines budget), the reattach gate's testFiles/assertionRefs track the post-split file names, and upstream's generation-preservation target-sync test is restored verbatim. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AqNqAZQTwmAkwGcHj5TYXk
|
Rebased onto current main (merge, history kept). Two notes for review:
|
|
Reproduced the partition mismatch this fixes on 1.4.182 (desktop client paired to an SSH host) — the remote snapshot carried only stale entries from the The reproduction is stable here, so I'm happy to test a build of this branch against that setup if that's useful. |
…stop round-tripping as deletions `workspaceSessionPartitionHostId` answered differently depending on who asked: the renderer mapped an SSH worktree's session to the `local` blob, the main-process runtime read-modify-wrote `ssh:<targetId>`. One workspace's session lived in two stores and no reader reunited them, so whatever landed on the unread side did not read as unknown — it round-tripped as absence. The remote-workspace upload is a `replace-session` patch, which turned that absence into deletion on the host, and the next pull applied the deletion locally and re-poisoned the snapshot. Collapse the two answers into one: every non-'local' host owns its partition. Boot hydration and the export fallback now read the SSH partition, and rows a shipping build left in `local` are folded back in once, gap-filling only — an empty tab row is a gap, never proof that anything was closed. Folder workspaces deliberately keep their existing 'local' routing: boot discovers SSH partitions from the repo catalog, so an SSH target that owns only a folder workspace has no partition any reader enumerates. They are still adopted back out of an SSH partition when a repo does name the host. Fixes #12721 Supersedes #12722 Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com>
…stop round-tripping as deletions `workspaceSessionPartitionHostId` answered differently depending on who asked: the renderer mapped an SSH worktree's session to the `local` blob, the main-process runtime read-modify-wrote `ssh:<targetId>`. One workspace's session lived in two stores and no reader reunited them, so whatever landed on the unread side did not read as unknown — it round-tripped as absence. The remote-workspace upload is a `replace-session` patch, which turned that absence into deletion on the host, and the next pull applied the deletion locally and re-poisoned the snapshot. Collapse the two answers into one: every non-'local' host owns its partition. Boot hydration and the export fallback now read the SSH partition, and rows a shipping build left in `local` are folded back in once, gap-filling only — an empty tab row is a gap, never proof that anything was closed. Folder workspaces deliberately keep their existing 'local' routing: boot discovers SSH partitions from the repo catalog, so an SSH target that owns only a folder workspace has no partition any reader enumerates. They are still adopted back out of an SSH partition when a repo does name the host. Fixes #12721 Supersedes #12722 Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com>
…stop round-tripping as deletions `workspaceSessionPartitionHostId` answered differently depending on who asked: the renderer mapped an SSH worktree's session to the `local` blob, the main-process runtime read-modify-wrote `ssh:<targetId>`. One workspace's session lived in two stores and no reader reunited them, so whatever landed on the unread side did not read as unknown — it round-tripped as absence. The remote-workspace upload is a `replace-session` patch, which turned that absence into deletion on the host, and the next pull applied the deletion locally and re-poisoned the snapshot. Collapse the two answers into one: every non-'local' host owns its partition. Boot hydration and the export fallback now read the SSH partition, and rows a shipping build left in `local` are folded back in once, gap-filling only — an empty tab row is a gap, never proof that anything was closed. Folder workspaces deliberately keep their existing 'local' routing: boot discovers SSH partitions from the repo catalog, so an SSH target that owns only a folder workspace has no partition any reader enumerates. They are still adopted back out of an SSH partition when a repo does name the host. Fixes #12721 Supersedes #12722 Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com>
…stop round-tripping as deletions (#19572) * fix(session): give an SSH workspace one owning partition so its tabs stop round-tripping as deletions `workspaceSessionPartitionHostId` answered differently depending on who asked: the renderer mapped an SSH worktree's session to the `local` blob, the main-process runtime read-modify-wrote `ssh:<targetId>`. One workspace's session lived in two stores and no reader reunited them, so whatever landed on the unread side did not read as unknown — it round-tripped as absence. The remote-workspace upload is a `replace-session` patch, which turned that absence into deletion on the host, and the next pull applied the deletion locally and re-poisoned the snapshot. Collapse the two answers into one: every non-'local' host owns its partition. Boot hydration and the export fallback now read the SSH partition, and rows a shipping build left in `local` are folded back in once, gap-filling only — an empty tab row is a gap, never proof that anything was closed. Folder workspaces deliberately keep their existing 'local' routing: boot discovers SSH partitions from the repo catalog, so an SSH target that owns only a folder workspace has no partition any reader enumerates. They are still adopted back out of an SSH partition when a repo does name the host. Fixes #12721 Supersedes #12722 Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com> * test(session): pin the old-client empty-publish skew direction * fix(session): adopt every workspace the host partition names, not only tabbed ones Review caught that gating adoption on `host.tabsByWorktree[key].length > 0` traded the #12721 deletion for a narrower one. The write path routes EVERY worktree-scoped field to the owning partition, so an SSH workspace with open editor files or browser tabs and no terminals had all of it dropped on every restart — and unlike terminal state it cannot be recovered from the host snapshot, which carries terminal fields only, so an unsaved `dirtyDraftContent` was destroyed outright. The defect was not a missing field. It was a hand-maintained field list deciding what the read recovers while the write used the ownership table, so the two could disagree. Adoption now walks `WORKSPACE_SESSION_FIELD_OWNERSHIP` with an exhaustive switch, and a new ownership kind is a compile-time decision rather than a silent omission. Session keys are normalized through the shared `normalizeWorkspaceSessionKeyToWorkspaceId` so host-qualified visit recency (`ssh:target|worktreeId`) reaches its workspace, and the regression is pinned by feeding the shipping split's own output back through the real boot read rather than a hand-built fixture. Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com> * fix(session): stop adoption overwriting rows it was never told about Three losses, one cause: the reader walks its own description of the partition layout while the writer walks another, so the two agree on which ownership kinds exist and not on what a kind means. - An empty host row replaced a populated base row, destroying an unsaved dirtyDraftContent the header comment says must never be destroyed. The host holding nothing is not evidence the base is wrong. - A contested bare id was adopted as if local and ssh:<target> were one workspace written twice, which is exactly the id where that premise is false. The read already reached that verdict and adoption could not ask for it, so it is passed in; contested keys are gap-filled, never replaced. mergeWorkspaceSessionsWithHostShadow now reports the real contested set, which primaryHostBySessionKey never was. - Tab-, pane- and file-keyed rows are adopted through the split's own indexes, so unified-only tabs come back and the pane key is parsed once. - A bare lastVisitedAtByWorktreeId key only fills a gap; the split has a dedicated branch for that field and the reader had none. * test(session): pin the tombstone/gap boundary the two readings meet at An explicit empty tabsByWorktree row means the user closed the last terminal; adoption reads an empty base row as a gap to fill. Same value, opposite readings, so the boundary is asserted rather than argued: the tombstone lands in the owning partition, restores as a present empty row rather than a deleted key, is declined by the real seeding predicate, is published as an empty list, and the legacy-transition resurrection happens once and cannot recur. * docs(reliability): record the adoption guards and the tombstone boundary in the gate * test(e2e): read the SSH restart assertions from the partition that owns them ssh-cold-activation-restore asserted persistence through session.get() with no host, which is the local partition an SSH worktree's rows no longer live in. The invariant it means to check is that the state is persisted where the boot read will find it, so it now unions local and ssh:<targetId> and stays correct on both layouts. Confirmed the product invariant separately rather than by the edit: the behavioural half of both tests - the full app restart, the active worktree, the eager terminal remount and the PTY-owner reclaim against a real Docker OpenSSH host - runs after this check and passes. 2 passed in 48.9s. * test(e2e): read ssh-restart-tab-accumulation from the owning partition too Same layout-coupled read as ssh-cold-activation-restore: the pre-quit flush asserted through session.get() with no host. Verified against a real Docker OpenSSH target - both repeated quit/relaunch cycles keep exactly the restored SSH tabs, no accumulation and no loss. 2 passed in 52.9s. * fix(lint): clear the casting gate on the partition adoption main tightened typescript/consistent-type-assertions to assertionStyle: never, which the rebase brings onto these added lines. Most of the round-trip fixtures did not need a cast at all -- three were hiding wrong-shaped literals (a browser workspace keyed 'name', a unified tab keyed 'type', a layout keyed 'direction'), now written as the types they stand for. The adoption reads narrow through an isRecord predicate instead of casting, which also stops a null entry throwing out of Object.keys. What is left is dynamic-field writes and unknown-typed IPC returns, each with its own SAFETY rationale. * fix(session): give an SSH folder workspace one owning partition boot can find The partition owner rule already names `ssh:<targetId>` for a repo-backed worktree, but `getFolderWorkspacePartitionHostId` still answered 'local' for a folder workspace while main's `RuntimeWorkspaceSessionController.getPreferredHostId` answered `ssh:<targetId>` for the same key. That is #12723 unfixed for folder workspaces, and once the renderer started writing `ssh:*` at all it got worse: a save's field-level patch carries only the rows routed to that partition, so a `tabsByWorktree` write without the folder row erased the row main had put there. The reason the renderer could not route there was real - boot discovered SSH partitions from the repo catalog, which cannot name a target whose only workspace is a folder. So persistence now answers that directly over `session:list-host-ids`, and boot reads the partitions that exist rather than the ones a catalog implies. Removing a folder workspace prunes its rows from the owning partition too, or the census would adopt them back on the next launch as a workspace the user already deleted. Adoption now decides from the repo catalog instead of from co-presence. Two partitions holding one bare `repoId::path` is not evidence of a collision - that is the exact shape the repair exists for - so the verdict comes from `resolveWorktreeExecutionHost`: a repo id registered on more than one host is contested and may only be gap-filled, and one the catalog positively resolves to a different host is residue this partition does not own and is not adopted at all. Without the second rule a stale partition sorting first won the read and was then written into the live one. Nothing is deleted either way; the rows stay where they are. Finally, a workspace adopted out of a partition now routes back to that partition. Routing used to re-derive an owner from the catalog, so a boot whose repos had not hydrated moved the rows it had just reunited back into 'local' and re-stranded them. Contested ids are withheld from that override, because routing the whole bare id to one host is the loss the gap-fill prevents. The publish path resolves each workspace's owner once for the whole publish, shared with the projection, so the per-target catalog attribution does not repeat it per connected host. * fix(session): drop a deleted workspace from every partition, not just the local blob Adversarial review of the previous commit found three ways the partition census - which now reads whatever persistence holds rather than what the repo catalog implies - keeps rows alive that nothing should keep alive. `deleteProjectGroup` pruned only the local blob, so every folder workspace under a deleted group left its rows in `ssh:<targetId>`; the next boot adopted them back, named that partition their owner and wrote them there again, forever. `removeFolderWorkspace` had the same hole for a workspace whose partition its host expression could not name: main never persists a folder workspace's `executionHostId`, and `RuntimeWorkspaceSessionController` can infer a connection from the group's repos that the workspace row itself does not carry. Deriving the partition at delete time is the wrong question - a deleted workspace owns nothing anywhere - so both paths now remove it from every partition. The third is on the read side. A contested id is deliberately withheld from the read-source override so the write cannot carry one host's rows into another's partition, but the routing that then re-derives an owner answers 'local' for an id the catalog cannot name. Adopting such a row moved it out of the partition that owns it and into the blob: the two-store split this change exists to remove. A contested id the assembled session holds no row for is therefore not adopted at all. Gap-filling stays available for a contested id the session already names, since that row's own partition is what the write follows. Declining to adopt leaves a row invisible for one boot; it never deletes one. Also: the folder-key guard in both catalog attributions was dead, because `getRepoIdFromWorktreeId` hands back the whole key rather than nothing when there is no `::`. The verdict was right and the resolution wasted; it now skips by shape. And the two type assertions the changed-code casting gate rejected are gone rather than suppressed. * fix(session): park the rows a partition read declines instead of letting the next write erase them A partition write replaces each field with exactly what the unified session routed there. So a row the read left out of that session is erased from its own partition the moment any sibling workspace writes the same one - and with SSH partitions now the owning store, that row is then in no partition at all. Three separate decisions produce such rows: residue the catalog attributes to another host, a contested id withheld so the write cannot carry one host's rows into another's partition, and a workspace the base already holds the live copy of. Declining to show a row was quietly deleting it. The machinery for this already exists. `attachHostSessionShadow` writes a contested runtime co-claimant's parked rows straight back into its own slice before the write, so the primary's write cannot erase them; the ssh partitions simply were not among the slices the contention split arbitrates. The read now parks everything it is not returning to an ssh partition into that same shadow, and the existing re-attach puts it back. Leak, never kill - docs/reference/ssh-execution- boundary.md - and a row no partition holds is unrecoverable. Second, the contested branch of the tab adoption read `Object.hasOwn` as "the base has tabs here". An empty list satisfies it, so whenever a legacy id happened to be contested, #12721's empty local row won over the host's real one - the exact reading the module's own header, and the gate invariant it is pinned by, say is wrong. An empty row is the gap this repair fills, so it is now treated as one. * test(session): pin the empty-base-row gap for a contested id Mutation testing found the assertion missing: reverting the gate to `Object.hasOwn` left all 39 assertions passing, which makes the fix that reads an empty base tab row as a gap unguarded. The #12721 shape does not stop being a gap because the id happens to be contested. --------- Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com>
…stop round-tripping as deletions (stablyai#19572) * fix(session): give an SSH workspace one owning partition so its tabs stop round-tripping as deletions `workspaceSessionPartitionHostId` answered differently depending on who asked: the renderer mapped an SSH worktree's session to the `local` blob, the main-process runtime read-modify-wrote `ssh:<targetId>`. One workspace's session lived in two stores and no reader reunited them, so whatever landed on the unread side did not read as unknown — it round-tripped as absence. The remote-workspace upload is a `replace-session` patch, which turned that absence into deletion on the host, and the next pull applied the deletion locally and re-poisoned the snapshot. Collapse the two answers into one: every non-'local' host owns its partition. Boot hydration and the export fallback now read the SSH partition, and rows a shipping build left in `local` are folded back in once, gap-filling only — an empty tab row is a gap, never proof that anything was closed. Folder workspaces deliberately keep their existing 'local' routing: boot discovers SSH partitions from the repo catalog, so an SSH target that owns only a folder workspace has no partition any reader enumerates. They are still adopted back out of an SSH partition when a repo does name the host. Fixes stablyai#12721 Supersedes stablyai#12722 Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com> * test(session): pin the old-client empty-publish skew direction * fix(session): adopt every workspace the host partition names, not only tabbed ones Review caught that gating adoption on `host.tabsByWorktree[key].length > 0` traded the stablyai#12721 deletion for a narrower one. The write path routes EVERY worktree-scoped field to the owning partition, so an SSH workspace with open editor files or browser tabs and no terminals had all of it dropped on every restart — and unlike terminal state it cannot be recovered from the host snapshot, which carries terminal fields only, so an unsaved `dirtyDraftContent` was destroyed outright. The defect was not a missing field. It was a hand-maintained field list deciding what the read recovers while the write used the ownership table, so the two could disagree. Adoption now walks `WORKSPACE_SESSION_FIELD_OWNERSHIP` with an exhaustive switch, and a new ownership kind is a compile-time decision rather than a silent omission. Session keys are normalized through the shared `normalizeWorkspaceSessionKeyToWorkspaceId` so host-qualified visit recency (`ssh:target|worktreeId`) reaches its workspace, and the regression is pinned by feeding the shipping split's own output back through the real boot read rather than a hand-built fixture. Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com> * fix(session): stop adoption overwriting rows it was never told about Three losses, one cause: the reader walks its own description of the partition layout while the writer walks another, so the two agree on which ownership kinds exist and not on what a kind means. - An empty host row replaced a populated base row, destroying an unsaved dirtyDraftContent the header comment says must never be destroyed. The host holding nothing is not evidence the base is wrong. - A contested bare id was adopted as if local and ssh:<target> were one workspace written twice, which is exactly the id where that premise is false. The read already reached that verdict and adoption could not ask for it, so it is passed in; contested keys are gap-filled, never replaced. mergeWorkspaceSessionsWithHostShadow now reports the real contested set, which primaryHostBySessionKey never was. - Tab-, pane- and file-keyed rows are adopted through the split's own indexes, so unified-only tabs come back and the pane key is parsed once. - A bare lastVisitedAtByWorktreeId key only fills a gap; the split has a dedicated branch for that field and the reader had none. * test(session): pin the tombstone/gap boundary the two readings meet at An explicit empty tabsByWorktree row means the user closed the last terminal; adoption reads an empty base row as a gap to fill. Same value, opposite readings, so the boundary is asserted rather than argued: the tombstone lands in the owning partition, restores as a present empty row rather than a deleted key, is declined by the real seeding predicate, is published as an empty list, and the legacy-transition resurrection happens once and cannot recur. * docs(reliability): record the adoption guards and the tombstone boundary in the gate * test(e2e): read the SSH restart assertions from the partition that owns them ssh-cold-activation-restore asserted persistence through session.get() with no host, which is the local partition an SSH worktree's rows no longer live in. The invariant it means to check is that the state is persisted where the boot read will find it, so it now unions local and ssh:<targetId> and stays correct on both layouts. Confirmed the product invariant separately rather than by the edit: the behavioural half of both tests - the full app restart, the active worktree, the eager terminal remount and the PTY-owner reclaim against a real Docker OpenSSH host - runs after this check and passes. 2 passed in 48.9s. * test(e2e): read ssh-restart-tab-accumulation from the owning partition too Same layout-coupled read as ssh-cold-activation-restore: the pre-quit flush asserted through session.get() with no host. Verified against a real Docker OpenSSH target - both repeated quit/relaunch cycles keep exactly the restored SSH tabs, no accumulation and no loss. 2 passed in 52.9s. * fix(lint): clear the casting gate on the partition adoption main tightened typescript/consistent-type-assertions to assertionStyle: never, which the rebase brings onto these added lines. Most of the round-trip fixtures did not need a cast at all -- three were hiding wrong-shaped literals (a browser workspace keyed 'name', a unified tab keyed 'type', a layout keyed 'direction'), now written as the types they stand for. The adoption reads narrow through an isRecord predicate instead of casting, which also stops a null entry throwing out of Object.keys. What is left is dynamic-field writes and unknown-typed IPC returns, each with its own SAFETY rationale. * fix(session): give an SSH folder workspace one owning partition boot can find The partition owner rule already names `ssh:<targetId>` for a repo-backed worktree, but `getFolderWorkspacePartitionHostId` still answered 'local' for a folder workspace while main's `RuntimeWorkspaceSessionController.getPreferredHostId` answered `ssh:<targetId>` for the same key. That is stablyai#12723 unfixed for folder workspaces, and once the renderer started writing `ssh:*` at all it got worse: a save's field-level patch carries only the rows routed to that partition, so a `tabsByWorktree` write without the folder row erased the row main had put there. The reason the renderer could not route there was real - boot discovered SSH partitions from the repo catalog, which cannot name a target whose only workspace is a folder. So persistence now answers that directly over `session:list-host-ids`, and boot reads the partitions that exist rather than the ones a catalog implies. Removing a folder workspace prunes its rows from the owning partition too, or the census would adopt them back on the next launch as a workspace the user already deleted. Adoption now decides from the repo catalog instead of from co-presence. Two partitions holding one bare `repoId::path` is not evidence of a collision - that is the exact shape the repair exists for - so the verdict comes from `resolveWorktreeExecutionHost`: a repo id registered on more than one host is contested and may only be gap-filled, and one the catalog positively resolves to a different host is residue this partition does not own and is not adopted at all. Without the second rule a stale partition sorting first won the read and was then written into the live one. Nothing is deleted either way; the rows stay where they are. Finally, a workspace adopted out of a partition now routes back to that partition. Routing used to re-derive an owner from the catalog, so a boot whose repos had not hydrated moved the rows it had just reunited back into 'local' and re-stranded them. Contested ids are withheld from that override, because routing the whole bare id to one host is the loss the gap-fill prevents. The publish path resolves each workspace's owner once for the whole publish, shared with the projection, so the per-target catalog attribution does not repeat it per connected host. * fix(session): drop a deleted workspace from every partition, not just the local blob Adversarial review of the previous commit found three ways the partition census - which now reads whatever persistence holds rather than what the repo catalog implies - keeps rows alive that nothing should keep alive. `deleteProjectGroup` pruned only the local blob, so every folder workspace under a deleted group left its rows in `ssh:<targetId>`; the next boot adopted them back, named that partition their owner and wrote them there again, forever. `removeFolderWorkspace` had the same hole for a workspace whose partition its host expression could not name: main never persists a folder workspace's `executionHostId`, and `RuntimeWorkspaceSessionController` can infer a connection from the group's repos that the workspace row itself does not carry. Deriving the partition at delete time is the wrong question - a deleted workspace owns nothing anywhere - so both paths now remove it from every partition. The third is on the read side. A contested id is deliberately withheld from the read-source override so the write cannot carry one host's rows into another's partition, but the routing that then re-derives an owner answers 'local' for an id the catalog cannot name. Adopting such a row moved it out of the partition that owns it and into the blob: the two-store split this change exists to remove. A contested id the assembled session holds no row for is therefore not adopted at all. Gap-filling stays available for a contested id the session already names, since that row's own partition is what the write follows. Declining to adopt leaves a row invisible for one boot; it never deletes one. Also: the folder-key guard in both catalog attributions was dead, because `getRepoIdFromWorktreeId` hands back the whole key rather than nothing when there is no `::`. The verdict was right and the resolution wasted; it now skips by shape. And the two type assertions the changed-code casting gate rejected are gone rather than suppressed. * fix(session): park the rows a partition read declines instead of letting the next write erase them A partition write replaces each field with exactly what the unified session routed there. So a row the read left out of that session is erased from its own partition the moment any sibling workspace writes the same one - and with SSH partitions now the owning store, that row is then in no partition at all. Three separate decisions produce such rows: residue the catalog attributes to another host, a contested id withheld so the write cannot carry one host's rows into another's partition, and a workspace the base already holds the live copy of. Declining to show a row was quietly deleting it. The machinery for this already exists. `attachHostSessionShadow` writes a contested runtime co-claimant's parked rows straight back into its own slice before the write, so the primary's write cannot erase them; the ssh partitions simply were not among the slices the contention split arbitrates. The read now parks everything it is not returning to an ssh partition into that same shadow, and the existing re-attach puts it back. Leak, never kill - docs/reference/ssh-execution- boundary.md - and a row no partition holds is unrecoverable. Second, the contested branch of the tab adoption read `Object.hasOwn` as "the base has tabs here". An empty list satisfies it, so whenever a legacy id happened to be contested, stablyai#12721's empty local row won over the host's real one - the exact reading the module's own header, and the gate invariant it is pinned by, say is wrong. An empty row is the gap this repair fills, so it is now treated as one. * test(session): pin the empty-base-row gap for a contested id Mutation testing found the assertion missing: reverting the gate to `Object.hasOwn` left all 39 assertions passing, which makes the fix that reads an empty base tab row as a gap unguarded. The stablyai#12721 shape does not stop being a gap because the id happens to be contested. --------- Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com>
|
Thank you @robertnisipeanu, your forensics in #12721 shaped the fix. Main now gives an SSH workspace one owning session partition and adopts rows stranded in the other one at startup (#19572, then #26098), so tabs no longer round-trip as deletions. Closing as fixed; please reopen if you can still reproduce it. |
Fixes #12721
What
Session state persisted into
ssh:<targetId>host partitions by main-process runtime flows is invisible to both readers of the workspace session — boot hydration reads only the local andruntime:*partitions, and theremoteWorkspace:setForConnectedTargetsno-session fallback exports from the local partition alone. A worktree whose only populated tab list lives in anssh:partition renders empty at boot and is uploaded to the target's remote workspace snapshot as an explicit empty tab list; the remote-authoritative pull merge then deletes those tabs on every client, on every launch, and pushes the emptied state back — a permanent, self-sustaining wipe (details and real-workspace forensics in #12721).This PR fixes the read side only; neither writer's partition mapping changes.
src/shared/workspace-session-partition-adoption.ts— adoption of orphaned partition entries. A worktree whose base session holds no tabs takes the partition's populated entry, together with the tab-scoped records (terminalLayoutsByTabId,remoteSessionIdsByTabId) of exactly the adopted tabs and the worktree-keyed scalars the base lacks. Populated base entries always win — the partition copy may be a stale fork. Tabs fenced by a surface tombstone on either side stay retired, so deliberately closed panes (whose retirement wrote a tombstone, fix(runtime): retire an exited pane's surface in its owning host partition (STA-3463) #12671) are never resurrected. Nothing is ever removed from the base.Renderer boot hydration (
workspace-session-ssh-partition-hydration.ts, wired intofetchWorkspaceSessionWithRuntimeHostOwners) reads thessh:partitions of known SSH repos and completes each adoption as a move: the local partition durably gains every adopted field via a patch built from a fresh read (the patch API replaces whole fields), awaited, before the source partition sheds the adopted entries. The ordering means a crash between the two writes cannot leave the tabs without a persisted copy on either side, and the prune means tabs the user closes later cannot be re-adopted on the next boot. Thessh:slices deliberately stay out of the runtime-owner map and the blind last-wins host merge.remoteWorkspace:setForConnectedTargetsadopts per target beforeexportSessionForTarget, read-only: the renderer owns the local partition and replaces its worktree map wholesale on every debounced write, so a store-side move on this hot path would be silently undone — with the only durable copy already pruned. The export is recomputed live from both partitions on every call, so the snapshot can no longer be poisoned with empty lists; the durable move belongs to boot hydration, where the adopted tabs enter the renderer's own state.Provider-authority gating on the target sync path (
remote-workspace-snapshot-cache.ts,remote-workspace-relay-sync.ts,remote-workspace-explicit-session-authority.ts) — every snapshot cache entry, relay pull/patch, and workspace-changed notification is keyed to the exactDirectSshAuthority(target, provider epoch, connection generation) that produced it and re-validated across each await, so a replaced provider's late responses cannot poison the current provider's cache or export a stale session under a fresh connection.Cross-target isolation is double-gated: each target's adoption reads only its own
ssh:<targetId>partition, andexportSessionForTargetstill filters by the repo'sconnectionIdbefore upload. Workspaces with no SSH repos take the exact pre-change paths (no partitions to adopt).Validation
pnpm vitest run src/shared/workspace-session-partition-adoption.test.ts src/main/persistence-ssh-partition-adoption.test.ts src/renderer/src/lib/workspace-session-host-persistence.test.ts src/main/ipc/remote-workspace.test.ts src/main/ipc/remote-workspace-patch-authority.test.ts src/main/ipc/remote-workspace-cache.test.ts— new unit coverage for adoption/move/prune semantics (base-wins, tombstone fencing, identity when nothing to adopt, no deletions from base, prune scoped to adopted entries), boot-time adoption with move ordering, stranded-tab export, and the fallback path staying write-free.pnpm vitest run src/renderer/src/lib/workspace-session src/main/ipc/remote-workspace— all adjacent suites green.pnpm typecheck,oxlint,oxfmtclean.🤖 Generated with Claude Code