From fb29895b44f97743907d65223c8d6c2fd0983773 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 13 Aug 2026 20:05:12 -0700 Subject: [PATCH] fix(ssh): hoist a pane only when both its halves survived salvage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2-C. Field-level salvage can drop a tab record while keeping its layout, or the reverse, so the hoist could move half a pane into local: a tab with no layout blanks its pane, and an orphan layout is unreachable. Local-plane salvage has the same asymmetry, but hoisting moved the damage out of a quarantined partition and into the plane everything reads. A partially-salvaged pane now stays put. P2-D. Drops the dead !lease.updatedAt clause from the expired-lease prune — normalizeSshRemotePtyLease always populates it, so the branch never ran. Mutation-proofed: removing the orphan-layout guard reddens the new test. --- src/main/persistence.ts | 20 ++++++++++-- .../ssh-partition-upgrade-tab-loss.test.ts | 32 +++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 89c6e78a322..e29f44a837a 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -2889,7 +2889,16 @@ export function hoistSshPartitionsIntoLocalSession( for (const [worktreeId, tabs] of Object.entries(partition.tabsByWorktree ?? {})) { const existing = local.tabsByWorktree?.[worktreeId] ?? [] const known = new Set(existing.map((tab) => tab.id)) - const missing = (tabs ?? []).filter((tab) => !known.has(tab.id) && !alreadyHoisted.has(tab.id)) + // Both halves or neither: field-level salvage can drop a tab while keeping its layout, or + // the reverse. A tab with no layout blanks its pane, and an orphan layout is unreachable — + // so a partially-salvaged pane stays quarantined in the partition rather than moving the + // damage into local. + const missing = (tabs ?? []).filter( + (tab) => + !known.has(tab.id) && + !alreadyHoisted.has(tab.id) && + Boolean(partition.terminalLayoutsByTabId?.[tab.id]) + ) if (missing.length === 0) { continue } @@ -2903,6 +2912,13 @@ export function hoistSshPartitionsIntoLocalSession( if (!layout || local.terminalLayoutsByTabId?.[tabId]) { continue } + // Only alongside a tab that exists somewhere; an orphan layout is dead weight in local. + const hasTab = Object.values(local.tabsByWorktree ?? {}).some((tabs) => + (tabs ?? []).some((tab) => tab.id === tabId) + ) + if (!hasTab) { + continue + } local.terminalLayoutsByTabId = { ...local.terminalLayoutsByTabId, [tabId]: layout } changed = true } @@ -4014,9 +4030,9 @@ export class Store { // removed one, so the list grew for the life of the profile. Retention sits far above the // pane-recovery grace window, which is the only remaining reader. const retainedLeases = (result.sshRemotePtyLeases ?? []).filter( + // normalizeSshRemotePtyLease always populates updatedAt, so no absent-timestamp case here. (lease) => lease.state !== 'expired' || - !lease.updatedAt || Date.now() - lease.updatedAt <= EXPIRED_SSH_LEASE_RETENTION_MS ) if (retainedLeases.length !== (result.sshRemotePtyLeases ?? []).length) { diff --git a/src/main/ssh-partition-upgrade-tab-loss.test.ts b/src/main/ssh-partition-upgrade-tab-loss.test.ts index 661ba5cc0fc..32b9e15628a 100644 --- a/src/main/ssh-partition-upgrade-tab-loss.test.ts +++ b/src/main/ssh-partition-upgrade-tab-loss.test.ts @@ -29,6 +29,7 @@ const SSH_PARTITION = toSshExecutionHostId(TARGET) const WORKTREE = 'repo-1:wt-1' const TAB = 'tab-1' const LEAF = '3f1c9a2e-7b4d-4e1a-9c8f-2d5e6a7b8c90' +const LEAF_TWO = '9a8b7c6d-5e4f-4a3b-8c9d-0e1f2a3b4c5d' const PTY = toAppSshPtyId(TARGET, 'pty-1') async function createStore(state: Record = {}) { @@ -214,6 +215,17 @@ describe('an SSH pane whose membership was persisted into the ssh partition', () ...second.tabsByWorktree[WORKTREE], { ...second.tabsByWorktree[WORKTREE][0], id: 'tab-2' } ] + // Its layout too: a pane is only hoisted when both halves survived, so a tab without one + // would be refused — correctly, but it would not exercise what this test is about. + second.terminalLayoutsByTabId = { + ...second.terminalLayoutsByTabId, + 'tab-2': { + root: { type: 'leaf' as const, leafId: LEAF_TWO }, + activeLeafId: LEAF_TWO, + expandedLeafId: null, + ptyIdsByLeafId: { [LEAF_TWO]: PTY } + } + } upgraded.setWorkspaceSession(second as never, SSH_PARTITION) upgraded.flushOrThrow() @@ -223,4 +235,24 @@ describe('an SSH pane whose membership was persisted into the ssh partition', () 'a pane the partition gained after the first hoist was stranded' ).toEqual(['tab-1', 'tab-2']) }) + + // Field-level salvage can drop a tab record while keeping its layout, or the reverse. Moving + // half a pane into local is worse than leaving it quarantined: a tab with no layout blanks, and + // an orphan layout is unreachable. + it('leaves a partially-salvaged pane in the partition rather than half-hoisting it', async () => { + const before = await createStore() + const partial = paneSession() + // The layout survives, the tab record does not — exactly what salvage produces. + partial.tabsByWorktree = { [WORKTREE]: [] } + before.setWorkspaceSession(partial as never, SSH_PARTITION) + before.flushOrThrow() + stripHoistMarker() + + const store = await reopenStore() + + expect( + store.getWorkspaceSession().terminalLayoutsByTabId?.[TAB], + 'an orphan layout was hoisted into local with no tab to reach it' + ).toBeUndefined() + }) })