From 2d56ebee77a1d8a066e35ceaac20b8713495c377 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 14 Jul 2026 03:14:36 -0700 Subject: [PATCH] fix(terminal): bind recovery to xterm lifecycle --- .../terminal-pane/pty-connection.test.ts | 16 +++- .../terminal-pane/pty-connection.ts | 20 ++++- .../terminal-pane-recovery.test.ts | 64 +++++++++++++ .../terminal-pane/terminal-pane-recovery.ts | 90 ++++++++++++++----- .../use-terminal-pane-lifecycle.test.ts | 18 ++++ .../use-terminal-pane-lifecycle.ts | 18 +++- .../pane-terminal-output-scheduler.test.ts | 16 ++++ .../pane-terminal-output-scheduler.ts | 5 ++ 8 files changed, 221 insertions(+), 26 deletions(-) diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index a4880c9f2fc..a7484e5af47 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -863,6 +863,7 @@ describe('connectPanePty', () => { ackColdRestore: vi.fn(), onClearBufferRequest: vi.fn(() => vi.fn()), onSerializeBufferRequest: vi.fn(() => vi.fn()), + sendSerializedBuffer: vi.fn(), declarePendingPaneSerializer: vi.fn().mockResolvedValue(1), settlePaneSerializer: vi.fn().mockResolvedValue(undefined), clearPendingPaneSerializer: vi.fn().mockResolvedValue(undefined) @@ -5637,7 +5638,7 @@ describe('connectPanePty', () => { expect(transport.resize).toHaveBeenLastCalledWith(65, 63) }) - it('adopts a still-live background PTY via attach instead of reattaching when an eager buffer exists', async () => { + it('adopts a live eager PTY and withholds snapshots after its renderer dies', async () => { // Why: background automation tabs spawn the agent PTY eagerly and register an // eager buffer, then never mount until opened. On first mount the restored // ptyId equals the tab ptyId, so without this guard the pane mis-routes into @@ -5681,6 +5682,19 @@ describe('connectPanePty', () => { ) const { hasPtySerializer } = await import('./pty-buffer-serializer') expect(hasPtySerializer(eagerPtyId)).toBe(true) + + const serializeRequestHandler = ( + window.api.pty.onSerializeBufferRequest as unknown as { + mock: { calls: [[(request: { requestId: string; ptyId: string }) => void]] } + } + ).mock.calls[0]?.[0] + const { notifyUndeliverableWrite } = + await import('@/lib/pane-manager/terminal-write-pipeline-health') + notifyUndeliverableWrite(pane.terminal, 'replay-wedged') + serializeRequestHandler?.({ requestId: 'dead-renderer', ptyId: eagerPtyId }) + await flushAsyncTicks() + + expect(window.api.pty.sendSerializedBuffer).toHaveBeenCalledWith('dead-renderer', null) }) it('does not adopt another tab live eager PTY from a stale restored leaf binding', async () => { diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index 0c88aa83788..c5f26f0a55c 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -56,6 +56,7 @@ import { } from '@/lib/pane-manager/terminal-write-pipeline-health' import { captureTerminalPaneRecoveryGeneration, + registerTerminalPaneRecoveryInstance, requestTerminalPaneRecovery } from './terminal-pane-recovery' import { @@ -954,6 +955,7 @@ export function connectPanePty( // Why: recovery ownership belongs to this xterm instance. A request that // settles after remount must not remount its already-replaced successor. const terminalRecoveryGeneration = captureTerminalPaneRecoveryGeneration(deps.tabId) + const terminalRecoveryInstance = registerTerminalPaneRecoveryInstance(deps.tabId) exposeE2eTerminalPtyOutputDebug() let disposed = false let connectFrame: number | null = null @@ -3375,6 +3377,7 @@ export function connectPanePty( ptyId: undeliverablePtyId, reason: 'input-undeliverable', terminalRecoveryGeneration, + terminalRecoveryInstanceId: terminalRecoveryInstance.id, // Why: pty:hasPty answers null for ids the local registry doesn't own, // and a disconnected remote pane would otherwise remount-churn on every // cooldown window while typing. Local panes keep the lenient gate. @@ -3397,7 +3400,8 @@ export function connectPanePty( tabId: deps.tabId, ptyId: transport.getPtyId() ?? storePtyId, reason, - terminalRecoveryGeneration + terminalRecoveryGeneration, + terminalRecoveryInstanceId: terminalRecoveryInstance.id }) } ) @@ -3945,7 +3949,15 @@ export function connectPanePty( ptyId, async (opts) => { try { + if (isTerminalWritePipelineCertifiedDead(pane.terminal)) { + return null + } await waitForTerminalOutputParsed(pane.terminal) + // Certification can land while the serializer waits for an older + // write; never publish a fossil frame from a dead renderer. + if (isTerminalWritePipelineCertifiedDead(pane.terminal)) { + return null + } // Why: alt-screen TUIs (vim, claude-code) hold transient state in // the alternate screen. The hydration path requests // altScreenForcesZeroRows so normal-buffer scrollback isn't bled @@ -6179,7 +6191,8 @@ export function connectPanePty( tabId: deps.tabId, ptyId: transport.getPtyId() ?? storePtyId, reason: 'restore-blocked', - terminalRecoveryGeneration + terminalRecoveryGeneration, + terminalRecoveryInstanceId: terminalRecoveryInstance.id }) } return false @@ -7544,6 +7557,9 @@ export function connectPanePty( reconcileIfSessionMissing, dispose() { disposed = true + // A normal park/reconnect/remount does not advance the recovery epoch; + // invalidate this concrete xterm so its delayed retry cannot hit the next. + terminalRecoveryInstance.unregister() unregisterUndeliverableWriteHandler() // Why: the post-spawn reconcile polls across frames; cancel its pending // rAF so a torn-down pane cannot keep fitting/resizing after disposal. diff --git a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts index 7034369c597..eb978324b66 100644 --- a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { _resetTerminalPaneRecoveryForTests, captureTerminalPaneRecoveryGeneration, + registerTerminalPaneRecoveryInstance, requestTerminalPaneRecovery } from './terminal-pane-recovery' @@ -237,6 +238,69 @@ describe('requestTerminalPaneRecovery', () => { expect(mocks.remountTerminalTabForRecovery).toHaveBeenCalledTimes(2) }) + it('cancels a retry when a non-recovery lifecycle replaces its xterm', async () => { + vi.useFakeTimers() + vi.setSystemTime(0) + const originalInstance = registerTerminalPaneRecoveryInstance('tab-1') + await requestTerminalPaneRecovery({ + tabId: 'tab-1', + ptyId: 'pty-1', + reason: 'write-stalled', + terminalRecoveryGeneration: captureTerminalPaneRecoveryGeneration('tab-1'), + terminalRecoveryInstanceId: originalInstance.id + }) + originalInstance.unregister() + + const wedgedReplacement = registerTerminalPaneRecoveryInstance('tab-1') + await requestTerminalPaneRecovery({ + tabId: 'tab-1', + ptyId: 'pty-1', + reason: 'replay-wedged', + terminalRecoveryGeneration: captureTerminalPaneRecoveryGeneration('tab-1'), + terminalRecoveryInstanceId: wedgedReplacement.id + }) + expect(vi.getTimerCount()).toBe(1) + + // Cold parking, SSH reconnect, and ordinary remounts dispose the binding + // without changing the recovery epoch; disposal owns timer invalidation. + wedgedReplacement.unregister() + const healthySuccessor = registerTerminalPaneRecoveryInstance('tab-1') + expect(vi.getTimerCount()).toBe(0) + await vi.advanceTimersByTimeAsync(600_000) + + expect(mocks.remountTerminalTabForRecovery).toHaveBeenCalledTimes(1) + healthySuccessor.unregister() + }) + + it('keeps a sibling pane retry when the first requesting split is disposed', async () => { + vi.useFakeTimers() + vi.setSystemTime(0) + await requestTerminalPaneRecovery({ + tabId: 'tab-1', + ptyId: 'pty-1', + reason: 'write-stalled' + }) + const recoveryGeneration = captureTerminalPaneRecoveryGeneration('tab-1') + const firstSplit = registerTerminalPaneRecoveryInstance('tab-1') + const secondSplit = registerTerminalPaneRecoveryInstance('tab-1') + for (const instance of [firstSplit, secondSplit]) { + await requestTerminalPaneRecovery({ + tabId: 'tab-1', + ptyId: `pty-${instance.id}`, + reason: 'replay-wedged', + terminalRecoveryGeneration: recoveryGeneration, + terminalRecoveryInstanceId: instance.id + }) + } + + firstSplit.unregister() + expect(vi.getTimerCount()).toBe(1) + await vi.advanceTimersByTimeAsync(15_000) + + expect(mocks.remountTerminalTabForRecovery).toHaveBeenCalledTimes(2) + secondSplit.unregister() + }) + it('budgets tabs independently', async () => { expect( await requestTerminalPaneRecovery({ tabId: 'tab-1', ptyId: 'pty-1', reason: 'write-stalled' }) diff --git a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts index afef0070843..bc5077221c9 100644 --- a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts +++ b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts @@ -23,9 +23,12 @@ type RecoveryRequest = { tabId: string ptyId: string | null reason: TerminalPaneRecoveryReason - /** Identifies the xterm generation making the request. A replaced xterm - * must not schedule or complete recovery for its successor. */ + /** Identifies the tab recovery epoch making the request. A successful + * recovery must immediately invalidate every pre-remount request. */ terminalRecoveryGeneration?: number + /** Identifies the concrete mounted xterm making the request. Disposal + * invalidates delayed work even when the tab's recovery epoch is unchanged. */ + terminalRecoveryInstanceId?: number /** Remote panes (runtime mirrors, app-SSH) must prove the PTY alive before * an input-undeliverable remount: pty:hasPty answers null for ids the local * registry doesn't own, and treating null as "proceed" would let a @@ -46,7 +49,15 @@ const RECOVERY_COOLDOWN_MS = 15_000 const recoveryTimestampsByTabId = new Map() const recoveryGenerationByTabId = new Map() -const pendingRetryTimerByTabId = new Map>() +const activeTerminalRecoveryInstanceIds = new Set() +const pendingRetryByTabId = new Map< + string, + { + timer: ReturnType + requestsByInstanceId: Map + } +>() +let nextTerminalRecoveryInstanceId = 0 type RecoveryBudget = | { allowed: true } @@ -81,38 +92,73 @@ export function captureTerminalPaneRecoveryGeneration(tabId: string): number { return recoveryGenerationByTabId.get(tabId) ?? 0 } -function requestBelongsToCurrentRecoveryGeneration(request: RecoveryRequest): boolean { +export function registerTerminalPaneRecoveryInstance(tabId: string): { + id: number + unregister: () => void +} { + const id = ++nextTerminalRecoveryInstanceId + activeTerminalRecoveryInstanceIds.add(id) + return { + id, + unregister: () => { + activeTerminalRecoveryInstanceIds.delete(id) + const pendingRetry = pendingRetryByTabId.get(tabId) + pendingRetry?.requestsByInstanceId.delete(id) + if (pendingRetry?.requestsByInstanceId.size === 0) { + cancelPendingRecoveryRetry(tabId) + } + } + } +} + +function isCurrentTerminalRecoveryRequest(request: RecoveryRequest): boolean { return ( - request.terminalRecoveryGeneration === undefined || - request.terminalRecoveryGeneration === captureTerminalPaneRecoveryGeneration(request.tabId) + (request.terminalRecoveryGeneration === undefined || + request.terminalRecoveryGeneration === + captureTerminalPaneRecoveryGeneration(request.tabId)) && + (request.terminalRecoveryInstanceId === undefined || + activeTerminalRecoveryInstanceIds.has(request.terminalRecoveryInstanceId)) ) } function scheduleRecoveryRetry(request: RecoveryRequest, delayMs: number): void { - if (!requestBelongsToCurrentRecoveryGeneration(request)) { + if (!isCurrentTerminalRecoveryRequest(request)) { return } - if (pendingRetryTimerByTabId.has(request.tabId)) { + const pendingRetry = pendingRetryByTabId.get(request.tabId) + if (pendingRetry) { + // Multiple split panes share a tab-wide remount. Keep one request per + // concrete xterm so disposing one pane cannot cancel a sibling's heal. + pendingRetry.requestsByInstanceId.set(request.terminalRecoveryInstanceId, request) return } + const requestsByInstanceId = new Map([ + [request.terminalRecoveryInstanceId, request] + ]) const timer = setTimeout( () => { - pendingRetryTimerByTabId.delete(request.tabId) - if (!requestBelongsToCurrentRecoveryGeneration(request)) { + pendingRetryByTabId.delete(request.tabId) + const currentRequest = [...requestsByInstanceId.values()].find( + isCurrentTerminalRecoveryRequest + ) + if (!currentRequest) { return } - void requestTerminalPaneRecovery(request) + void requestTerminalPaneRecovery(currentRequest) }, Math.max(delayMs, 1_000) ) - pendingRetryTimerByTabId.set(request.tabId, timer) + pendingRetryByTabId.set(request.tabId, { + timer, + requestsByInstanceId + }) } function cancelPendingRecoveryRetry(tabId: string): void { - const timer = pendingRetryTimerByTabId.get(tabId) - if (timer !== undefined) { - clearTimeout(timer) - pendingRetryTimerByTabId.delete(tabId) + const pendingRetry = pendingRetryByTabId.get(tabId) + if (pendingRetry !== undefined) { + clearTimeout(pendingRetry.timer) + pendingRetryByTabId.delete(tabId) } } @@ -125,7 +171,7 @@ function cancelPendingRecoveryRetry(tabId: string): void { * exited"), and remounting there would race it. */ export async function requestTerminalPaneRecovery(request: RecoveryRequest): Promise { - if (!requestBelongsToCurrentRecoveryGeneration(request)) { + if (!isCurrentTerminalRecoveryRequest(request)) { return false } const budget = recoveryBudget(request.tabId, Date.now()) @@ -160,7 +206,7 @@ export async function requestTerminalPaneRecovery(request: RecoveryRequest): Pro } // Re-check the budget across the await: a concurrent detector may have // already consumed it for this tab. - if (!requestBelongsToCurrentRecoveryGeneration(request)) { + if (!isCurrentTerminalRecoveryRequest(request)) { return false } const recheck = recoveryBudget(request.tabId, Date.now()) @@ -223,8 +269,10 @@ export async function requestTerminalPaneRecovery(request: RecoveryRequest): Pro export function _resetTerminalPaneRecoveryForTests(): void { recoveryTimestampsByTabId.clear() recoveryGenerationByTabId.clear() - for (const timer of pendingRetryTimerByTabId.values()) { - clearTimeout(timer) + activeTerminalRecoveryInstanceIds.clear() + nextTerminalRecoveryInstanceId = 0 + for (const pendingRetry of pendingRetryByTabId.values()) { + clearTimeout(pendingRetry.timer) } - pendingRetryTimerByTabId.clear() + pendingRetryByTabId.clear() } diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.ts index 744fc1d0f60..a2226262be4 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.ts @@ -8,11 +8,29 @@ import { resolvePaneLinkCwd, resolvePaneSeedCwd, resolveQueuedInitialCwd, + resetTerminalKeyboardProtocolAfterInterrupt, shouldDetachPaneTransportOnUnmount, splitPaneWithOneShotStartup, suppressIntentionalPaneCloseExit } from './use-terminal-pane-lifecycle' +describe('resetTerminalKeyboardProtocolAfterInterrupt', () => { + it('does not write to an xterm whose pipeline is certified dead', async () => { + const { _resetWritePipelineHealthForTests, notifyUndeliverableWrite } = + await import('@/lib/pane-manager/terminal-write-pipeline-health') + const terminal = { write: vi.fn() } + try { + notifyUndeliverableWrite(terminal, 'replay-wedged') + + resetTerminalKeyboardProtocolAfterInterrupt(terminal as never) + + expect(terminal.write).not.toHaveBeenCalled() + } finally { + _resetWritePipelineHealthForTests(terminal) + } + }) +}) + describe('splitPaneWithOneShotStartup', () => { it('only exposes startup to the intentional split and clears it afterwards', () => { const deps: { startup?: { command: string; env?: Record } | null } = { diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts index 3430580e68d..9e194f664c6 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts @@ -15,7 +15,10 @@ import { resolveTerminalCursorInactiveStyle } from '@/lib/pane-manager/pane-terminal-options' import { normalizeDesktopTerminalScrollbackRows } from '../../../../shared/terminal-scrollback-policy' -import { configureTerminalOutputBacklogCap } from '@/lib/pane-manager/pane-terminal-output-scheduler' +import { + configureTerminalOutputBacklogCap, + writeTerminalOutput +} from '@/lib/pane-manager/pane-terminal-output-scheduler' import { normalizeTerminalLineHeight } from '../../../../shared/terminal-line-height-settings' import { normalizeTerminalTuiMouseWheelMultiplier } from '@/lib/pane-manager/pane-terminal-mouse-wheel' import { buildWindowsPtyCompatibilityOptions } from '@/lib/pane-manager/windows-pty-compatibility' @@ -132,6 +135,17 @@ import { shouldClearLaunchAgentForClosedPane } from './terminal-pane-close-identity' +export function resetTerminalKeyboardProtocolAfterInterrupt(terminal: Terminal): void { + // Use the guarded output path so a certified/throwing xterm cannot escape a + // keyboard handler or retain more writes while pane recovery is delayed. + writeTerminalOutput(terminal, RESET_KITTY_KEYBOARD_PROTOCOL, { + foreground: true, + // The interrupt itself already took the synchronous input path. Queue the + // renderer reset so it cannot flush a PTY backlog inside the key handler. + latencySensitive: false + }) +} + export function recordRuntimeCreatedTerminalPaneSplit( createdPane: unknown, args: { @@ -981,7 +995,7 @@ export function useTerminalPaneLifecycle({ pane.terminal.input(TERMINAL_INTERRUPT_INPUT) // Why: CLIs such as Codex can die on SIGINT before restoring // xterm's renderer-side Kitty flags, leaving the shell corrupted. - pane.terminal.write(RESET_KITTY_KEYBOARD_PROTOCOL) + resetTerminalKeyboardProtocolAfterInterrupt(pane.terminal) } else { pendingTerminalInterruptKeyup = false } diff --git a/src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.test.ts b/src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.test.ts index 4d260d03f4d..9fe3af69c1a 100644 --- a/src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.test.ts +++ b/src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.test.ts @@ -249,6 +249,22 @@ describe('pane terminal output scheduler', () => { } }) + it('does not probe a certified terminal while waiting for parsed output', async () => { + const { waitForTerminalOutputParsed } = await loadScheduler() + const { _resetWritePipelineHealthForTests, notifyUndeliverableWrite } = + await import('./terminal-write-pipeline-health') + const terminal = createTerminal() + try { + notifyUndeliverableWrite(terminal, 'replay-wedged') + + await waitForTerminalOutputParsed(terminal) + + expect(terminal.write).not.toHaveBeenCalled() + } finally { + _resetWritePipelineHealthForTests(terminal) + } + }) + it('credits an empty write immediately', async () => { const { writeTerminalOutput } = await loadScheduler() const terminal = createTerminal() diff --git a/src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.ts b/src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.ts index 2d2fca3ac6b..cbcb9f1b29a 100644 --- a/src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.ts +++ b/src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.ts @@ -1484,6 +1484,11 @@ export function registerTerminalBacklogRecovery( export function waitForTerminalOutputParsed(terminal: TerminalOutputTarget): Promise { flushTerminalOutput(terminal) + if (isTerminalWritePipelineCertifiedDead(terminal)) { + // A dead pipeline cannot settle; recovery owns it and serializers must not + // enqueue probe writes while a bounded remount retry is pending. + return Promise.resolve() + } return new Promise((resolve) => { let settled = false