From 5dfcebb2ff3db6b049ee9b96f2c0f4d107ae4794 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 27 Aug 2026 22:12:56 -0700 Subject: [PATCH] fix(terminal): a restarted relay must not hand a new tab a dead PTY's exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A redeployed SSH relay renumbers its PTY ids from pty-1, so a freshly spawned PTY can be handed an id a dead one used to own. The renderer's pre-handler buffer is keyed on that id alone, so `registerExit` found the dead PTY's buffered exit and reported the brand-new shell as `exitedBeforeAttach` — the pane never bound a PTY and the tab came up blank forever. Date every buffered record with a monotonic sequence and fence a fresh spawn against it: state recorded before the spawn request left the renderer belongs to the id's earlier owner. State recorded after it is kept, so the real pre-attach race (a shell that dies instantly, or writes before the pane registers its handler) still works. Makes ssh-lost-kill-tab-resurrection.spec.ts:178 pass; it failed in SETUP. --- .../terminal-pane/ipc-pty-connect.ts | 11 ++++ .../pty-pre-handler-buffer.test.ts | 51 ++++++++++++++++ .../terminal-pane/pty-pre-handler-buffer.ts | 59 +++++++++++++++++-- 3 files changed, 115 insertions(+), 6 deletions(-) diff --git a/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts b/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts index 7dc5647d8b8..c734ea6ccee 100644 --- a/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts +++ b/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts @@ -3,6 +3,8 @@ import { extractIpcErrorMessage } from '@/lib/ipc-error' import { ensurePtyDispatcher } from './pty-dispatcher' import { clearConsumedPreHandlerPtyExit, + currentPreHandlerPtySequence, + discardPreHandlerPtyStateFromPriorIncarnation, hasPreHandlerPtyExit, isPreHandlerPtyStateDiscarded } from './pty-pre-handler-buffer' @@ -68,6 +70,10 @@ export async function connectIpcPty( if (options.shouldContinue && !options.shouldContinue()) { return } + // Why read it before the request and not after: a redeployed SSH relay renumbers from pty-1, so + // this spawn can be handed an id a dead PTY used to own. State dated at or below this fence was + // recorded before we asked for a PTY, so it belongs to that earlier owner, not to us. + const priorIncarnationFence = currentPreHandlerPtySequence() const spawnResult = await spawnIpcPty(transportOptions, options, admittedSessionId) const retireFreshSpawn = async (): Promise => { if (!spawnResult.isReattach && !spawnResult.coldRestore) { @@ -87,6 +93,11 @@ export async function connectIpcPty( context.getCallbacks().onReattachDetermined?.() } + if (!admittedSessionId && !spawnResult.isReattach && !spawnResult.coldRestore) { + // Why only a fresh spawn: a reattach deliberately re-owns an id that already existed, so its + // buffered exit is the real thing. A fresh spawn's PTY did not exist yet. + discardPreHandlerPtyStateFromPriorIncarnation(spawnResult.id, priorIncarnationFence) + } context.bind(spawnResult.id) if (!spawnResult.isReattach && !spawnResult.coldRestore) { onPtySpawn?.(spawnResult.id) diff --git a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts index 19b22971b41..489b2f3cd24 100644 --- a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts @@ -5,9 +5,11 @@ import { clearConsumedPreHandlerPtyExit, clearPreHandlerPtyState, consumePreHandlerPtyState, + currentPreHandlerPtySequence, drainPreHandlerPtyData, drainPreHandlerPtyExit, discardPreHandlerPtyState, + discardPreHandlerPtyStateFromPriorIncarnation, hasPreHandlerPtyExit, replayPreHandlerPtyData } from './pty-pre-handler-buffer' @@ -16,6 +18,7 @@ const RESCAN_PTY_ID = 'pty-pre-handler-rescan' const TRIM_PTY_ID = 'pty-pre-handler-trim' const EXIT_PTY_ID = 'pty-pre-handler-exit' const CAPPED_EXIT_PTY_IDS = Array.from({ length: 65 }, (_, index) => `pty-capped-exit-${index}`) +const RECYCLED_PTY_ID = 'ssh:target@@pty-2' describe('pre-handler PTY buffer', () => { afterEach(() => { @@ -25,6 +28,7 @@ describe('pre-handler PTY buffer', () => { for (const ptyId of CAPPED_EXIT_PTY_IDS) { clearPreHandlerPtyState(ptyId) } + clearPreHandlerPtyState(RECYCLED_PTY_ID) }) it('does not rescan historical chunks while buffering small startup output', () => { @@ -187,4 +191,51 @@ describe('pre-handler PTY buffer', () => { expect(exits).not.toContain(0) expect(exits).toContain(64) }) + + // A redeployed SSH relay renumbers from pty-1, so a fresh spawn is handed an id whose dead owner + // left an exit here. Applying it reports the brand-new shell as already exited and the pane never + // binds a PTY at all — the tab comes up blank forever. + it("drops a recycled id's exit recorded before the fresh spawn was requested", () => { + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 0) + bufferPreHandlerPtyData(RECYCLED_PTY_ID, 'output from the dead shell') + + const fence = currentPreHandlerPtySequence() + discardPreHandlerPtyStateFromPriorIncarnation(RECYCLED_PTY_ID, fence) + + const exit = vi.fn() + const data = vi.fn() + expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(false) + drainPreHandlerPtyExit(RECYCLED_PTY_ID, exit) + drainPreHandlerPtyData(RECYCLED_PTY_ID, data) + expect(exit).not.toHaveBeenCalled() + expect(data).not.toHaveBeenCalled() + }) + + it('keeps state recorded after the fence, so a shell that dies instantly still reports its exit', () => { + const fence = currentPreHandlerPtySequence() + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 1) + bufferPreHandlerPtyData(RECYCLED_PTY_ID, 'startup bytes') + + discardPreHandlerPtyStateFromPriorIncarnation(RECYCLED_PTY_ID, fence) + + const exit = vi.fn() + const data = vi.fn() + expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(true) + // Data first: draining the exit transfers ownership and clears the buffered bytes with it. + drainPreHandlerPtyData(RECYCLED_PTY_ID, data) + drainPreHandlerPtyExit(RECYCLED_PTY_ID, exit) + expect(exit).toHaveBeenCalledWith(1) + expect(data).toHaveBeenCalledWith('startup bytes', undefined) + }) + + it('re-admits exits for a recycled id whose prior incarnation was consumed', () => { + consumePreHandlerPtyState(RECYCLED_PTY_ID) + + discardPreHandlerPtyStateFromPriorIncarnation(RECYCLED_PTY_ID, currentPreHandlerPtySequence()) + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 3) + + const exit = vi.fn() + drainPreHandlerPtyExit(RECYCLED_PTY_ID, exit) + expect(exit).toHaveBeenCalledWith(3) + }) }) diff --git a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts index 46171d40fda..9e2f4d3e929 100644 --- a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts +++ b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts @@ -11,10 +11,17 @@ type BufferedPreHandlerPtyState = { chunks: BufferedPreHandlerPtyData[] head: number bytes: number + /** Sequence of the newest chunk, used to fence state left by a prior incarnation of a reused id. */ + sequence: number +} + +type BufferedPreHandlerPtyExit = { + code: number + sequence: number } const preHandlerPtyData = new Map() -const preHandlerPtyExit = new Map() +const preHandlerPtyExit = new Map() const consumedPreHandlerPtyExits = new Map() const discardedPreHandlerPtyStates = new Map>() const DISCARDED_PRE_HANDLER_PTY_STATE_TTL_MS = 60_000 @@ -31,6 +38,21 @@ const PRE_HANDLER_PTY_EXIT_MAX_PTYS = 64 const PRE_HANDLER_PTY_DATA_WARN_BYTES = 64 * 1024 const warnedLostHandlerPtyIds = new Set() +// Why: pty ids are NOT unique over time — a redeployed SSH relay renumbers from pty-1, so a fresh +// spawn can be handed an id whose previous incarnation left buffered state here. A monotonic +// counter dates every record so a spawn can tell "arrived before I asked for this PTY" (someone +// else's) from "arrived while it was starting" (mine). +let preHandlerPtySequence = 0 + +function nextPreHandlerPtySequence(): number { + preHandlerPtySequence += 1 + return preHandlerPtySequence +} + +export function currentPreHandlerPtySequence(): number { + return preHandlerPtySequence +} + export function bufferPreHandlerPtyData(ptyId: string, data: string, meta?: PtyDataMeta): void { if (discardedPreHandlerPtyStates.has(ptyId)) { return @@ -51,9 +73,10 @@ export function bufferPreHandlerPtyData(ptyId: string, data: string, meta?: PtyD : meta let state = preHandlerPtyData.get(ptyId) if (!state) { - state = { chunks: [], head: 0, bytes: 0 } + state = { chunks: [], head: 0, bytes: 0, sequence: 0 } preHandlerPtyData.set(ptyId, state) } + state.sequence = nextPreHandlerPtySequence() state.chunks.push({ data: chunk.data, bytes: chunk.bytes, @@ -117,7 +140,31 @@ export function bufferPreHandlerPtyExit(ptyId: string, code: number): void { preHandlerPtyExit.delete(oldestPtyId) } } - preHandlerPtyExit.set(ptyId, code) + preHandlerPtyExit.set(ptyId, { code, sequence: nextPreHandlerPtySequence() }) +} + +/** Drop pre-handler state a freshly spawned PTY inherited from an earlier owner of its id. + * + * `fenceSequence` is read before the spawn request leaves the renderer, so anything at or below it + * was recorded when this PTY did not yet exist and cannot describe it. Bytes and exits recorded + * after the fence are kept: that is the real pre-attach race (a shell that dies instantly, or + * writes before the pane registers its handler) and losing it would blank a legitimate pane. */ +export function discardPreHandlerPtyStateFromPriorIncarnation( + ptyId: string, + fenceSequence: number +): void { + const exit = preHandlerPtyExit.get(ptyId) + if (exit && exit.sequence <= fenceSequence) { + preHandlerPtyExit.delete(ptyId) + } + const data = preHandlerPtyData.get(ptyId) + if (data && data.sequence <= fenceSequence) { + preHandlerPtyData.delete(ptyId) + warnedLostHandlerPtyIds.delete(ptyId) + } + // Why: the id now names a different, live PTY, so a prior incarnation's consumed/discarded marks + // must not suppress this one's own exit — the same admission boundary a same-id reattach gets. + clearConsumedPreHandlerPtyExit(ptyId) } // Why: primary handlers and pane-less parked owners have fully handled this @@ -173,13 +220,13 @@ export function hasPreHandlerPtyExit(ptyId: string): boolean { } export function drainPreHandlerPtyExit(ptyId: string, handler: (code: number) => void): void { - const code = preHandlerPtyExit.get(ptyId) - if (code === undefined) { + const exit = preHandlerPtyExit.get(ptyId) + if (exit === undefined) { return } preHandlerPtyExit.delete(ptyId) try { - handler(code) + handler(exit.code) } finally { // Why: draining transfers ownership to this handler. Even when it throws, // a duplicate exit must not become a new pre-handler event.