From 4716cc2fea89e1b5c0cf4ce5dec559ed697b92d4 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 13 Aug 2026 20:17:07 -0700 Subject: [PATCH] fix(ssh): restore cleared bindings on rollback, bound the hoist ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F3 (P2): the supersession undo restored lease state but not the pane bindings the forward path cleared, so a failed flush left a predecessor holding a live lease with no binding — and the clear queues its own save, writing that hybrid to disk. Now snapshots via the same helpers the async twin already uses. F5 (P3): sshHoistedTabIds grew for the life of the profile and, because the CLI can reuse a tab id after its tab closes, a reused id was stranded forever. Bounded to the newest 512 — far above any realistic open-tab count, and the worst case of eviction is re-hoisting a long-closed tab once. F4 (P3): the 'counterfactual' test no longer isolates mayCreate now that the hoist runs before it, so it is relabelled as the end-to-end assertion it actually is rather than left claiming a property it does not test. --- src/main/persistence.ts | 14 +++++++++++++- src/main/ssh-partition-upgrade-tab-loss.test.ts | 9 +++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 8c5c8ebb63f..851c09988af 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -2850,6 +2850,9 @@ export type PtyBindingSourceExpectation = { } +/** Caps the hoist ledger. Far above any realistic open-tab count, so a live tab is never evicted. */ +const MAX_HOISTED_TAB_IDS = 512 + /** Expired leases are unreachable from reattach; kept only long enough to outlive any in-flight * recovery window by orders of magnitude. */ const EXPIRED_SSH_LEASE_RETENTION_MS = 7 * 24 * 60 * 60 * 1000 @@ -2941,7 +2944,12 @@ export function hoistSshPartitionsIntoLocalSession( } } if (changed) { - state.settings = { ...state.settings, sshHoistedTabIds: [...alreadyHoisted] } + // Bounded, newest-last. A tab id can be reused by the CLI after its tab closes, and an + // unbounded list would both grow for the life of the profile and strand such a pane forever. + // Dropping the oldest entries only risks re-hoisting a long-closed tab once, which is the + // milder failure of the two. + const bounded = [...alreadyHoisted].slice(-MAX_HOISTED_TAB_IDS) + state.settings = { ...state.settings, sshHoistedTabIds: bounded } } return changed } @@ -7631,11 +7639,15 @@ export class Store { } // Why: matching on lease ptyId first means this scrubs only the predecessor's // stale binding — the winner's own binding cannot match and is left intact. + // Snapshot before clearing: the clear mutates live pane state (tab.ptyId, ptyIdsByLeafId), so + // restoring only the lease rows would leave a predecessor holding a live lease with no binding. + const bindingsBefore = this.snapshotSshLeaseBindings(winner.targetId, superseded) this.clearSshRemotePtyBindingsForLeases(winner.targetId, superseded, 'local') return () => { for (const undo of restore) { undo() } + this.restoreSshLeaseBindings(winner.targetId, superseded, bindingsBefore) } } diff --git a/src/main/ssh-partition-upgrade-tab-loss.test.ts b/src/main/ssh-partition-upgrade-tab-loss.test.ts index 9ac8508ab37..02b70e1673b 100644 --- a/src/main/ssh-partition-upgrade-tab-loss.test.ts +++ b/src/main/ssh-partition-upgrade-tab-loss.test.ts @@ -137,10 +137,11 @@ describe('an SSH pane whose membership was persisted into the ssh partition', () ).toBe(true) }) - // The counterfactual that makes this a REGRESSION rather than a standing limitation: before - // `mayCreate` existed, the same state re-created the tab instead of discarding it. That is what - // "before, many of the tabs were retained" described — they were being recreated. - it('was recreated rather than discarded before the refusal was introduced', async () => { + // NOTE: with the hoist in place this no longer isolates `mayCreate` — the reopen has already + // folded the tab into local, so the bind would succeed either way. Kept as a plain end-to-end + // assertion that the upgraded profile can bind, not as a counterfactual. The counterfactual it + // used to encode is now carried by the hoist's own mutation test. + it('binds without needing to create, once the pane has been hoisted', async () => { const before = await createStore() before.setWorkspaceSession(paneSession() as never, SSH_PARTITION) before.flushOrThrow()