From 38f660f65ca1adfcfa37e0e420e88c1cb74b7b01 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 13 Aug 2026 20:15:21 -0700 Subject: [PATCH] fix(ssh): compare the relay id in the exit proof, and stop requiring a layout to hoist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1 (P1): isPtyProvenGoneForReplacement compared the proof's id against the APP-scoped ptyId, but the relay formats the RELAY-scoped one it was sent. The comparison could never be true, so the previous commit's 'accept a proof naming this shell' was behaviourally a blanket refusal — the exact stranding it was written to fix. Now compares parsedPtyId.relayPtyId. The function had ZERO tests, which is how an always-false branch survived two review rounds; it is exported and covered now, including the mutation that reinstates the app-id comparison. F2 (P1): requiring a layout to hoist a tab re-created the original tab loss for the CLI/headless cohort. A background or CLI-created tab persists with tab.ptyId and no layout entry until its pane first mounts, and hydration falls back to tab.ptyId for exactly that shape. The orphan-layout direction was already handled independently by the hasTab guard, so the tab-side clause bought nothing. --- .../pty-proven-gone-for-replacement.test.ts | 49 +++++++++++++++++++ src/main/ipc/pty.ts | 11 +++-- src/main/persistence.ts | 14 +++--- .../ssh-partition-upgrade-tab-loss.test.ts | 21 +++++++- 4 files changed, 83 insertions(+), 12 deletions(-) create mode 100644 src/main/ipc/pty-proven-gone-for-replacement.test.ts diff --git a/src/main/ipc/pty-proven-gone-for-replacement.test.ts b/src/main/ipc/pty-proven-gone-for-replacement.test.ts new file mode 100644 index 00000000000..43f2a8b8f6d --- /dev/null +++ b/src/main/ipc/pty-proven-gone-for-replacement.test.ts @@ -0,0 +1,49 @@ +/** + * `isPtyProvenGoneForReplacement` decides whether a failure authorizes destroying a pane's shell + * and spawning a replacement. It had no test, and an always-false id comparison shipped through + * two review rounds because of it — the relay names a PTY by its RELAY id while the caller holds + * the APP-scoped one. + */ +import { describe, expect, it } from 'vitest' +import { isPtyProvenGoneForReplacement } from './pty' +import { formatPtyExitedError, SSH_SESSION_EXPIRED_ERROR } from '../../shared/ssh-pty-failure-tokens' +import { toAppSshPtyId } from '../../shared/ssh-pty-id' + +const TARGET = 'ssh-target-1' +const RELAY_PTY = 'pty-7' +const APP_PTY = toAppSshPtyId(TARGET, RELAY_PTY) +const INCARNATION = '6f1c9a2e-7b4d-4e1a-9c8f-2d5e6a7b8c90' + +describe('what authorizes replacing a remote shell', () => { + it('accepts the expiry the reattach mints after verifying the proof', () => { + expect( + isPtyProvenGoneForReplacement(new Error(`boom ${SSH_SESSION_EXPIRED_ERROR}`), APP_PTY) + ).toBe(true) + }) + + // The regression: the relay formats the relay-scoped id, so comparing the app-scoped id was + // always false and every un-attested exit read as "not proven" — stranding the pane. + it('accepts a raw exit proof that names this shell by its relay id', () => { + expect( + isPtyProvenGoneForReplacement( + new Error(formatPtyExitedError(RELAY_PTY, 0, INCARNATION)), + APP_PTY + ) + ).toBe(true) + }) + + it('refuses a raw exit proof about a different shell', () => { + expect( + isPtyProvenGoneForReplacement( + new Error(formatPtyExitedError('pty-9', 0, INCARNATION)), + APP_PTY + ) + ).toBe(false) + }) + + it('refuses a bare not-found, which never proves a shell exited', () => { + expect(isPtyProvenGoneForReplacement(new Error(`PTY "${RELAY_PTY}" not found`), APP_PTY)).toBe( + false + ) + }) +}) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 338e2f6e3d1..aacfd9f04ce 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -1118,8 +1118,10 @@ function normalizeNodePtySpawnError(err: unknown): Error { * not-found proves an exit only from a provider that owns its ptys; a replacement relay answers the * same for shells its predecessor is still running. */ -function isPtyProvenGoneForReplacement(err: unknown, ptyId: string): boolean { - if (!parseAppSshPtyId(ptyId)) { +/** Exported for test: this had zero coverage, which is how an always-false id comparison shipped. */ +export function isPtyProvenGoneForReplacement(err: unknown, ptyId: string): boolean { + const parsedPtyId = parseAppSshPtyId(ptyId) + if (!parsedPtyId) { return isPtyAlreadyGoneError(err) } const message = err instanceof Error ? err.message : String(err) @@ -1134,7 +1136,10 @@ function isPtyProvenGoneForReplacement(err: unknown, ptyId: string): boolean { // panes whose exit really was proven and for which no incarnation exists to check. Require the // proof to name THIS pty: without an incarnation that is the only evidence there is, and an id // mismatch is exactly the case that must not authorize a respawn. - return parsePtyExitedError(message)?.id === ptyId + // Against the RELAY id: the relay formats `params.id`, which is what the caller sent it, and + // callers send the relay-scoped id. Comparing the app-scoped id here never matched, so this + // read as a blanket refusal — the stranding it exists to avoid. + return parsePtyExitedError(message)?.id === parsedPtyId.relayPtyId } function isPtyAlreadyGoneError(err: unknown): boolean { diff --git a/src/main/persistence.ts b/src/main/persistence.ts index e29f44a837a..8c5c8ebb63f 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -2889,15 +2889,13 @@ 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)) - // 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. + // Deliberately NOT requiring a layout: a background or CLI-created tab persists with + // `tab.ptyId` and no layout entry until its pane first mounts, and hydration falls back to + // `tab.ptyId` for exactly that shape. Refusing those stranded the CLI/headless cohort — + // the original tab loss, for the very users whose live writes this hoist exists to carry. + // The orphan-layout direction is handled independently by the `hasTab` guard below. const missing = (tabs ?? []).filter( - (tab) => - !known.has(tab.id) && - !alreadyHoisted.has(tab.id) && - Boolean(partition.terminalLayoutsByTabId?.[tab.id]) + (tab) => !known.has(tab.id) && !alreadyHoisted.has(tab.id) ) if (missing.length === 0) { continue diff --git a/src/main/ssh-partition-upgrade-tab-loss.test.ts b/src/main/ssh-partition-upgrade-tab-loss.test.ts index 32b9e15628a..9ac8508ab37 100644 --- a/src/main/ssh-partition-upgrade-tab-loss.test.ts +++ b/src/main/ssh-partition-upgrade-tab-loss.test.ts @@ -225,7 +225,7 @@ describe('an SSH pane whose membership was persisted into the ssh partition', () expandedLeafId: null, ptyIdsByLeafId: { [LEAF_TWO]: PTY } } - } + } as typeof second.terminalLayoutsByTabId upgraded.setWorkspaceSession(second as never, SSH_PARTITION) upgraded.flushOrThrow() @@ -255,4 +255,23 @@ describe('an SSH pane whose membership was persisted into the ssh partition', () 'an orphan layout was hoisted into local with no tab to reach it' ).toBeUndefined() }) + + // A background or CLI-created tab persists with `tab.ptyId` and no layout until its pane first + // mounts, and hydration falls back to `tab.ptyId` for that shape. Requiring a layout stranded + // exactly the CLI/headless cohort whose live writes this hoist exists to carry. + it('hoists a tab that has no layout yet', async () => { + const before = await createStore() + const layoutless = paneSession() + layoutless.terminalLayoutsByTabId = {} as typeof layoutless.terminalLayoutsByTabId + before.setWorkspaceSession(layoutless as never, SSH_PARTITION) + before.flushOrThrow() + stripHoistMarker() + + const store = await reopenStore() + + expect( + store.getWorkspaceSession().tabsByWorktree?.[WORKTREE] ?? [], + 'a tab whose pane had not mounted yet was stranded in the partition' + ).toHaveLength(1) + }) })