From 22b7ac0d9b8bc0691d58ee7ef864e4f7c71131c7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 11 Aug 2026 23:23:42 -0700 Subject: [PATCH] refactor(ssh): delete the pane-binding fold; its premise was false MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The migration moved legacy `ssh:` pane bindings into `local` and cleared them, on the theory that the ssh partition was a stale spill of the desktop plane's state. Investigation of both hypotheses the team raised disproved that: - NOT cross-version compat. The introducing commit says it is for CONCURRENT multi-host. Nothing about the partition crosses the wire (zero references under src/main/runtime/rpc/), and the payload that does cross the SSH boundary — RemoteWorkspaceSnapshot — is projected from `local`. The only downgrade-compat comment protects `local`, the other direction. - NOT multi-client. PersistedState is one file on one machine; phones and CLI are RPC clients into that same process. Orca's real per-client state is `mobileClientTabSelectionsByDeviceId`, 13 lines above in the same struct, and it carries selections only — never a ptyId. What it actually is: the headless/CLI/mobile plane's OWN home, written and read deliberately across several tickets (STA-3463, STA-3465), with tests that assert that partition by name. So the fold was not tidying a spill — it was erasing another plane's live state. Measured symptom: a split SSH tab would disappear from mobile while its shell kept running. The desktop plane's fix never needed it. Supersession multiplied panes because `durablyBoundPtyIdForPane` hedged into the other plane's copy; reading `local` alone is the fix, and it stands without any migration. Deleting rather than redesigning, because the redesign had no target: five review rounds each found a defect in that function, three of them inside the previous round's fix, and every one was a cell of a merge matrix that only existed to serve a premise that was false. The `boundPtyIdsAcrossPartitions` clause is INVERTED, not dropped. It required the two partitions to AGREE after load — which encoded the false premise, and is what made a migration look necessary. It now asserts the narrower, stronger property: what the desktop plane resolves follows `local` alone, whatever the other plane holds. Also proven, and the reason unification was NOT attempted: worktreeId is `::` where repoId is a randomUUID minted client-side at repo add (orca-runtime.ts:18722), so two servers cannot collide on one. Host scoping was not protecting against that here — but the two planes' opposite choices are each deliberate and each test-pinned, so choosing a winner is an architecture call, not a cleanup. Net production: -75 lines. --- src/main/persistence.ts | 83 +---------- src/main/ssh-pane-binding-partition.test.ts | 144 ++------------------ 2 files changed, 18 insertions(+), 209 deletions(-) diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 48b90fb52f3..e9e07cf3771 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -2859,8 +2859,6 @@ export class Store { const loaded = this.load() const normalized = normalizePersistedPaneIdentityState(loaded) this.state = normalized.state - // After the leaf remap, so every folded binding keys on a UUID. - const foldedSshPaneBindings = this.foldSshPaneBindingsIntoLocalPartition() // Why: activeView is a frequent, tiny preference; keeping it beside the // profile avoids serializing the multi-MB recovery store on navigation. this.activeViewPreference = new ActiveViewPreference(this.dataFile, this.state.ui?.activeView) @@ -2879,7 +2877,7 @@ export class Store { this.state.legacyPaneKeyAliasEntries = entries this.scheduleSave() }) - if (normalized.changed || this.loadNeedsSave || adaptedProjectGroups || foldedSshPaneBindings) { + if (normalized.changed || this.loadNeedsSave || adaptedProjectGroups) { // Why: rewrite legacy pane:1 leaves so older renderer writes can't revive them; other migrations also set loadNeedsSave. this.scheduleSave() } @@ -7356,82 +7354,9 @@ export class Store { * * Returns the number of leases retired, for logging. */ - /** - * One-time fold: SSH pane bindings written to an `ssh:` partition by an older build move - * into `local`, the partition the renderer has always published pane membership to. Two homes is - * the STA-3077 defect itself — the reattach writer updated one and supersession read the other. - * `local` wins on conflict because it is the live publisher; the loser is dropped rather than - * demoted to an orphan candidate, since relay pty ids recycle and cannot identify a shell alone. - */ - private foldSshPaneBindingsIntoLocalPartition(): boolean { - const partitions = this.state.workspaceSessionsByHostId ?? {} - const local = this.state.workspaceSession - if (!local) { - // Nothing to fold into, and defaulting one here would mutate state on a path that reports no - // change — so the save would never be scheduled for it. - return false - } - let changed = false - for (const hostId of Object.keys(partitions)) { - if (parseExecutionHostId(hostId)?.kind !== 'ssh') { - continue - } - const host: WorkspaceSessionState | undefined = partitions[hostId] - const incarnations = host?.terminalPtyIncarnationsByPaneKey - for (const [tabId, layout] of Object.entries(host?.terminalLayoutsByTabId ?? {})) { - const bindings = layout.ptyIdsByLeafId - if (!bindings || Object.keys(bindings).length === 0) { - continue - } - const localLayout = local.terminalLayoutsByTabId?.[tabId] - if (!localLayout) { - // No local layout owns this tab, so there is no second home to disagree with and nothing - // to fold. Clearing here would delete the only record of the binding. - continue - } - const localBindings = localLayout.ptyIdsByLeafId ?? {} - // An incarnation is the other half of its binding's fence, so no binding is cleared without - // its fence being resolved in the same pass — one of three ways, by which pty ends up - // bound. (An incarnation whose binding was already missing before the fold is left alone: - // it is a pre-existing orphan, isolated in this partition, and not ours to reinterpret.) - for (const [leafId, ptyId] of Object.entries(bindings)) { - const paneKey = `${tabId}:${leafId}` - const incarnationId = incarnations?.[paneKey] - const localPtyId = localBindings[leafId] - if (!localPtyId) { - // This binding moves, so local's fence for the leaf becomes this partition's — whatever - // it is, INCLUDING absent. A local incarnation with no local binding is a leftover, and - // leaving it in place would fence the arriving pty with a value never written beside it. - local.terminalPtyIncarnationsByPaneKey ??= {} - if (incarnationId) { - local.terminalPtyIncarnationsByPaneKey[paneKey] = incarnationId - } else { - delete local.terminalPtyIncarnationsByPaneKey[paneKey] - } - } else if (incarnationId && localPtyId === ptyId) { - // Same shell from both sides; keep whichever fence local already holds. - local.terminalPtyIncarnationsByPaneKey ??= {} - local.terminalPtyIncarnationsByPaneKey[paneKey] ??= incarnationId - } - // The remaining case is local winning with a DIFFERENT pty. This incarnation belongs to - // the binding that just lost, so it is dropped rather than moved: pairing it with local's - // pty would synthesize a pair no writer produced, and leaving it here would orphan it - // from a binding that no longer exists. - if (incarnationId) { - delete incarnations![paneKey] - } - } - localLayout.ptyIdsByLeafId = { ...bindings, ...localBindings } - layout.ptyIdsByLeafId = {} - changed = true - } - } - return changed - } - - /** The PTY a pane is durably bound to. SSH pane bindings live in the local partition only — - * see `foldSshPaneBindingsIntoLocalPartition`. Hedging across two partitions is what let a - * stale `ssh:` copy outvote the live binding and no-op supersession (STA-3077). */ + /** The PTY a pane is durably bound to. The desktop plane's home is `local` — the renderer is its + * only publisher of pane membership. Hedging into `ssh:` let the headless plane's copy + * outvote the live binding and silently no-op supersession (STA-3077). */ private durablyBoundPtyIdForPane( targetId: string, tabId: string, diff --git a/src/main/ssh-pane-binding-partition.test.ts b/src/main/ssh-pane-binding-partition.test.ts index c2d69e9c757..95db734ae39 100644 --- a/src/main/ssh-pane-binding-partition.test.ts +++ b/src/main/ssh-pane-binding-partition.test.ts @@ -136,19 +136,6 @@ function liveLeaseIdsForPane(store: TestStore): string[] { .sort() } -/** Every pty id this one pane is durably bound to, across every partition. */ -function boundPtyIdsAcrossPartitions(store: TestStore): string[] { - const partitions = [LOCAL_EXECUTION_HOST_ID, SSH_PARTITION] - return Array.from( - new Set( - partitions - .map((hostId) => store.getWorkspaceSession(hostId).terminalLayoutsByTabId?.[TAB]) - .map((layout) => layout?.ptyIdsByLeafId?.[LEAF]) - .filter((ptyId): ptyId is string => typeof ptyId === 'string') - ) - ).sort() -} - /** The argument text of every `callee(...)` call in a production source file. */ function callArgumentsIn(source: string, callee: string): string[] { const calls: string[] = [] @@ -238,129 +225,26 @@ describe('STA-3077 step P: one pane, one live claim across partitions', () => { }) }) -describe('STA-3077 step P: the pane binding has one home', () => { - // Loading legacy state must fold the SSH partition's pane bindings into the - // one partition every reader consults; two homes is the defect itself. - it('resolves one pane to one durable pty id after loading legacy state', async () => { +describe('STA-3077 step P: the desktop plane resolves from its own home', () => { + // INVERTED. This clause used to require the two partitions to AGREE after load, which assumed + // `ssh:` was a stale spill of this plane's state. It is not — it is the headless/CLI + // plane's own home, which that plane writes and reads deliberately (STA-3463, STA-3465). The + // property that actually matters is narrower and stronger: what the desktop plane resolves must + // follow `local` alone, whatever the other plane holds. Asserting agreement made a migration look + // necessary that in fact erased another plane's live state. + it('resolves to the local binding regardless of what the ssh partition holds', async () => { const store = await createStore(diskAfterEarlierSession('pty-1')) rendererPublishesPane(store, 'pty-1') expect(relayReattachBindsPane(store, 'pty-2')).toBe(true) - - expect(boundPtyIdsAcrossPartitions(store)).toEqual([appPtyId('pty-2')]) - }) -}) - -describe('STA-3077 step P: the fold never destroys a binding it cannot move', () => { - // A tab that exists only in the ssh partition has no second home to disagree with, so there is - // nothing to fold. Clearing it anyway would delete the only record that pane was ever bound. - it('leaves an ssh-only tab binding intact', async () => { - const store = await createStore({ - workspaceSession: { ...getDefaultWorkspaceSession(), terminalLayoutsByTabId: {} }, - workspaceSessionsByHostId: { [SSH_PARTITION]: paneSession('pty-1') } - }) + // The other plane still names the predecessor, and is deliberately left alone. + somethingRewritesTheSshPartition(store, 'pty-1') + sshSpawnUpsertsLease(store, 'pty-2') expect( - store.getWorkspaceSession(SSH_PARTITION).terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF] - ).toBe(appPtyId('pty-1')) - }) -}) - -describe('STA-3077 step P: the fold carries the whole fence', () => { - // `persistPtyBinding` writes the binding and its incarnation into the SAME partition, and the - // incarnation is what its CAS compares. Folding only the binding would move the guard's subject - // to `local` while the guard itself stayed in the partition nothing reads — the CAS would then - // compare undefined against undefined and pass for any incarnation. - it('folds the pane incarnation along with the binding it fences', async () => { - const sshSession = paneSession('pty-1') as unknown as { - terminalPtyIncarnationsByPaneKey: Record - } - sshSession.terminalPtyIncarnationsByPaneKey = { [`${TAB}:${LEAF}`]: 'inc-pty-1' } - const store = await createStore({ - workspaceSession: paneSession('pty-1'), - workspaceSessionsByHostId: { [SSH_PARTITION]: sshSession } - }) - - expect(store.getWorkspaceSession().terminalPtyIncarnationsByPaneKey?.[`${TAB}:${LEAF}`]).toBe( - 'inc-pty-1' - ) - expect( - store.getWorkspaceSession(SSH_PARTITION).terminalPtyIncarnationsByPaneKey?.[`${TAB}:${LEAF}`] - ).toBeUndefined() - }) -}) - -describe('STA-3077 step P: the fold never synthesizes a pair that was never written', () => { - // The two partitions can name DIFFERENT ptys for one leaf — that divergence is the defect being - // migrated. Local wins the binding, so the losing partition's incarnation must not follow it: - // (local pty, superseded incarnation) is a pair no writer ever produced, and persistPtyBinding's - // CAS compares both, so it would refuse the pane's next legitimate update. - it('leaves the superseded incarnation behind when local wins with a different pty', async () => { - const sshSession = paneSession('pty-1') as unknown as { - terminalPtyIncarnationsByPaneKey: Record - } - sshSession.terminalPtyIncarnationsByPaneKey = { [`${TAB}:${LEAF}`]: 'inc-pty-1' } - const store = await createStore({ - workspaceSession: paneSession('pty-2'), - workspaceSessionsByHostId: { [SSH_PARTITION]: sshSession } - }) - - const local = store.getWorkspaceSession() - expect(local.terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF]).toBe(appPtyId('pty-2')) - expect(local.terminalPtyIncarnationsByPaneKey?.[`${TAB}:${LEAF}`]).not.toBe('inc-pty-1') - // Not merely "not copied" — the losing binding is cleared, so leaving its incarnation here - // would strand a fence with nothing to fence. - expect( - store.getWorkspaceSession(SSH_PARTITION).terminalPtyIncarnationsByPaneKey?.[`${TAB}:${LEAF}`] - ).toBeUndefined() - }) - - // Case 7 of the fold matrix, and the one two independent reviewers both landed on. A moving - // binding with NO incarnation of its own must not inherit local's leftover: (arriving pty, - // unrelated incarnation) is a pair no writer produced, and persistPtyBinding's CAS compares both, - // so it would refuse this pane's next legitimate update. - it('does not pair a moving binding with a local incarnation when it has none of its own', async () => { - const paneKey = `${TAB}:${LEAF}` - const localSession = paneSession('pty-1') as unknown as { - terminalLayoutsByTabId: Record }> - terminalPtyIncarnationsByPaneKey: Record - } - localSession.terminalLayoutsByTabId[TAB]!.ptyIdsByLeafId = {} - localSession.terminalPtyIncarnationsByPaneKey = { [paneKey]: 'inc-orphaned' } - const store = await createStore({ - workspaceSession: localSession, - // The ssh partition carries the binding but never an incarnation for it. - workspaceSessionsByHostId: { [SSH_PARTITION]: paneSession('pty-9') } - }) - - const local = store.getWorkspaceSession() - expect(local.terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF]).toBe(appPtyId('pty-9')) - expect(local.terminalPtyIncarnationsByPaneKey?.[paneKey]).toBeUndefined() - }) - - // A local incarnation with no local binding is a leftover, not a fence. When the ssh binding is - // the one that survives, its own incarnation must come with it and outrank that leftover — - // otherwise the surviving pty is fenced by a value never written beside it. - it('lets a moving binding overwrite a local incarnation that fences nothing', async () => { - const paneKey = `${TAB}:${LEAF}` - const sshSession = paneSession('pty-1') as unknown as { - terminalPtyIncarnationsByPaneKey: Record - } - sshSession.terminalPtyIncarnationsByPaneKey = { [paneKey]: 'inc-pty-1' } - const localSession = paneSession('pty-1') as unknown as { - terminalLayoutsByTabId: Record }> - terminalPtyIncarnationsByPaneKey: Record - } - localSession.terminalLayoutsByTabId[TAB]!.ptyIdsByLeafId = {} - localSession.terminalPtyIncarnationsByPaneKey = { [paneKey]: 'inc-orphaned' } - const store = await createStore({ - workspaceSession: localSession, - workspaceSessionsByHostId: { [SSH_PARTITION]: sshSession } - }) - - const local = store.getWorkspaceSession() - expect(local.terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF]).toBe(appPtyId('pty-1')) - expect(local.terminalPtyIncarnationsByPaneKey?.[paneKey]).toBe('inc-pty-1') + store.getWorkspaceSession().terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF] + ).toBe(appPtyId('pty-2')) + expect(liveLeaseIdsForPane(store)).toEqual(['pty-2']) }) })