From bdf798b43990bb31376d45cb3ea69d342ea0a739 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Thu, 2 Jul 2026 17:27:59 -0400 Subject: [PATCH] Make replay-guard stall release probe-certified instead of time-based MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous stall watchdog blindly released the input guard after 10s. If a replay were genuinely still parsing on a starved machine, that early release could leak xterm's auto-replies into the shell — and into agent TUIs, where a leaked ESC reads as the user pressing Escape. Replace the blind release with a probe: when a completion looks overdue, enqueue an empty write behind the replay. xterm parses writes in order, so every outcome is provably safe: - probe parses after the replay completion ran: normal release already happened; probe is a no-op. - probe parses but the replay completion never ran: all replay bytes have parsed, no further auto-replies can exist — the completion was genuinely lost. Release + breadcrumb. - probe never parses (bounded wait): the pipeline is wedged, and a dead parser can never emit auto-replies, so releasing cannot leak input. Release + breadcrumb naming the pane as needing recovery. While the probe is pending — a slow-but-alive replay — the guard now HOLDS instead of releasing early; that case is pinned by a regression test. Co-authored-by: Orca --- .../terminal-pane/replay-guard.test.ts | 124 ++++++++++++++---- .../components/terminal-pane/replay-guard.ts | 97 ++++++++++---- 2 files changed, 170 insertions(+), 51 deletions(-) diff --git a/src/renderer/src/components/terminal-pane/replay-guard.test.ts b/src/renderer/src/components/terminal-pane/replay-guard.test.ts index 8722c18c5d0..a4e468d59b0 100644 --- a/src/renderer/src/components/terminal-pane/replay-guard.test.ts +++ b/src/renderer/src/components/terminal-pane/replay-guard.test.ts @@ -191,28 +191,55 @@ describe('replay-guard', () => { }) }) -describe('replay-guard watchdog', () => { - it('force-releases the guard when xterm never completes the write (wedged pipeline repro)', () => { - // Why: a sync throw escaping xterm's WriteBuffer loop drops the pending - // write completion forever (xterm-write-buffer-stall.repro.test.ts). - // Without the watchdog the guard latches and pty-connection.ts onData - // silently eats every keystroke on a live pane. +describe('replay-guard stall handling (probe-certified release)', () => { + it('HOLDS the guard while a slow replay is still parsing — a probe is queued, never a blind release', () => { + // Why this is the load-bearing safety test: a time-based release here + // would leak xterm auto-replies into the shell (and a leaked ESC into an + // agent TUI reads as the user pressing Escape). The guard must only + // release when the pipeline itself proves the replay parsed. + vi.useFakeTimers() + const ref = makeRef() + const { pane, terminal } = makeFakePane(1) + + replayIntoTerminal(pane, ref, 'slow but alive', 1_000) + expect(isPaneReplaying(ref, 1)).toBe(true) + + // Stall check fires: an empty probe write is enqueued behind the replay. + vi.advanceTimersByTime(1_000) + expect(terminal.lastData).toEqual(['slow but alive', '']) + // Probe is pending → replay genuinely still parsing → guard holds. + expect(isPaneReplaying(ref, 1)).toBe(true) + vi.advanceTimersByTime(999) + expect(isPaneReplaying(ref, 1)).toBe(true) + + // Parsing finishes: FIFO runs the replay completion first (normal + // release), then the probe completion as a no-op. + terminal.flush() + expect(isPaneReplaying(ref, 1)).toBe(false) + expect(ref.current.has(1)).toBe(false) + expect(mocks.recordRendererCrashBreadcrumb).not.toHaveBeenCalled() + + vi.advanceTimersByTime(120_000) + expect(ref.current.has(1)).toBe(false) + }) + + it('releases when the probe parses but the replay completion was lost, and reports it', () => { vi.useFakeTimers() const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) try { const ref = makeRef() - const { pane } = makeFakePane(1) + const { pane, terminal } = makeFakePane(1) - replayIntoTerminal(pane, ref, 'restored bytes', 10_000) - expect(isPaneReplaying(ref, 1)).toBe(true) + replayIntoTerminal(pane, ref, 'restored bytes', 1_000) + terminal.pendingCallbacks.shift() // xterm lost the replay's completion + vi.advanceTimersByTime(1_000) // stall check → probe enqueued - vi.advanceTimersByTime(9_999) - expect(isPaneReplaying(ref, 1)).toBe(true) - - vi.advanceTimersByTime(1) + // The probe's completion firing certifies every earlier replay byte + // parsed — releasing now cannot leak auto-replies. + terminal.flush() expect(isPaneReplaying(ref, 1)).toBe(false) expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith( - 'terminal_replay_guard_watchdog_release', + 'terminal_replay_guard_lost_completion', { paneId: 1 } ) } finally { @@ -220,28 +247,68 @@ describe('replay-guard watchdog', () => { } }) - it('does not double-decrement when the completion arrives after the watchdog fired', () => { + it('releases after the probe itself never parses (wedged pipeline) and reports it', () => { + vi.useFakeTimers() + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + const ref = makeRef() + const { pane } = makeFakePane(1) + + replayIntoTerminal(pane, ref, 'restored bytes', 1_000) + vi.advanceTimersByTime(1_000) // stall check → probe enqueued + expect(isPaneReplaying(ref, 1)).toBe(true) + + // A wedged parser will never run the probe callback — and can never + // emit auto-replies either, so this bounded release cannot leak input. + vi.advanceTimersByTime(1_000) + expect(isPaneReplaying(ref, 1)).toBe(false) + expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_replay_guard_wedged_release', + { paneId: 1 } + ) + } finally { + errorSpy.mockRestore() + } + }) + + it('releases immediately when the probe write throws (terminal disposed mid-replay)', () => { vi.useFakeTimers() const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) try { const ref = makeRef() const { pane, terminal } = makeFakePane(1) - // First replay's completion is lost; second replay is healthy but slow. - replayIntoTerminal(pane, ref, 'lost completion', 1_000) - replayIntoTerminal(pane, ref, 'slow completion', 60_000) - terminal.pendingCallbacks.shift() // drop the first completion entirely - expect(isPaneReplaying(ref, 1)).toBe(true) - + replayIntoTerminal(pane, ref, 'restored bytes', 1_000) + terminal.write = () => { + throw new Error('terminal disposed') + } vi.advanceTimersByTime(1_000) - // Watchdog released only the lost engagement; the healthy one still holds. - expect(isPaneReplaying(ref, 1)).toBe(true) + expect(isPaneReplaying(ref, 1)).toBe(false) + expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_replay_guard_wedged_release', + { paneId: 1 } + ) + } finally { + errorSpy.mockRestore() + } + }) - terminal.flush() + it('keeps overlapping engagements independent through a lost completion', () => { + vi.useFakeTimers() + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + const ref = makeRef() + const { pane, terminal } = makeFakePane(1) + + replayIntoTerminal(pane, ref, 'lost completion', 1_000) + replayIntoTerminal(pane, ref, 'healthy completion', 60_000) + terminal.pendingCallbacks.shift() // drop only the first completion + vi.advanceTimersByTime(1_000) // first engagement's probe enqueued + + terminal.flush() // healthy completion + probe both parse expect(isPaneReplaying(ref, 1)).toBe(false) expect(ref.current.has(1)).toBe(false) - // A very late duplicate release must be a no-op. vi.advanceTimersByTime(120_000) expect(ref.current.has(1)).toBe(false) } finally { @@ -249,7 +316,7 @@ describe('replay-guard watchdog', () => { } }) - it('does not fire the watchdog after a normal completion', () => { + it('never probes after a normal completion', () => { vi.useFakeTimers() const ref = makeRef() const { pane, terminal } = makeFakePane(1) @@ -259,10 +326,11 @@ describe('replay-guard watchdog', () => { expect(isPaneReplaying(ref, 1)).toBe(false) vi.advanceTimersByTime(60_000) + expect(terminal.lastData).toEqual(['healthy']) expect(mocks.recordRendererCrashBreadcrumb).not.toHaveBeenCalled() }) - it('resolves replayIntoTerminalAsync via the watchdog so restore chains cannot hang', async () => { + it('resolves replayIntoTerminalAsync via the wedged path so restore chains cannot hang', async () => { vi.useFakeTimers() const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) try { @@ -275,7 +343,7 @@ describe('replay-guard watchdog', () => { resolved = true }) - await vi.advanceTimersByTimeAsync(1_000) + await vi.advanceTimersByTimeAsync(2_000) expect(resolved).toBe(true) expect(isPaneReplaying(ref, 1)).toBe(false) } finally { diff --git a/src/renderer/src/components/terminal-pane/replay-guard.ts b/src/renderer/src/components/terminal-pane/replay-guard.ts index 5f082c1b399..abaf8df0ffd 100644 --- a/src/renderer/src/components/terminal-pane/replay-guard.ts +++ b/src/renderer/src/components/terminal-pane/replay-guard.ts @@ -28,41 +28,60 @@ import { recordRendererCrashBreadcrumb } from '@/lib/crash-diagnostics' export type ReplayingPanesRef = React.RefObject> -// Why a watchdog: the decrement above only runs when xterm completes the -// write. A wedged WriteBuffer (sync throw escaping a parse handler or a -// write-completion callback — see xterm-write-buffer-stall.repro.test.ts) or -// a disposed-terminal race can drop that completion forever, leaving the -// guard latched on a live pane — which silently eats every keystroke -// (Discord #performance / issue #2836). Real replays parse in well under a -// second, so a 10s ceiling only ever fires on a genuinely lost completion. -const REPLAY_GUARD_WATCHDOG_MS = 10_000 +// Why stall handling exists: the decrement above only runs when xterm +// completes the write. A wedged WriteBuffer (sync throw escaping a parse +// handler or a write-completion callback — see +// xterm-write-buffer-stall.repro.test.ts) or a disposed-terminal race can +// drop that completion forever, leaving the guard latched on a live pane — +// which silently eats every keystroke (Discord #performance / issue #2836). +// +// Why release is probe-certified, never time-based: a blind timeout release +// while a slow replay is still parsing would let xterm's auto-replies leak +// into the shell — and into agent TUIs, where a leaked ESC reads as the user +// pressing Escape. Instead, when a completion looks overdue we enqueue an +// empty probe write. xterm parses writes in order, so only three states are +// possible, and release is provably safe in every state that releases: +// 1. probe completes, replay callback already ran → normal release won. +// 2. probe completes, replay callback never ran → every replay byte has +// parsed (FIFO), so no further auto-replies can exist; the completion +// was genuinely lost. Release. +// 3. probe never completes → the pipeline is +// wedged; a dead parser can never emit auto-replies, so releasing after +// a bounded wait cannot leak anything — and the pane needs recovery, +// which the breadcrumb reports. +// While the probe is pending (slow-but-alive replay), the guard HOLDS. +const REPLAY_GUARD_STALL_CHECK_MS = 10_000 export function isPaneReplaying(ref: ReplayingPanesRef, paneId: number): boolean { return (ref.current.get(paneId) ?? 0) > 0 } +type ReplayGuardWriteTarget = Pick + /** * Engage the replay counter for one write and return the release function. * Release runs exactly once — from xterm's write completion or, failing - * that, from the watchdog — so a lost completion cannot latch the guard. + * that, from the probe-certified stall path — so a lost completion cannot + * latch the guard. */ function engageReplayGuard( map: Map, paneId: number, - watchdogMs: number, + terminal: ReplayGuardWriteTarget, + stallCheckMs: number, onRelease?: () => void ): () => void { map.set(paneId, (map.get(paneId) ?? 0) + 1) let released = false - let watchdog: ReturnType | null = null - const release = (reason: 'parsed' | 'watchdog'): void => { + let timer: ReturnType | null = null + const release = (reason: 'parsed' | 'lost-completion' | 'wedged'): void => { if (released) { return } released = true - if (watchdog !== null) { - clearTimeout(watchdog) - watchdog = null + if (timer !== null) { + clearTimeout(timer) + timer = null } const remaining = (map.get(paneId) ?? 1) - 1 if (remaining <= 0) { @@ -70,15 +89,36 @@ function engageReplayGuard( } else { map.set(paneId, remaining) } - if (reason === 'watchdog') { + if (reason === 'lost-completion') { console.error( - `[terminal] replay guard force-released for pane ${paneId} — xterm never completed the replay write (wedged write pipeline?)` + `[terminal] replay guard released for pane ${paneId} — the probe write parsed but the replay completion never arrived (lost write callback)` ) - recordRendererCrashBreadcrumb('terminal_replay_guard_watchdog_release', { paneId }) + recordRendererCrashBreadcrumb('terminal_replay_guard_lost_completion', { paneId }) + } else if (reason === 'wedged') { + console.error( + `[terminal] replay guard released for pane ${paneId} — the probe write never parsed (wedged xterm write pipeline; pane likely needs recovery)` + ) + recordRendererCrashBreadcrumb('terminal_replay_guard_wedged_release', { paneId }) } onRelease?.() } - watchdog = setTimeout(() => release('watchdog'), watchdogMs) + const probeForStall = (): void => { + if (released) { + return + } + try { + // FIFO certification: this callback can only run after every replay + // byte queued before it has parsed (state 2 above). + terminal.write('', () => release('lost-completion')) + } catch { + // write threw (terminal disposed mid-replay): nothing will ever parse, + // so no auto-replies can leak. + release('wedged') + return + } + timer = setTimeout(() => release('wedged'), stallCheckMs) + } + timer = setTimeout(probeForStall, stallCheckMs) return () => release('parsed') } @@ -90,12 +130,17 @@ export function replayIntoTerminal( pane: ManagedPane, replayingPanesRef: ReplayingPanesRef, data: string, - watchdogMs: number = REPLAY_GUARD_WATCHDOG_MS + stallCheckMs: number = REPLAY_GUARD_STALL_CHECK_MS ): void { if (!data) { return } - const releaseParsed = engageReplayGuard(replayingPanesRef.current, pane.id, watchdogMs) + const releaseParsed = engageReplayGuard( + replayingPanesRef.current, + pane.id, + pane.terminal, + stallCheckMs + ) // Why: hidden/snapshot replay bypasses the live foreground write path, but // WebGL/canvas renderers still need a post-parse repaint to drop stale cells. writeForegroundTerminalChunk(pane.terminal, data, { @@ -109,7 +154,7 @@ export function replayIntoTerminalAsync( pane: ManagedPane, replayingPanesRef: ReplayingPanesRef, data: string, - watchdogMs: number = REPLAY_GUARD_WATCHDOG_MS + stallCheckMs: number = REPLAY_GUARD_STALL_CHECK_MS ): Promise { if (!data) { return Promise.resolve() @@ -117,7 +162,13 @@ export function replayIntoTerminalAsync( return new Promise((resolve) => { // Why resolve on either release path: callers await this to sequence // restore steps; a lost write completion must not hang the restore chain. - const releaseParsed = engageReplayGuard(replayingPanesRef.current, pane.id, watchdogMs, resolve) + const releaseParsed = engageReplayGuard( + replayingPanesRef.current, + pane.id, + pane.terminal, + stallCheckMs, + resolve + ) writeForegroundTerminalChunk(pane.terminal, data, { forceViewportRefresh: true, followupViewportRefresh: true,