From a2eb9909e84fca374171b2a7d13c34079536beee Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 13 Aug 2026 19:49:15 -0700 Subject: [PATCH] fix(ssh): stamp the hoist instead of deleting, and normalize what it folds in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the partition delete with a one-shot settings stamp. The delete broke five deliberate tests: ssh: partitions are legitimately owned by the headless and CLI planes, and destroying them also made a rollback unable to recover those tabs. The stamp stops the re-hoist without touching the data. Moves the hoist BEFORE pane-identity normalization. A partition can carry a legacy non-UUID leaf; injecting one raw skips the normalizer that exists for exactly that, the renderer then mints a fresh UUID on hydrate, the binding lookup misses, and that pane hits the same tab loss this change exists to fix. The earlier claim that normalization prunes partitions was false — those attempts failed because the test fixture was schema-invalid, not because of ordering. Also narrows isPtyProvenGoneForReplacement to the minted expiry (P1-a): a raw SSH_PTY_EXITED is one the reattach could NOT tie to this shell, and honouring it retired the owner and respawned onto a shell still in use. Also prunes expired SSH leases older than 7 days at load (P1-d): they are unreachable from every reattach path and previously grew for the life of the profile. Retention sits far above the 30s pane-recovery grace window, the only remaining reader. --- src/main/ipc/pty.ts | 8 ++-- src/main/persistence.ts | 47 +++++++++++++------ .../ssh-partition-upgrade-tab-loss.test.ts | 16 ++++++- src/shared/types.ts | 2 + 4 files changed, 55 insertions(+), 18 deletions(-) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 0bf1e54ec61..321632f8568 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -1121,10 +1121,12 @@ function isPtyProvenGoneForReplacement(err: unknown, ptyId: string): boolean { if (!parseAppSshPtyId(ptyId)) { return isPtyAlreadyGoneError(err) } - // For SSH, the two proving answers are the relay's own observed exit and the expiry the reattach - // mints only after verifying that proof names this shell. A bare not-found is neither. + // The ONLY proving answer is the expiry the reattach mints after verifying the relay's proof + // names this shell and this incarnation. Accepting a bare SSH_PTY_EXITED too was over-acceptance: + // the reattach deliberately rethrows one it could NOT tie to us, and honouring that here retires + // the owner and respawns onto a shell somebody else is still using. const message = err instanceof Error ? err.message : String(err) - return isSshPtyExitedError(err) || message.includes(SSH_SESSION_EXPIRED_ERROR) + return message.includes(SSH_SESSION_EXPIRED_ERROR) } function isPtyAlreadyGoneError(err: unknown): boolean { diff --git a/src/main/persistence.ts b/src/main/persistence.ts index cbe99c70171..44da7320035 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -2850,6 +2850,10 @@ export type PtyBindingSourceExpectation = { } +/** 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 + /** * Fold SSH pane membership left in `workspaceSessionsByHostId["ssh:"]` into the local * session, which is the only plane the binding writer maintains. @@ -2864,6 +2868,9 @@ export function hoistSshPartitionsIntoLocalSession( /** Passed explicitly: pane-identity normalization prunes partitions before this runs. */ sourcePartitions?: PersistedState['workspaceSessionsByHostId'] ): boolean { + if (state.settings?.sshPartitionHoistSeed === 'done') { + return false + } const partitions = sourcePartitions ?? state.workspaceSessionsByHostId if (!partitions) { return false @@ -2913,16 +2920,12 @@ export function hoistSshPartitionsIntoLocalSession( local.activeTabIdByWorktree = { ...local.activeTabIdByWorktree, [worktreeId]: tabId } changed = true } - // Consume the source. A migration that leaves it behind runs again every launch and - // resurrects panes the user has since closed, because "local has no such tab" is exactly - // what this function treats as "needs hoisting". - if (state.workspaceSessionsByHostId?.[hostId]) { - const remaining = { ...state.workspaceSessionsByHostId } - delete remaining[hostId] - state.workspaceSessionsByHostId = remaining - changed = true - } } + // Mark it done even when nothing moved. The partition itself must SURVIVE — headless and CLI + // own panes in that plane — so the only way to stop re-hoisting is to record that we have run. + // Without this, "local has no such tab" is both the hoist condition and the state produced by + // the user closing a tab, so closed panes came back on the next launch. + state.settings = { ...state.settings, sshPartitionHoistSeed: 'done' } return changed } @@ -2984,13 +2987,15 @@ export class Store { fallbackSnapshotRoot: legacySnapshotRoot === profileSnapshotRoot ? null : legacySnapshotRoot } const loaded = this.load() - const normalized = normalizePersistedPaneIdentityState(loaded) - this.state = normalized.state // Why: earlier builds wrote SSH pane membership into `ssh:` partitions; this build's // binding writer is local-only and its reattach refuses to create, so an unhoisted partition - // means every pane is refused and the user's tabs are discarded. Runs after pane-identity - // normalization, which rebuilds the session and would otherwise drop the hoisted panes. - if (hoistSshPartitionsIntoLocalSession(this.state, this.sshPartitionsAtLoad)) { + // means every pane is refused and the user's tabs are discarded. Runs BEFORE pane-identity + // normalization so hoisted panes are normalized with everything else — a partition can carry + // a legacy non-UUID leaf, and injecting one raw re-creates the same tab loss for that pane. + const hoisted = hoistSshPartitionsIntoLocalSession(loaded, this.sshPartitionsAtLoad) + const normalized = normalizePersistedPaneIdentityState(loaded) + this.state = normalized.state + if (hoisted) { this.loadNeedsSave = true } // Why: activeView is a frequent, tiny preference; keeping it beside the @@ -4002,6 +4007,20 @@ export class Store { this.loadNeedsSave = true } + // Why: an expired lease is unreachable from every reattach path, and only host deletion ever + // 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( + (lease) => + lease.state !== 'expired' || + !lease.updatedAt || + Date.now() - lease.updatedAt <= EXPIRED_SSH_LEASE_RETENTION_MS + ) + if (retainedLeases.length !== (result.sshRemotePtyLeases ?? []).length) { + result = { ...result, sshRemotePtyLeases: retainedLeases } + this.loadNeedsSave = true + } + if (gcStaleWorktreeMeta(result) > 0) { this.loadNeedsSave = true } diff --git a/src/main/ssh-partition-upgrade-tab-loss.test.ts b/src/main/ssh-partition-upgrade-tab-loss.test.ts index 64feef17978..e4c11fcc2a0 100644 --- a/src/main/ssh-partition-upgrade-tab-loss.test.ts +++ b/src/main/ssh-partition-upgrade-tab-loss.test.ts @@ -8,7 +8,7 @@ * survivors blank", and no oracle covered it because every test writes both planes consistently. */ import { beforeEach, describe, expect, it, vi } from 'vitest' -import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs' +import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs' import { join } from 'node:path' import { tmpdir } from 'node:os' import { getDefaultPersistedState, getDefaultWorkspaceSession } from '../shared/constants' @@ -44,6 +44,17 @@ async function createStore(state: Record = {}) { return new Store() } +/** A pre-upgrade profile has the partition and no hoist marker; the marker only exists because + * this build writes it. Stripping it models the on-disk state an upgrading user actually has. */ +function stripHoistMarker(): void { + const file = join(testState.dir, 'orca-data.json') + const state = JSON.parse(readFileSync(file, 'utf-8')) as { + settings?: { sshPartitionHoistSeed?: string } + } + delete state.settings?.sshPartitionHoistSeed + writeFileSync(file, JSON.stringify(state), 'utf-8') +} + /** Reopen the SAME profile — the upgrade launch. The hoist migration runs at load, so a test that * writes the partition into an already-loaded store would never exercise it. */ async function reopenStore() { @@ -100,6 +111,7 @@ describe('an SSH pane whose membership was persisted into the ssh partition', () ).toHaveLength(1) expect(before.getWorkspaceSession().tabsByWorktree?.[WORKTREE] ?? []).toHaveLength(0) before.flushOrThrow() + stripHoistMarker() // The upgrade launch. const store = await reopenStore() @@ -131,6 +143,7 @@ describe('an SSH pane whose membership was persisted into the ssh partition', () const before = await createStore() before.setWorkspaceSession(paneSession() as never, SSH_PARTITION) before.flushOrThrow() + stripHoistMarker() const store = await reopenStore() const bound = store.persistPtyBinding({ @@ -164,6 +177,7 @@ describe('an SSH pane whose membership was persisted into the ssh partition', () const before = await createStore() before.setWorkspaceSession(paneSession() as never, SSH_PARTITION) before.flushOrThrow() + stripHoistMarker() // The upgrade launch hoists the pane. const upgraded = await reopenStore() diff --git a/src/shared/types.ts b/src/shared/types.ts index b2ebefa57a5..710da60e962 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -3210,6 +3210,8 @@ export type GlobalSettings = { /** One-shot cohort marker for the tab-switch keybinding swap. 'pending' = * pre-existing install (seed pins old chords, then flips to 'done'); 'done' = fresh install. */ tabSwitchKeybindingSeed?: 'pending' | 'done' + /** One-shot: SSH host session partitions have been folded into the local session. */ + sshPartitionHoistSeed?: 'done' /** Local voice/dictation config. Optional for pre-voice profiles; getDefaultSettings() hydrates defaults via the persistence merge. */ voice?: VoiceSettings }