From 7724a950ecd8c15bbb51de855cbb01a92568b158 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 12 Aug 2026 02:40:29 -0700 Subject: [PATCH] fix(ssh): a supersession decided from one plane only mutates that plane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness round 2 confirmed finding. Arbitration ranks leases using the desktop plane's binding, then handed its losers to a scrub that walked BOTH planes — so the headless/CLI/mobile plane's binding was deleted for a lease it never got to vote on. Its owner then has no durable record to reattach that shell by, and can fall back to recency or orphan a shell that is still running. `clearSshRemotePtyBindingsForLeases` now takes `arbitratedFrom`. A decision reached by reading one plane may only mutate that plane. An explicit expiry or termination is plane-agnostic — the pty is gone for everyone — so those callers pass nothing and still scrub both, which is what the existing `markSshRemotePtyLease` oracle pins. I initially assessed this as not-a-defect, reasoning that local winning IS the STA-3077 fix. That was about RANKING and did not justify DELETING the other plane's record; the round-2 verdict was right and I was wrong. It did also catch a comment of mine that had gone stale — a clause claimed the other plane was "deliberately left alone" while the code cleared it. The comment now says what the code does and why. Mutation: letting arbitration scrub both planes again reddens the new clause. 43,256 pass; the 7 failures are pre-existing on clean main in this environment. --- src/main/persistence.ts | 22 ++++++++++------ src/main/ssh-pane-binding-partition.test.ts | 28 ++++++++++++++++++++- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 5781bcd379b..6c954cfe710 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -7344,7 +7344,7 @@ 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. - this.clearSshRemotePtyBindingsForLeases(winner.targetId, superseded) + this.clearSshRemotePtyBindingsForLeases(winner.targetId, superseded, 'local') } /** @@ -7453,7 +7453,7 @@ export class Store { return 0 } const sessionsBefore = this.cloneSshLeaseBindingSessions(targetId) - this.clearSshRemotePtyBindingsForLeases(targetId, superseded) + this.clearSshRemotePtyBindingsForLeases(targetId, superseded, 'local') try { // Why flushOrThrow: flush() swallows write errors, which would leave these // leases retired in memory but attached on disk for the rest of the session. @@ -7632,19 +7632,27 @@ export class Store { this.clearSshRemotePtyBindingsForLeases(targetId, leases ?? []) } + /** `arbitratedFrom` names the plane a supersession DECISION was made from. A decision reached by + * reading one plane may only mutate that plane: the other plane's binding never got a vote, and + * deleting it strands a shell its owner can still be using. An explicit expiry or termination is + * plane-agnostic — the pty is gone for everyone — so those callers pass nothing. */ private clearSshRemotePtyBindingsForLeases( targetId: string, - leases: SshRemotePtyLease[] + leases: SshRemotePtyLease[], + arbitratedFrom?: 'local' ): boolean { if (!leases?.length) { return false } let changed = false const sessions = new Set( - [ - this.state.workspaceSession, - this.state.workspaceSessionsByHostId?.[toSshExecutionHostId(targetId)] - ].filter((session): session is WorkspaceSessionState => Boolean(session)) + (arbitratedFrom === 'local' + ? [this.state.workspaceSession] + : [ + this.state.workspaceSession, + this.state.workspaceSessionsByHostId?.[toSshExecutionHostId(targetId)] + ] + ).filter((session): session is WorkspaceSessionState => Boolean(session)) ) for (const session of sessions) { for (const [worktreeId, tabs] of Object.entries(session.tabsByWorktree ?? {})) { diff --git a/src/main/ssh-pane-binding-partition.test.ts b/src/main/ssh-pane-binding-partition.test.ts index 9ea523e4a08..bada2d1812f 100644 --- a/src/main/ssh-pane-binding-partition.test.ts +++ b/src/main/ssh-pane-binding-partition.test.ts @@ -225,6 +225,29 @@ describe('STA-3077 step P: one pane, one live claim across partitions', () => { }) }) +describe('STA-3077: a decision made from one plane only mutates that plane', () => { + // Arbitration ranks using the desktop plane's binding. The headless plane's binding never gets a + // vote, so deleting it here would strand a shell its owner may still be using — and that owner + // would then have no durable record to reattach by. An explicit expiry is different: the pty is + // gone for everyone, and that path still scrubs both. + it('leaves the headless plane binding intact when supersession retires its lease', async () => { + const sshSession = paneSession('pty-old') as unknown as Record + const store = await createStore({ + workspaceSession: paneSession('pty-new'), + workspaceSessionsByHostId: { [SSH_PARTITION]: sshSession } + }) + sshSpawnUpsertsLease(store, 'pty-old') + + // Local names pty-new, so arbitration expires pty-old without the other plane being consulted. + sshSpawnUpsertsLease(store, 'pty-new') + + expect(liveLeaseIdsForPane(store)).toEqual(['pty-new']) + expect( + store.getWorkspaceSession(SSH_PARTITION).terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF] + ).toBe(appPtyId('pty-old')) + }) +}) + describe('STA-3077: a pane moved between tabs still supersedes its own predecessor', () => { // The reported cardinality growth, in its surviving form. A lease freezes its tabId at write // time. Break the pane out into a new tab and its next lease carries the NEW tab, so matching @@ -314,7 +337,10 @@ describe('STA-3077 step P: the desktop plane resolves from its own home', () => rendererPublishesPane(store, 'pty-1') expect(relayReattachBindsPane(store, 'pty-2')).toBe(true) - // The other plane still names the predecessor, and is deliberately left alone. + // The other plane names the predecessor. It does not get a vote here — local speaking about + // this leaf IS the desktop plane's live view, and letting the other copy outrank it is the + // STA-3077 defect. Its binding to the loser is then scrubbed with every other reference to + // that retired lease, so the two planes do not drift further apart. somethingRewritesTheSshPartition(store, 'pty-1') sshSpawnUpsertsLease(store, 'pty-2')