diff --git a/src/preload/api/pty-api.ts b/src/preload/api/pty-api.ts index fc558b60d74..7eee16ea522 100644 --- a/src/preload/api/pty-api.ts +++ b/src/preload/api/pty-api.ts @@ -48,6 +48,8 @@ export type PtyApi = { telemetry?: { agent_kind: AgentKind; launch_source: LaunchSource; request_kind: RequestKind } }) => Promise<{ id: string + /** Which lifetime of `id` this reply named; absent when the execution host predates the field. */ + incarnationId?: string launchAgent?: TuiAgent launchConfig?: SleepingAgentLaunchConfig snapshot?: string @@ -196,7 +198,13 @@ export type PtyApi = { /** Title-only replay snapshot for (re)attach; attention facts never replay. */ getSideEffectSnapshot: (id: string) => Promise onExit: ( - callback: (data: { id: string; code: number; preserveRendererBinding?: boolean }) => void + callback: (data: { + id: string + code: number + preserveRendererBinding?: boolean + /** Which lifetime of `id` died; absent when the execution host predates the field. */ + incarnationId?: string + }) => void ) => () => void onSpawned: (callback: (data: { id: string }) => void) => () => void onSerializeBufferRequest: ( diff --git a/src/preload/index.ts b/src/preload/index.ts index 1b75b5b56da..5bc62cbfe2e 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1058,6 +1058,8 @@ const api = { telemetry?: { agent_kind: AgentKind; launch_source: LaunchSource; request_kind: RequestKind } }): Promise<{ id: string + /** Which lifetime of `id` this reply named; absent when the execution host predates the field. */ + incarnationId?: string launchConfig?: SleepingAgentLaunchConfig snapshot?: string snapshotCols?: number @@ -1304,10 +1306,23 @@ const api = { ipcRenderer.invoke('pty:sideEffectSnapshot', { id }), onExit: ( - callback: (data: { id: string; code: number; preserveRendererBinding?: boolean }) => void + callback: (data: { + id: string + code: number + preserveRendererBinding?: boolean + /** Which lifetime of `id` died; absent when the execution host predates the field. */ + incarnationId?: string + }) => void ): (() => void) => { - const listener = (_event: Electron.IpcRendererEvent, data: { id: string; code: number }) => - callback(data) + const listener = ( + _event: Electron.IpcRendererEvent, + data: { + id: string + code: number + preserveRendererBinding?: boolean + incarnationId?: string + } + ) => callback(data) ipcRenderer.on('pty:exit', listener) return () => ipcRenderer.removeListener('pty:exit', listener) }, 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 c734ea6ccee..70d3a0fd047 100644 --- a/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts +++ b/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts @@ -4,6 +4,7 @@ import { ensurePtyDispatcher } from './pty-dispatcher' import { clearConsumedPreHandlerPtyExit, currentPreHandlerPtySequence, + discardPreHandlerPtyExitFromForeignIncarnation, discardPreHandlerPtyStateFromPriorIncarnation, hasPreHandlerPtyExit, isPreHandlerPtyStateDiscarded @@ -93,6 +94,10 @@ export async function connectIpcPty( context.getCallbacks().onReattachDetermined?.() } + // Why unconditional: this runs on identity, not timing. Whatever we attached to — fresh, + // reattach or cold restore — an exit naming a different incarnation of the id is not ours, so + // it is safe to drop even for the reattach the fence below deliberately leaves alone. + discardPreHandlerPtyExitFromForeignIncarnation(spawnResult.id, spawnResult.incarnationId) 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. @@ -103,7 +108,7 @@ export async function connectIpcPty( onPtySpawn?.(spawnResult.id) } handlers.registerData(spawnResult.id) - const exitedBeforeAttach = handlers.registerExit(spawnResult.id) + const exitedBeforeAttach = handlers.registerExit(spawnResult.id, spawnResult.incarnationId) if (exitedBeforeAttach) { return { id: spawnResult.id, exitedBeforeAttach: true } } diff --git a/src/renderer/src/components/terminal-pane/ipc-pty-session-handlers.ts b/src/renderer/src/components/terminal-pane/ipc-pty-session-handlers.ts index 8d73479687c..ad225b928bb 100644 --- a/src/renderer/src/components/terminal-pane/ipc-pty-session-handlers.ts +++ b/src/renderer/src/components/terminal-pane/ipc-pty-session-handlers.ts @@ -30,7 +30,7 @@ type IpcPtySessionHandlersOptions = { export type IpcPtySessionHandlers = { registerData: (id: string) => void - registerExit: (id: string) => boolean + registerExit: (id: string, incarnationId?: string) => boolean unregisterAll: (id: string) => void unregisterData: (id: string) => void clearAccumulatedState: () => void @@ -133,8 +133,8 @@ export function createIpcPtySessionHandlers({ } } - function registerExit(id: string): boolean { - const hadBufferedExit = hasPreHandlerPtyExit(id) + function registerExit(id: string, incarnationId?: string): boolean { + const hadBufferedExit = hasPreHandlerPtyExit(id, incarnationId) const exit = (code: number): void => { const currentId = getPtyId() if (currentId !== null && currentId !== id) { @@ -154,7 +154,7 @@ export function createIpcPtySessionHandlers({ ptyTeardownHandlers.set(id, clearAccumulatedState) ptyShutdownLifecycleHandlers.set(id, shutdownLifecycle) try { - drainPreHandlerPtyExit(id, exit) + drainPreHandlerPtyExit(id, exit, incarnationId) } catch (error) { if (!hadBufferedExit) { throw error diff --git a/src/renderer/src/components/terminal-pane/ipc-pty-spawn-request.ts b/src/renderer/src/components/terminal-pane/ipc-pty-spawn-request.ts index bef2d22a06f..f6ff1a8e7e2 100644 --- a/src/renderer/src/components/terminal-pane/ipc-pty-spawn-request.ts +++ b/src/renderer/src/components/terminal-pane/ipc-pty-spawn-request.ts @@ -2,11 +2,19 @@ import type { IpcPtyTransportOptions, PtyConnectResult, PtyTransport } from './p type PtyConnectOptions = Parameters[0] +/** `incarnationId` names which lifetime of the returned id this spawn owns; absent when the + * execution host predates the field. It is deliberately NOT on `PtyConnectResult` — only the + * connect handshake needs it, to fence buffered state left by an earlier owner of the same id. */ +export type IpcPtySpawnResponse = PtyConnectResult & { + isReattach?: boolean + incarnationId?: string +} + export async function spawnIpcPty( transportOptions: IpcPtyTransportOptions, connectOptions: PtyConnectOptions, admittedSessionId?: string -): Promise { +): Promise { const { cwd, cwdFallback, @@ -70,5 +78,5 @@ export async function spawnIpcPty( ...(projectRuntime ? { projectRuntime } : {}), ...(terminalColorQueryReplies ? { terminalColorQueryReplies } : {}), ...(telemetry ? { telemetry } : {}) - }) as Promise + }) as Promise } diff --git a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts index 73f756c0857..ccb38ae2f55 100644 --- a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts +++ b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts @@ -203,6 +203,9 @@ function attachPtySecondaryPushListeners(unsubscribes: (() => void)[]): void { deliverPtyExitToHandlers({ ptyId: payload.id, code: payload.code, + // Why forwarded: pty ids are reused, so a buffered exit needs the lifetime it describes to + // tell "this pane's shell died" from "the id's previous owner died" (#16970). + ...(payload.incarnationId ? { incarnationId: payload.incarnationId } : {}), ...(primary ? { primary } : {}), sidecars: sidecars ? Array.from(sidecars) : [] }) @@ -258,9 +261,13 @@ export function getEagerPtyBufferHandle(ptyId: string): EagerPtyHandle | undefin // Why: cap matches TerminalPane's scrollback serialization limit so a restored shell (e.g. tail -f) can't grow unbounded. const EAGER_BUFFER_MAX_BYTES = TERMINAL_SCROLLBACK_SESSION_BUFFER_BYTE_LIMIT +/** `incarnationId` names the lifetime the caller just spawned. Without it a background launch that + * is handed a relay-recycled id drains whatever the id's PREVIOUS owner left here and tears its own + * freshly started agent session down seconds after launch. */ export function registerEagerPtyBuffer( ptyId: string, - onExit: (ptyId: string, code: number) => void + onExit: (ptyId: string, code: number) => void, + incarnationId?: string ): EagerPtyHandle { ensurePtyDispatcher() // Why: head index instead of Array.shift() (O(n)) so pre-attach buffering isn't quadratic under many small chunks. @@ -328,7 +335,7 @@ export function registerEagerPtyBuffer( // Why: defer the pre-handler exit one microtask so the caller receives the returned handle before onExit fires. queueMicrotask(() => { if (ptyExitHandlers.get(ptyId) === exitHandler) { - drainPreHandlerPtyExit(ptyId, exitHandler) + drainPreHandlerPtyExit(ptyId, exitHandler, incarnationId) } else { clearPreHandlerPtyState(ptyId) } diff --git a/src/renderer/src/components/terminal-pane/pty-exit-delivery.ts b/src/renderer/src/components/terminal-pane/pty-exit-delivery.ts index 9180aabf890..a01e09c2d8f 100644 --- a/src/renderer/src/components/terminal-pane/pty-exit-delivery.ts +++ b/src/renderer/src/components/terminal-pane/pty-exit-delivery.ts @@ -7,6 +7,8 @@ import { type PtyExitDelivery = { ptyId: string code: number + /** Which lifetime of `ptyId` died. Absent when the execution host predates the field. */ + incarnationId?: string primary?: (code: number) => void sidecars: readonly ((code: number, context: { hadPrimary: boolean }) => void)[] } @@ -26,7 +28,7 @@ export function deliverPtyExitToHandlers(delivery: PtyExitDelivery): void { consumePreHandlerPtyState(delivery.ptyId) } } else { - bufferPreHandlerPtyExit(delivery.ptyId, delivery.code) + bufferPreHandlerPtyExit(delivery.ptyId, delivery.code, delivery.incarnationId) } } catch (error) { firstError = error 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 489b2f3cd24..a8f4a706a89 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 @@ -8,6 +8,7 @@ import { currentPreHandlerPtySequence, drainPreHandlerPtyData, drainPreHandlerPtyExit, + discardPreHandlerPtyExitFromForeignIncarnation, discardPreHandlerPtyState, discardPreHandlerPtyStateFromPriorIncarnation, hasPreHandlerPtyExit, @@ -19,6 +20,10 @@ 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' +// Two lifetimes of the same relay-renumbered id: the shell that died while the transport was down, +// and the one the fresh spawn just got handed. +const PRIOR_INCARNATION_ID = 'incarnation-before-the-relay-restarted' +const FRESH_INCARNATION_ID = 'incarnation-of-the-shell-now-attaching' describe('pre-handler PTY buffer', () => { afterEach(() => { @@ -228,6 +233,121 @@ describe('pre-handler PTY buffer', () => { expect(data).toHaveBeenCalledWith('startup bytes', undefined) }) + // The case the sequence fence structurally cannot reach: the stale exit is recorded AFTER the + // renderer asked for a fresh PTY, so it is newer than the fence and passes it. Only the + // incarnation says the exit describes a lifetime of the id that ended before this one began. + it("drops a recycled id's exit that arrived after the spawn request, on incarnation alone", () => { + const fence = currentPreHandlerPtySequence() + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 0, PRIOR_INCARNATION_ID) + + discardPreHandlerPtyExitFromForeignIncarnation(RECYCLED_PTY_ID, FRESH_INCARNATION_ID) + discardPreHandlerPtyStateFromPriorIncarnation(RECYCLED_PTY_ID, fence) + + const exit = vi.fn() + expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(false) + drainPreHandlerPtyExit(RECYCLED_PTY_ID, exit) + expect(exit).not.toHaveBeenCalled() + }) + + it('keeps a post-fence exit from the incarnation that is attaching, so an instantly dead shell still reports', () => { + const fence = currentPreHandlerPtySequence() + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 1, FRESH_INCARNATION_ID) + + discardPreHandlerPtyExitFromForeignIncarnation(RECYCLED_PTY_ID, FRESH_INCARNATION_ID) + discardPreHandlerPtyStateFromPriorIncarnation(RECYCLED_PTY_ID, fence) + + const exit = vi.fn() + drainPreHandlerPtyExit(RECYCLED_PTY_ID, exit) + expect(exit).toHaveBeenCalledWith(1) + }) + + // Absence is unknown, never a mismatch: a host that predates the field, and the relay's own + // `{ id, code: -1 }` stale-PTY drop, must keep the fence's behaviour exactly. + it('never discards on incarnation when either side is unnamed', () => { + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 2) + discardPreHandlerPtyExitFromForeignIncarnation(RECYCLED_PTY_ID, FRESH_INCARNATION_ID) + expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(true) + drainPreHandlerPtyExit(RECYCLED_PTY_ID, vi.fn()) + + clearConsumedPreHandlerPtyExit(RECYCLED_PTY_ID) + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 3, PRIOR_INCARNATION_ID) + discardPreHandlerPtyExitFromForeignIncarnation(RECYCLED_PTY_ID, undefined) + expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(true) + }) + + // The identity rule is about exits only. `pty:data` carries no incarnation, so buffered bytes + // stay on the sequence fence and must survive a discard that rejects an exit beside them. + it('leaves buffered bytes alone when it discards a foreign exit', () => { + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 0, PRIOR_INCARNATION_ID) + bufferPreHandlerPtyData(RECYCLED_PTY_ID, 'startup bytes') + + discardPreHandlerPtyExitFromForeignIncarnation(RECYCLED_PTY_ID, FRESH_INCARNATION_ID) + + const data = vi.fn() + expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(false) + drainPreHandlerPtyData(RECYCLED_PTY_ID, data) + expect(data).toHaveBeenCalledWith('startup bytes', undefined) + }) + + // The intersection of the two races this buffer exists for: the shell we just spawned dies before + // the pane attaches, AND the relay flushes the previous owner's exit for the same recycled id + // afterwards. Keyed on the id alone, the stranger overwrites our own exit and the identity discard + // then removes the only survivor — a pane bound to a PTY that is dead and never reported dead. + it("keeps our own pre-attach exit when a late stranger's exit lands on the same recycled id", () => { + const fence = currentPreHandlerPtySequence() + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 1, FRESH_INCARNATION_ID) + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 0, PRIOR_INCARNATION_ID) + + discardPreHandlerPtyExitFromForeignIncarnation(RECYCLED_PTY_ID, FRESH_INCARNATION_ID) + discardPreHandlerPtyStateFromPriorIncarnation(RECYCLED_PTY_ID, fence) + + const exit = vi.fn() + expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(true) + drainPreHandlerPtyExit(RECYCLED_PTY_ID, exit) + expect(exit).toHaveBeenCalledWith(1) + }) + + it('replaces a duplicate exit for the same lifetime instead of crowding out another lifetime', () => { + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 1, FRESH_INCARNATION_ID) + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 9, FRESH_INCARNATION_ID) + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 0, PRIOR_INCARNATION_ID) + + discardPreHandlerPtyExitFromForeignIncarnation(RECYCLED_PTY_ID, FRESH_INCARNATION_ID) + + const exit = vi.fn() + drainPreHandlerPtyExit(RECYCLED_PTY_ID, exit) + expect(exit).toHaveBeenCalledWith(9) + }) + + // Reads are filtered by lifetime inside the buffer, so a consumer that never calls the discard — + // a background launch registering an eager buffer straight off its own spawn — still cannot be + // handed the previous owner's exit and tear its freshly started session down. + it('never reports or delivers a foreign exit to a reader that names its lifetime', () => { + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 0, PRIOR_INCARNATION_ID) + + const exit = vi.fn() + expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID, FRESH_INCARNATION_ID)).toBe(false) + drainPreHandlerPtyExit(RECYCLED_PTY_ID, exit, FRESH_INCARNATION_ID) + expect(exit).not.toHaveBeenCalled() + + // A reader with no incarnation still sees it: absence is unknown, so it has nothing to judge by. + expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(true) + }) + + // A malformed incarnation is evidence of nothing. Treating it as a value that disagrees with + // everything would discard the very exits the buffer exists to deliver. + it('treats a malformed incarnation as unknown rather than as a disagreement', () => { + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 4, { not: 'a string' }) + discardPreHandlerPtyExitFromForeignIncarnation(RECYCLED_PTY_ID, FRESH_INCARNATION_ID) + expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(true) + drainPreHandlerPtyExit(RECYCLED_PTY_ID, vi.fn()) + + clearConsumedPreHandlerPtyExit(RECYCLED_PTY_ID) + bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 5, PRIOR_INCARNATION_ID) + discardPreHandlerPtyExitFromForeignIncarnation(RECYCLED_PTY_ID, 42) + expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(true) + }) + it('re-admits exits for a recycled id whose prior incarnation was consumed', () => { consumePreHandlerPtyState(RECYCLED_PTY_ID) 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 9e2f4d3e929..bbe4b783ee9 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 @@ -1,3 +1,4 @@ +import { isPtyIncarnationId } from '../../../../shared/pty-incarnation' import { clampUtf8Tail } from './pty-eager-buffer-clamp' import type { PtyDataMeta } from './pty-dispatcher' @@ -18,10 +19,16 @@ type BufferedPreHandlerPtyState = { type BufferedPreHandlerPtyExit = { code: number sequence: number + /** Which lifetime of `ptyId` died. Absent when the emitting host predates the field. */ + incarnationId?: string } const preHandlerPtyData = new Map() -const preHandlerPtyExit = new Map() +// Why one record per lifetime and not one per id: a recycled id can have its previous lifetime's +// exit still in flight while the lifetime that just replaced it also dies pre-attach. With a single +// slot the late stranger overwrites the real exit, and the identity discard below then removes the +// only survivor — leaving a pane bound to a PTY that is dead and will never be reported dead. +const preHandlerPtyExit = new Map() const consumedPreHandlerPtyExits = new Map() const discardedPreHandlerPtyStates = new Map>() const DISCARDED_PRE_HANDLER_PTY_STATE_TTL_MS = 60_000 @@ -32,6 +39,9 @@ const DISCARDED_PRE_HANDLER_PTY_STATE_TTL_MS = 60_000 const PRE_HANDLER_PTY_DATA_MAX_BYTES = 512 * 1024 const PRE_HANDLER_PTY_DATA_MAX_PTYS = 64 const PRE_HANDLER_PTY_EXIT_MAX_PTYS = 64 +// Why small: only lifetimes racing the same pre-attach window can coexist, which in practice is the +// outgoing one and the incoming one. +const PRE_HANDLER_PTY_EXIT_MAX_INCARNATIONS_PER_PTY = 4 // Why: legit pre-attach windows drain within milliseconds and hold little // data. Sustained accumulation means a pane lost its data handler (the // frozen-pane detach/attach race) — leave a breadcrumb for trace capture. @@ -53,6 +63,64 @@ export function currentPreHandlerPtySequence(): number { return preHandlerPtySequence } +/** Map preserves insertion order, so the first key is the least recently admitted id. */ +function evictOldestPtyIfAtCap(map: Map, ptyId: string, cap: number): void { + if (map.has(ptyId) || map.size < cap) { + return + } + const oldestPtyId = map.keys().next().value + if (typeof oldestPtyId === 'string') { + map.delete(oldestPtyId) + } +} + +/** Drop a buffered exit proven to describe a different lifetime of `ptyId` than the one now + * attaching, whenever it arrived. + * + * Why identity and not the sequence fence below: a fence is a clock, so it can only reject records + * dated before the spawn request left the renderer. An exit from the id's previous owner that + * arrives AFTER that — a relay that restarted mid-spawn and flushed it late — passes the fence and + * reports a brand-new shell as already dead. The incarnation says whose lifetime the exit + * describes, so it settles that case on evidence rather than timing. + * + * Only a positive disagreement discards: both sides must name an incarnation. Absence is + * "unknown", never a mismatch, so an execution host that predates the field keeps the fence's + * behaviour exactly. Safe on a reattach and a cold restore too — those deliberately re-own an + * existing id, and re-owning incarnation X is still proof that W's exit was not theirs. */ +export function discardPreHandlerPtyExitFromForeignIncarnation( + ptyId: string, + incarnationId: unknown +): void { + // Why the shared guard and not a truthiness check: anything that is not a well-formed incarnation + // is evidence of nothing, and must read as "unknown" rather than disagree with everything. + if (!isPtyIncarnationId(incarnationId)) { + return + } + retainPreHandlerPtyExits( + ptyId, + (exit) => exit.incarnationId === undefined || exit.incarnationId === incarnationId + ) +} + +function retainPreHandlerPtyExits( + ptyId: string, + keep: (exit: BufferedPreHandlerPtyExit) => boolean +): void { + const exits = preHandlerPtyExit.get(ptyId) + if (!exits) { + return + } + const kept = exits.filter(keep) + if (kept.length === exits.length) { + return + } + if (kept.length === 0) { + preHandlerPtyExit.delete(ptyId) + return + } + preHandlerPtyExit.set(ptyId, kept) +} + export function bufferPreHandlerPtyData(ptyId: string, data: string, meta?: PtyDataMeta): void { if (discardedPreHandlerPtyStates.has(ptyId)) { return @@ -61,12 +129,7 @@ export function bufferPreHandlerPtyData(ptyId: string, data: string, meta?: PtyD if (!chunk.data) { return } - if (!preHandlerPtyData.has(ptyId) && preHandlerPtyData.size >= PRE_HANDLER_PTY_DATA_MAX_PTYS) { - const oldestPtyId = preHandlerPtyData.keys().next().value - if (typeof oldestPtyId === 'string') { - preHandlerPtyData.delete(oldestPtyId) - } - } + evictOldestPtyIfAtCap(preHandlerPtyData, ptyId, PRE_HANDLER_PTY_DATA_MAX_PTYS) const bufferedMeta = meta && chunk.data.length !== data.length && typeof meta.rawLength === 'number' ? { ...meta, rawLength: chunk.bytes } @@ -130,17 +193,37 @@ export function replayPreHandlerPtyData(ptyId: string, observer: (data: string) } } -export function bufferPreHandlerPtyExit(ptyId: string, code: number): void { +export function bufferPreHandlerPtyExit( + ptyId: string, + code: number, + incarnationId?: unknown +): void { if (consumedPreHandlerPtyExits.has(ptyId) || discardedPreHandlerPtyStates.has(ptyId)) { return } - if (!preHandlerPtyExit.has(ptyId) && preHandlerPtyExit.size >= PRE_HANDLER_PTY_EXIT_MAX_PTYS) { - const oldestPtyId = preHandlerPtyExit.keys().next().value - if (typeof oldestPtyId === 'string') { - preHandlerPtyExit.delete(oldestPtyId) - } + evictOldestPtyIfAtCap(preHandlerPtyExit, ptyId, PRE_HANDLER_PTY_EXIT_MAX_PTYS) + const exit: BufferedPreHandlerPtyExit = { + code, + sequence: nextPreHandlerPtySequence(), + // Record only a well-formed incarnation; a malformed one must not become evidence. + ...(isPtyIncarnationId(incarnationId) ? { incarnationId } : {}) + } + const exits = preHandlerPtyExit.get(ptyId) + if (!exits) { + preHandlerPtyExit.set(ptyId, [exit]) + return + } + // A duplicate exit for a lifetime replaces that lifetime's record rather than crowding out + // another one's; unnamed records share the single `undefined` slot, as they did before. + const sameLifetime = exits.findIndex((entry) => entry.incarnationId === exit.incarnationId) + if (sameLifetime !== -1) { + exits[sameLifetime] = exit + return + } + exits.push(exit) + if (exits.length > PRE_HANDLER_PTY_EXIT_MAX_INCARNATIONS_PER_PTY) { + exits.shift() } - preHandlerPtyExit.set(ptyId, { code, sequence: nextPreHandlerPtySequence() }) } /** Drop pre-handler state a freshly spawned PTY inherited from an earlier owner of its id. @@ -148,15 +231,18 @@ export function bufferPreHandlerPtyExit(ptyId: string, code: number): void { * `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. */ + * writes before the pane registers its handler) and losing it would blank a legitimate pane. + * + * Still needed alongside `discardPreHandlerPtyExitFromForeignIncarnation`, which supersedes it + * wherever both sides name an incarnation. Two cases have none to compare and rest on the fence + * alone: buffered BYTES, because `pty:data` carries no incarnation at all; and exits from an + * execution host that predates the field or from a main-side path that synthesizes one without it + * (a relay dropping a stale PTY after a failed reattach sends `{ id, code: -1 }`). */ export function discardPreHandlerPtyStateFromPriorIncarnation( ptyId: string, fenceSequence: number ): void { - const exit = preHandlerPtyExit.get(ptyId) - if (exit && exit.sequence <= fenceSequence) { - preHandlerPtyExit.delete(ptyId) - } + retainPreHandlerPtyExits(ptyId, (exit) => exit.sequence > fenceSequence) const data = preHandlerPtyData.get(ptyId) if (data && data.sequence <= fenceSequence) { preHandlerPtyData.delete(ptyId) @@ -215,12 +301,42 @@ export function discardPreHandlerPtyState(ptyId: string): void { discardedPreHandlerPtyStates.set(ptyId, timer) } -export function hasPreHandlerPtyExit(ptyId: string): boolean { - return preHandlerPtyExit.has(ptyId) +/** The records for `ptyId` that could describe `incarnationId`'s lifetime. + * + * Every read of the exit buffer goes through here, so a record proven to belong to a different + * lifetime is unreachable BY CONSTRUCTION rather than because each caller remembered to discard + * first. A caller that cannot name an incarnation — a reattach that has not round-tripped yet — + * still sees everything, which is the honest answer: it holds no evidence to discriminate with. */ +function admissiblePreHandlerPtyExits( + ptyId: string, + incarnationId: unknown +): BufferedPreHandlerPtyExit[] { + const exits = preHandlerPtyExit.get(ptyId) ?? [] + if (!isPtyIncarnationId(incarnationId)) { + return exits + } + return exits.filter( + (exit) => exit.incarnationId === undefined || exit.incarnationId === incarnationId + ) } -export function drainPreHandlerPtyExit(ptyId: string, handler: (code: number) => void): void { - const exit = preHandlerPtyExit.get(ptyId) +export function hasPreHandlerPtyExit(ptyId: string, incarnationId?: unknown): boolean { + return admissiblePreHandlerPtyExits(ptyId, incarnationId).length > 0 +} + +export function drainPreHandlerPtyExit( + ptyId: string, + handler: (code: number) => void, + incarnationId?: unknown +): void { + // Newest admissible record: picking by sequence keeps the last-write-wins delivery a single-slot + // buffer always had, now scoped to the lifetime actually asking. + let exit: BufferedPreHandlerPtyExit | undefined + for (const candidate of admissiblePreHandlerPtyExits(ptyId, incarnationId)) { + if (!exit || candidate.sequence > exit.sequence) { + exit = candidate + } + } if (exit === undefined) { return } diff --git a/src/renderer/src/components/terminal-pane/pty-transport-recycled-pty-incarnation.test.ts b/src/renderer/src/components/terminal-pane/pty-transport-recycled-pty-incarnation.test.ts new file mode 100644 index 00000000000..7936300fb06 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-transport-recycled-pty-incarnation.test.ts @@ -0,0 +1,97 @@ +/** A restarted SSH relay renumbers PTYs from pty-1, so a fresh spawn is routinely handed an id + * whose previous shell is still emitting a late exit. Applying that exit to the new shell reports + * it as already dead (`exitedBeforeAttach`), the pane never binds a PTY, and the tab is blank and + * unusable forever — the runtime-proven failure behind #16970. + * + * #16970 dated every buffered record and dropped anything older than the spawn request. That fence + * is a clock, so it cannot judge a stale exit that arrives AFTER the request left. These specs pin + * the identity rule that can: the incarnation names which lifetime of the id died. */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + installIpcPtyWindow, + restorePtySpecWindow, + type PtyExitPayload +} from './pty-transport-test-harness' + +const RECYCLED_PTY_ID = 'ssh:target@@pty-1' +const PRIOR_INCARNATION_ID = 'incarnation-before-the-relay-restarted' +const FRESH_INCARNATION_ID = 'incarnation-of-the-shell-now-attaching' + +describe('createIpcPtyTransport against a relay-recycled PTY id', () => { + const originalWindow = (globalThis as { window?: typeof window }).window + let onExit: ((payload: PtyExitPayload) => void) | null = null + + beforeEach(() => { + vi.resetModules() + onExit = null + installIpcPtyWindow(originalWindow, { + exit: (callback) => { + onExit = callback + } + }) + }) + + afterEach(() => { + restorePtySpecWindow(originalWindow) + }) + + /** Deliver `exit` while the spawn request is in flight, then settle the spawn. + * + * Firing from inside the spawn mock makes "after the request left the renderer" *definitional* + * rather than microtask-scheduled: the fence is read before `spawnIpcPty` is called, so an exit + * buffered during the call is always newer than the fence. That is the whole point — the fence + * keeps this record, and only the incarnation can reject it. */ + async function connectWithExitDuringSpawn( + exit: PtyExitPayload, + spawnResponse: { id: string; incarnationId?: string } + ): Promise<{ result: unknown; paneExit: ReturnType }> { + const { createIpcPtyTransport } = await import('./pty-transport') + const spawn = window.api.pty.spawn as unknown as ReturnType + spawn.mockImplementationOnce(() => { + onExit?.(exit) + return Promise.resolve(spawnResponse) + }) + const paneExit = vi.fn() + const transport = createIpcPtyTransport({}) + + const result = await transport.connect({ url: '', callbacks: { onExit: paneExit } }) + return { result, paneExit } + } + + it('does not report a fresh shell as exited when the id’s previous incarnation exits mid-spawn', async () => { + const { result, paneExit } = await connectWithExitDuringSpawn( + { id: RECYCLED_PTY_ID, code: 0, incarnationId: PRIOR_INCARNATION_ID }, + { id: RECYCLED_PTY_ID, incarnationId: FRESH_INCARNATION_ID } + ) + + // Positive, not just "not dead": a bare `not.toMatchObject` would also pass on a bailed-out + // connect that returned undefined. A healthy fresh spawn resolves to its pty id. + expect(result).toBe(RECYCLED_PTY_ID) + expect(paneExit).not.toHaveBeenCalled() + }) + + // The reason the pre-handler buffer exists: a shell that dies before the pane registers its exit + // handler must still be reported, or the pane hangs on a PTY that is already gone. + it('still reports an exit from the incarnation it actually attached to', async () => { + const { result, paneExit } = await connectWithExitDuringSpawn( + { id: RECYCLED_PTY_ID, code: 3, incarnationId: FRESH_INCARNATION_ID }, + { id: RECYCLED_PTY_ID, incarnationId: FRESH_INCARNATION_ID } + ) + + expect(result).toMatchObject({ id: RECYCLED_PTY_ID, exitedBeforeAttach: true }) + expect(paneExit).toHaveBeenCalledWith(3) + }) + + // Absence is unknown, never a mismatch — so an SSH host predating the field, and the relay's own + // unnamed `{ id, code: -1 }` drop, keep exactly the behaviour #16970 shipped. (`remote:` runtime + // PTYs are not covered by this: their exits never traverse `pty:exit` at all.) + it('never discards on incarnation when the host names none', async () => { + const { result, paneExit } = await connectWithExitDuringSpawn( + { id: RECYCLED_PTY_ID, code: 3 }, + { id: RECYCLED_PTY_ID } + ) + + expect(result).toMatchObject({ id: RECYCLED_PTY_ID, exitedBeforeAttach: true }) + expect(paneExit).toHaveBeenCalledWith(3) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/pty-transport-test-harness.ts b/src/renderer/src/components/terminal-pane/pty-transport-test-harness.ts index 0937830959c..727327aa811 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport-test-harness.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport-test-harness.ts @@ -1,7 +1,13 @@ import { vi } from 'vitest' export type PtyStreamPayload = { id: string; data: string } -export type PtyExitPayload = { id: string; code: number; preserveRendererBinding?: boolean } +export type PtyExitPayload = { + id: string + code: number + preserveRendererBinding?: boolean + /** Which lifetime of `id` died; absent when the execution host predates the field. */ + incarnationId?: string +} /** Sinks let each spec own its `onData`/`onExit` bindings, so test bodies keep calling them directly. */ export type PtyListenerSinks = { diff --git a/src/renderer/src/lib/launch-agent-background-session.test.ts b/src/renderer/src/lib/launch-agent-background-session.test.ts index eedd1e9fb32..42df8332882 100644 --- a/src/renderer/src/lib/launch-agent-background-session.test.ts +++ b/src/renderer/src/lib/launch-agent-background-session.test.ts @@ -92,6 +92,7 @@ describe('launchAgentBackgroundSession', () => { it('spawns a PTY first and creates the inactive tab already bound to it', async () => { const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + mockSpawn.mockResolvedValue({ id: 'pty-1', incarnationId: 'inc-fresh' }) const result = await launchAgentBackgroundSession({ agent: 'claude', @@ -160,7 +161,13 @@ describe('launchAgentBackgroundSession', () => { recordInteraction: false }) expect(mockUpdateTabPtyId).toHaveBeenCalledWith(tabId, 'pty-1') - expect(mockRegisterEagerPtyBuffer).toHaveBeenCalledWith('pty-1', expect.any(Function)) + // The incarnation rides along so a relay-recycled id cannot drain the previous owner's exit + // into this handler and tear the session down right after launch. + expect(mockRegisterEagerPtyBuffer).toHaveBeenCalledWith( + 'pty-1', + expect.any(Function), + 'inc-fresh' + ) expect(mockSubscribeToPtyData).toHaveBeenCalledWith('pty-1', expect.any(Function)) expect(mockSubscribeToPtyExit).toHaveBeenCalledWith('pty-1', expect.any(Function)) expect(result).toMatchObject({ tabId, paneKey, ptyId: 'pty-1' }) diff --git a/src/renderer/src/lib/launch-agent-background-session.ts b/src/renderer/src/lib/launch-agent-background-session.ts index 947cabb317b..a1d9f1df654 100644 --- a/src/renderer/src/lib/launch-agent-background-session.ts +++ b/src/renderer/src/lib/launch-agent-background-session.ts @@ -127,7 +127,9 @@ export async function launchAgentBackgroundSession( ) let ptyId = '', runtimeTerminalHandle: string | null = null - let returnedLaunchConfig: typeof startupPlan.launchConfig | undefined + // What the local spawn answered and later steps still need: which lifetime of `ptyId` this launch + // owns, and the config the host actually launched. Both absent for a runtime terminal. + let spawned: { incarnationId?: string; launchConfig?: typeof startupPlan.launchConfig } = {} let tab: ReturnType | null = null let exitHandled = false, eagerPtyBuffer: EagerPtyHandle | null = null @@ -219,7 +221,7 @@ export async function launchAgentBackgroundSession( } }) ptyId = result.id - returnedLaunchConfig = result.launchConfig + spawned = result } const adopted = await adoptAgentBackgroundSessionTab({ store, @@ -227,7 +229,7 @@ export async function launchAgentBackgroundSession( reservedTabId, ptyId, paneKey, - launchConfig: returnedLaunchConfig ?? startupPlan.launchConfig, + launchConfig: spawned.launchConfig ?? startupPlan.launchConfig, launchRegistration, runtimeTarget, runtimeTerminalHandle, @@ -280,7 +282,9 @@ export async function launchAgentBackgroundSession( .then((result) => handleExit(ptyId, result.wait.exitCode ?? 0)) .catch(() => {}) } else { - eagerPtyBuffer = registerEagerPtyBuffer(ptyId, handleExit) + // Why the incarnation: a relay-recycled id can hold the previous owner's exit, and draining + // that into this handler tears the agent session down seconds after it launched. + eagerPtyBuffer = registerEagerPtyBuffer(ptyId, handleExit, spawned.incarnationId) unsubscribeData = subscribeToPtyData(ptyId, handleData) // Why: opening the workspace attaches a real terminal transport and disposes // the eager exit handler. This sidecar keeps automation completion tracking diff --git a/src/renderer/src/lib/launch-worktree-background-terminals.ts b/src/renderer/src/lib/launch-worktree-background-terminals.ts index 049176c1e81..791504d6948 100644 --- a/src/renderer/src/lib/launch-worktree-background-terminals.ts +++ b/src/renderer/src/lib/launch-worktree-background-terminals.ts @@ -113,12 +113,15 @@ function persistExitedPaneOutput(tabId: string, leafId: string, output: string): }) } -function registerBackgroundPaneBuffer(tabId: string, leafId: string, ptyId: string): void { +// Why the incarnation: a relay-recycled id can hold the previous owner's exit, and draining that +// into this handler tears the pane down seconds after it launched. +function registerBackgroundPaneBuffer(tabId: string, leafId: string, pane: SpawnedPane): void { let eagerBuffer: EagerPtyHandle | null = null - eagerBuffer = registerEagerPtyBuffer(ptyId, (exitPtyId) => { + const onExit = (exitPtyId: string): void => { persistExitedPaneOutput(tabId, leafId, eagerBuffer?.flush() ?? '') useAppStore.getState().clearTabPtyId(tabId, exitPtyId) - }) + } + eagerBuffer = registerEagerPtyBuffer(pane.ptyId, onExit, pane.incarnationId) } function buildSetupCommand(setup: WorktreeSetupLaunch): string { @@ -130,6 +133,9 @@ function buildSetupCommand(setup: WorktreeSetupLaunch): string { ) } +/** The id a background pane got, plus which lifetime of it this spawn owns. */ +type SpawnedPane = { ptyId: string; incarnationId?: string } + async function spawnPane(args: { worktree: Worktree connectionId: string | null @@ -137,7 +143,7 @@ async function spawnPane(args: { leafId: string command?: string env?: Record -}): Promise { +}): Promise { const result = await window.api.pty.spawn({ cols: 120, rows: 40, @@ -149,7 +155,10 @@ async function spawnPane(args: { tabId: args.tabId, leafId: args.leafId }) - return result.id + return { + ptyId: result.id, + ...(result.incarnationId ? { incarnationId: result.incarnationId } : {}) + } } async function createBackgroundTab(args: { @@ -171,9 +180,9 @@ async function createBackgroundTab(args: { const leafId = createBrowserUuid() store.setTabLayout(tab.id, singlePaneLayoutSnapshot(leafId)) - let ptyId: string + let pane: SpawnedPane try { - ptyId = await spawnPane({ + pane = await spawnPane({ worktree: args.worktree, connectionId: args.connectionId, tabId: tab.id, @@ -188,16 +197,16 @@ async function createBackgroundTab(args: { if ( await retireUnownedTerminal({ owner: { tabId: tab.id }, - ptyId, + ptyId: pane.ptyId, runtimeTarget: { kind: 'local' } }) ) { throw new Error('The terminal tab was closed before its session finished starting.') } - store.updateTabPtyId(tab.id, ptyId) - store.setTabLayout(tab.id, singlePaneLayoutSnapshot(leafId, ptyId)) - registerBackgroundPaneBuffer(tab.id, leafId, ptyId) - return { tabId: tab.id, primary: { leafId, ptyId } } + store.updateTabPtyId(tab.id, pane.ptyId) + store.setTabLayout(tab.id, singlePaneLayoutSnapshot(leafId, pane.ptyId)) + registerBackgroundPaneBuffer(tab.id, leafId, pane) + return { tabId: tab.id, primary: { leafId, ptyId: pane.ptyId } } } async function addSetupSplit(args: { @@ -209,7 +218,7 @@ async function addSetupSplit(args: { }): Promise { const store = useAppStore.getState() const setupLeafId = createBrowserUuid() - const setupPtyId = await spawnPane({ + const setupPane = await spawnPane({ worktree: args.worktree, connectionId: args.connectionId, tabId: args.tab.tabId, @@ -220,23 +229,23 @@ async function addSetupSplit(args: { if ( await retireUnownedTerminal({ owner: { tabId: args.tab.tabId }, - ptyId: setupPtyId, + ptyId: setupPane.ptyId, runtimeTarget: { kind: 'local' } }) ) { return } - store.updateTabPtyId(args.tab.tabId, setupPtyId) + store.updateTabPtyId(args.tab.tabId, setupPane.ptyId) store.setTabLayout( args.tab.tabId, buildSplitLayout( args.tab.primary, - { leafId: setupLeafId, ptyId: setupPtyId }, + { leafId: setupLeafId, ptyId: setupPane.ptyId }, args.direction, getSetupTabTitle() ) ) - registerBackgroundPaneBuffer(args.tab.tabId, setupLeafId, setupPtyId) + registerBackgroundPaneBuffer(args.tab.tabId, setupLeafId, setupPane) } function getDefaultTabLaunches(