From 2c860af9ff524cafedd13a3717e6adea4b486fa4 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Thu, 2 Jul 2026 16:54:02 -0400 Subject: [PATCH] Harden xterm write pipeline against sync-throw wedge that freezes panes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A synchronous exception escaping xterm's WriteBuffer loop permanently wedges that terminal: _innerWrite has no try/catch around the parse action or the write-completion callback, the tail re-schedule never runs, and write() only re-arms on an empty buffer. The pane stops rendering and, if a replay was in flight, the replay guard latches and pty-connection's onData silently eats every keystroke — matching the field reports (Discord #performance, issue #2836: content visible, shell alive, daemon output.log flat). Both vectors verified against vendored xterm 6.1.0-beta.287 in xterm-write-buffer-stall.repro.test.ts. Three layers of defense: - Guard every write-completion callback Orca hands xterm at the two choke points (writeForegroundTerminalChunk, writeBackgroundTerminalChunk), with settle and onParsed guarded separately so a WebGL/renderer failure during viewport settle cannot starve the replay-guard release. - Guard all throwing-capable custom parser handlers (DA1, OSC 10/11, CSI ?h/?l mode reports, OSC 52 clipboard, OSC 7 cwd), degrading a throw to "not handled" — same escape class as terminal-link-provider-guard.ts. - Replay-guard watchdog: each engagement releases exactly once, from xterm's completion or a 10s watchdog, so a lost completion (wedged pipeline, disposed-terminal race) cannot latch the guard on a live pane; replayIntoTerminalAsync resolves on either path so restore chains cannot hang. Force-releases record a crash breadcrumb. All guard trips record rate-capped crash breadcrumbs, so the next field occurrence names the throwing stack instead of failing silently. Co-authored-by: Orca --- .../terminal-pane/replay-guard.test.ts | 118 +++++++++++++++++- .../components/terminal-pane/replay-guard.ts | 85 +++++++++---- .../terminal-pane/terminal-appearance.ts | 85 +++++++------ .../terminal-capability-replies.ts | 70 ++++++----- .../terminal-parser-handler-guard.test.ts | 108 ++++++++++++++++ .../terminal-parser-handler-guard.ts | 44 +++++++ .../use-terminal-pane-lifecycle.ts | 35 +++--- .../pane-terminal-foreground-render-settle.ts | 28 +++-- .../pane-terminal-output-scheduler.ts | 27 ++-- .../xterm-write-callback-guard.test.ts | 110 ++++++++++++++++ .../xterm-write-callback-guard.ts | 40 ++++++ 11 files changed, 622 insertions(+), 128 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/terminal-parser-handler-guard.test.ts create mode 100644 src/renderer/src/components/terminal-pane/terminal-parser-handler-guard.ts create mode 100644 src/renderer/src/lib/pane-manager/xterm-write-callback-guard.test.ts create mode 100644 src/renderer/src/lib/pane-manager/xterm-write-callback-guard.ts 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 6431b773bd2..8722c18c5d0 100644 --- a/src/renderer/src/components/terminal-pane/replay-guard.test.ts +++ b/src/renderer/src/components/terminal-pane/replay-guard.test.ts @@ -1,6 +1,27 @@ -import { describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { ManagedPane } from '@/lib/pane-manager/pane-manager' -import { isPaneReplaying, replayIntoTerminal, type ReplayingPanesRef } from './replay-guard' +import { + isPaneReplaying, + replayIntoTerminal, + replayIntoTerminalAsync, + type ReplayingPanesRef +} from './replay-guard' + +const mocks = vi.hoisted(() => ({ + recordRendererCrashBreadcrumb: vi.fn() +})) + +vi.mock('@/lib/crash-diagnostics', () => ({ + recordRendererCrashBreadcrumb: mocks.recordRendererCrashBreadcrumb +})) + +beforeEach(() => { + mocks.recordRendererCrashBreadcrumb.mockClear() +}) + +afterEach(() => { + vi.useRealTimers() +}) function makeRef(): ReplayingPanesRef { return { current: new Map() } as ReplayingPanesRef @@ -169,3 +190,96 @@ 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. + vi.useFakeTimers() + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + const ref = makeRef() + const { pane } = makeFakePane(1) + + replayIntoTerminal(pane, ref, 'restored bytes', 10_000) + expect(isPaneReplaying(ref, 1)).toBe(true) + + vi.advanceTimersByTime(9_999) + expect(isPaneReplaying(ref, 1)).toBe(true) + + vi.advanceTimersByTime(1) + expect(isPaneReplaying(ref, 1)).toBe(false) + expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_replay_guard_watchdog_release', + { paneId: 1 } + ) + } finally { + errorSpy.mockRestore() + } + }) + + it('does not double-decrement when the completion arrives after the watchdog fired', () => { + 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) + + vi.advanceTimersByTime(1_000) + // Watchdog released only the lost engagement; the healthy one still holds. + expect(isPaneReplaying(ref, 1)).toBe(true) + + terminal.flush() + 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 { + errorSpy.mockRestore() + } + }) + + it('does not fire the watchdog after a normal completion', () => { + vi.useFakeTimers() + const ref = makeRef() + const { pane, terminal } = makeFakePane(1) + + replayIntoTerminal(pane, ref, 'healthy', 1_000) + terminal.flush() + expect(isPaneReplaying(ref, 1)).toBe(false) + + vi.advanceTimersByTime(60_000) + expect(mocks.recordRendererCrashBreadcrumb).not.toHaveBeenCalled() + }) + + it('resolves replayIntoTerminalAsync via the watchdog so restore chains cannot hang', async () => { + vi.useFakeTimers() + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + const ref = makeRef() + const { pane } = makeFakePane(1) + + const replayDone = replayIntoTerminalAsync(pane, ref, 'restored bytes', 1_000) + let resolved = false + void replayDone.then(() => { + resolved = true + }) + + await vi.advanceTimersByTimeAsync(1_000) + expect(resolved).toBe(true) + expect(isPaneReplaying(ref, 1)).toBe(false) + } finally { + errorSpy.mockRestore() + } + }) +}) diff --git a/src/renderer/src/components/terminal-pane/replay-guard.ts b/src/renderer/src/components/terminal-pane/replay-guard.ts index 0229bde316d..5f082c1b399 100644 --- a/src/renderer/src/components/terminal-pane/replay-guard.ts +++ b/src/renderer/src/components/terminal-pane/replay-guard.ts @@ -1,5 +1,6 @@ import type { ManagedPane } from '@/lib/pane-manager/pane-manager' import { writeForegroundTerminalChunk } from '@/lib/pane-manager/pane-terminal-foreground-render-settle' +import { recordRendererCrashBreadcrumb } from '@/lib/crash-diagnostics' // Why: xterm.js auto-responds to terminal query sequences (DA1 `CSI c`, // DECRQM `CSI ? Ps $ p`, OSC 10/11 color queries, focus events, CPR) by @@ -27,10 +28,60 @@ import { writeForegroundTerminalChunk } from '@/lib/pane-manager/pane-terminal-f 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 + export function isPaneReplaying(ref: ReplayingPanesRef, paneId: number): boolean { return (ref.current.get(paneId) ?? 0) > 0 } +/** + * 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. + */ +function engageReplayGuard( + map: Map, + paneId: number, + watchdogMs: 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 => { + if (released) { + return + } + released = true + if (watchdog !== null) { + clearTimeout(watchdog) + watchdog = null + } + const remaining = (map.get(paneId) ?? 1) - 1 + if (remaining <= 0) { + map.delete(paneId) + } else { + map.set(paneId, remaining) + } + if (reason === 'watchdog') { + console.error( + `[terminal] replay guard force-released for pane ${paneId} — xterm never completed the replay write (wedged write pipeline?)` + ) + recordRendererCrashBreadcrumb('terminal_replay_guard_watchdog_release', { paneId }) + } + onRelease?.() + } + watchdog = setTimeout(() => release('watchdog'), watchdogMs) + return () => release('parsed') +} + /** Writes `data` into the pane's terminal with the replay guard engaged, * so xterm's auto-replies to embedded query sequences do not leak to the * shell as input. The counter increments/decrements so nested replays @@ -38,53 +89,39 @@ export function isPaneReplaying(ref: ReplayingPanesRef, paneId: number): boolean export function replayIntoTerminal( pane: ManagedPane, replayingPanesRef: ReplayingPanesRef, - data: string + data: string, + watchdogMs: number = REPLAY_GUARD_WATCHDOG_MS ): void { if (!data) { return } - const map = replayingPanesRef.current - map.set(pane.id, (map.get(pane.id) ?? 0) + 1) - const onParsed = (): void => { - const remaining = (map.get(pane.id) ?? 1) - 1 - if (remaining <= 0) { - map.delete(pane.id) - } else { - map.set(pane.id, remaining) - } - } + const releaseParsed = engageReplayGuard(replayingPanesRef.current, pane.id, watchdogMs) // 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, { forceViewportRefresh: true, followupViewportRefresh: true, - onParsed + onParsed: releaseParsed }) } export function replayIntoTerminalAsync( pane: ManagedPane, replayingPanesRef: ReplayingPanesRef, - data: string + data: string, + watchdogMs: number = REPLAY_GUARD_WATCHDOG_MS ): Promise { if (!data) { return Promise.resolve() } - const map = replayingPanesRef.current - map.set(pane.id, (map.get(pane.id) ?? 0) + 1) 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) writeForegroundTerminalChunk(pane.terminal, data, { forceViewportRefresh: true, followupViewportRefresh: true, - onParsed: () => { - const remaining = (map.get(pane.id) ?? 1) - 1 - if (remaining <= 0) { - map.delete(pane.id) - } else { - map.set(pane.id, remaining) - } - resolve() - } + onParsed: releaseParsed }) }) } diff --git a/src/renderer/src/components/terminal-pane/terminal-appearance.ts b/src/renderer/src/components/terminal-pane/terminal-appearance.ts index 2ec75fc3fac..86e2999dae3 100644 --- a/src/renderer/src/components/terminal-pane/terminal-appearance.ts +++ b/src/renderer/src/components/terminal-pane/terminal-appearance.ts @@ -10,6 +10,7 @@ import { resolveEffectiveTerminalAppearance } from '@/lib/terminal-theme' import { buildFontFamily } from './layout-serialization' +import { guardParserHandler } from './terminal-parser-handler-guard' import { captureScrollState, restoreScrollState, safeFit } from '@/lib/pane-manager/pane-tree-ops' import { normalizeTerminalFastScrollSensitivity, @@ -55,50 +56,56 @@ export function installMode2031Handlers(deps: Mode2031HandlerDeps): IDisposable[ // continue processing the same sequence, so compound sequences like // `CSI ?25;2031h` still update cursor visibility correctly. return [ - deps.parser.registerCsiHandler({ prefix: '?', final: 'h' }, (params) => { - if (hasMode2031(params)) { - // Why: a restored xterm buffer may contain `CSI ?2031h` emitted by - // the previous session's TUI (e.g. Claude Code). Replaying that - // buffer runs this handler, and without the guard we'd push - // `CSI ?997;1n` via transport.sendInput into a fresh shell that has - // no TUI consuming it — zsh then echoes the literal escape sequence - // onto the prompt. The replay guard in pty-connection.ts only covers - // xterm's own onData auto-replies, not handler-triggered sends, so - // gate explicitly here. We also skip recording the subscribe bit: - // the fresh shell is not actually subscribed, so a later theme flip - // must not push either. A real TUI that starts up after restore will - // re-emit `?2031h` itself and register normally. - // - // Why this broad guard is safe across all replay sources: the only - // replay path that can carry raw `?2031h` is cold-restore scrollback - // (pty-connection.ts), which is disk-replayed PTY output against a - // fresh shell — the case this guard targets. Daemon snapshot payloads - // (`rehydrateSequences + SerializeAddon.serialize()`) and persisted - // scrollback (`SerializeAddon.serialize()`) never contain `?2031`: - // SerializeAddon's _serializeModes whitelists only ?1h/?66h/?2004h/ - // [4h/?6h/?45h/?1004h/?7l/mouse modes/?25l, and buildRehydrateSequences - // emits only ?1049h/?2004h/?1h/mouse reporting modes. If xterm ever - // adds ?2031 to that whitelist, this guard would start suppressing - // legitimate subscribes during snapshot reattach — revisit then. - if (deps.isReplaying()) { - return false + deps.parser.registerCsiHandler( + { prefix: '?', final: 'h' }, + guardParserHandler('csi-mode2031-subscribe', (params) => { + if (hasMode2031(params)) { + // Why: a restored xterm buffer may contain `CSI ?2031h` emitted by + // the previous session's TUI (e.g. Claude Code). Replaying that + // buffer runs this handler, and without the guard we'd push + // `CSI ?997;1n` via transport.sendInput into a fresh shell that has + // no TUI consuming it — zsh then echoes the literal escape sequence + // onto the prompt. The replay guard in pty-connection.ts only covers + // xterm's own onData auto-replies, not handler-triggered sends, so + // gate explicitly here. We also skip recording the subscribe bit: + // the fresh shell is not actually subscribed, so a later theme flip + // must not push either. A real TUI that starts up after restore will + // re-emit `?2031h` itself and register normally. + // + // Why this broad guard is safe across all replay sources: the only + // replay path that can carry raw `?2031h` is cold-restore scrollback + // (pty-connection.ts), which is disk-replayed PTY output against a + // fresh shell — the case this guard targets. Daemon snapshot payloads + // (`rehydrateSequences + SerializeAddon.serialize()`) and persisted + // scrollback (`SerializeAddon.serialize()`) never contain `?2031`: + // SerializeAddon's _serializeModes whitelists only ?1h/?66h/?2004h/ + // [4h/?6h/?45h/?1004h/?7l/mouse modes/?25l, and buildRehydrateSequences + // emits only ?1049h/?2004h/?1h/mouse reporting modes. If xterm ever + // adds ?2031 to that whitelist, this guard would start suppressing + // legitimate subscribes during snapshot reattach — revisit then. + if (deps.isReplaying()) { + return false + } + deps.paneMode2031.set(deps.paneId, true) + deps.onSubscribe() } - deps.paneMode2031.set(deps.paneId, true) - deps.onSubscribe() - } - return false - }), + return false + }) + ), // Why no replay guard on the unsubscribe branch: clearing stale bookkeeping // is harmless. We only push CSI 997 on subscribe, never on unsubscribe, so // even if a cold-restore replay carries `?2031l`, this handler just deletes // map entries that a later real `?2031h` will re-populate normally. - deps.parser.registerCsiHandler({ prefix: '?', final: 'l' }, (params) => { - if (hasMode2031(params)) { - deps.paneMode2031.delete(deps.paneId) - deps.paneLastThemeMode.delete(deps.paneId) - } - return false - }) + deps.parser.registerCsiHandler( + { prefix: '?', final: 'l' }, + guardParserHandler('csi-mode2031-unsubscribe', (params) => { + if (hasMode2031(params)) { + deps.paneMode2031.delete(deps.paneId) + deps.paneLastThemeMode.delete(deps.paneId) + } + return false + }) + ) ] } diff --git a/src/renderer/src/components/terminal-pane/terminal-capability-replies.ts b/src/renderer/src/components/terminal-pane/terminal-capability-replies.ts index 8308fc8b210..37353c7727c 100644 --- a/src/renderer/src/components/terminal-pane/terminal-capability-replies.ts +++ b/src/renderer/src/components/terminal-pane/terminal-capability-replies.ts @@ -5,6 +5,7 @@ import { terminalOscColorQuerySlotsForBody, type TerminalOscColorQuerySlot } from '../../../../shared/terminal-osc-color-reply' +import { guardParserHandler } from './terminal-parser-handler-guard' export const DEFAULT_DA1_RESPONSE = '\x1b[?1;2c' export const CONPTY_DA1_RESPONSE = '\x1b[?61;4c' @@ -118,37 +119,46 @@ export function installTerminalCapabilityReplyHandlers( deps: TerminalCapabilityRepliesDeps ): IDisposable { const disposables = [ - deps.parser.registerCsiHandler({ final: 'c' }, (params) => { - if (!isPrimaryDeviceAttributesQuery(params)) { - return false - } - // Why: restored scrollback may contain old DA1 queries; answering those - // into the fresh shell recreates the stray-input leak this handler fixes. - if (!deps.isReplaying()) { - deps.sendInput(deps.da1Response ?? DEFAULT_DA1_RESPONSE) - } - return true - }), - deps.parser.registerOscHandler(10, (data) => { - const slots = terminalOscColorQuerySlotsForBody(10, data.trim()) - if (!slots) { - return false - } - if (deps.isReplaying()) { + deps.parser.registerCsiHandler( + { final: 'c' }, + guardParserHandler('csi-da1', (params) => { + if (!isPrimaryDeviceAttributesQuery(params)) { + return false + } + // Why: restored scrollback may contain old DA1 queries; answering those + // into the fresh shell recreates the stray-input leak this handler fixes. + if (!deps.isReplaying()) { + deps.sendInput(deps.da1Response ?? DEFAULT_DA1_RESPONSE) + } return true - } - return sendTerminalOscColorQueryRepliesForSlots(slots, deps.terminal, deps.sendInput) - }), - deps.parser.registerOscHandler(11, (data) => { - const slots = terminalOscColorQuerySlotsForBody(11, data.trim()) - if (!slots) { - return false - } - if (deps.isReplaying()) { - return true - } - return sendTerminalOscColorQueryRepliesForSlots(slots, deps.terminal, deps.sendInput) - }) + }) + ), + deps.parser.registerOscHandler( + 10, + guardParserHandler('osc-10-color-query', (data) => { + const slots = terminalOscColorQuerySlotsForBody(10, data.trim()) + if (!slots) { + return false + } + if (deps.isReplaying()) { + return true + } + return sendTerminalOscColorQueryRepliesForSlots(slots, deps.terminal, deps.sendInput) + }) + ), + deps.parser.registerOscHandler( + 11, + guardParserHandler('osc-11-color-query', (data) => { + const slots = terminalOscColorQuerySlotsForBody(11, data.trim()) + if (!slots) { + return false + } + if (deps.isReplaying()) { + return true + } + return sendTerminalOscColorQueryRepliesForSlots(slots, deps.terminal, deps.sendInput) + }) + ) ] return { diff --git a/src/renderer/src/components/terminal-pane/terminal-parser-handler-guard.test.ts b/src/renderer/src/components/terminal-pane/terminal-parser-handler-guard.test.ts new file mode 100644 index 00000000000..2c84ec15990 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-parser-handler-guard.test.ts @@ -0,0 +1,108 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { Terminal } from '@xterm/headless' +import { + _resetParserHandlerReportsForTests, + guardParserHandler +} from './terminal-parser-handler-guard' + +const mocks = vi.hoisted(() => ({ + recordRendererCrashBreadcrumb: vi.fn() +})) + +vi.mock('@/lib/crash-diagnostics', () => ({ + recordRendererCrashBreadcrumb: mocks.recordRendererCrashBreadcrumb +})) + +beforeEach(() => { + mocks.recordRendererCrashBreadcrumb.mockClear() + _resetParserHandlerReportsForTests() +}) + +afterEach(() => { + vi.useRealTimers() +}) + +describe('guardParserHandler', () => { + it('passes arguments and return value through for healthy handlers', () => { + const handler = vi.fn((data: string) => data === 'handled') + const guarded = guardParserHandler('test-handler', handler) + expect(guarded('handled')).toBe(true) + expect(guarded('other')).toBe(false) + expect(handler).toHaveBeenCalledTimes(2) + expect(mocks.recordRendererCrashBreadcrumb).not.toHaveBeenCalled() + }) + + it('degrades a throwing handler to "not handled" and reports a breadcrumb', () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + const guarded = guardParserHandler('exploding-handler', () => { + throw new TypeError('synthetic handler failure') + }) + expect(guarded()).toBe(false) + expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_parser_handler_error', + expect.objectContaining({ + handler: 'exploding-handler', + errorName: 'TypeError', + errorMessage: 'synthetic handler failure' + }) + ) + } finally { + errorSpy.mockRestore() + } + }) + + it('caps repeated reports per handler', () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + const guarded = guardParserHandler('spammy-handler', () => { + throw new Error('always fails') + }) + for (let i = 0; i < 20; i++) { + guarded() + } + expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledTimes(5) + } finally { + errorSpy.mockRestore() + } + }) + + it('keeps the real xterm write pipeline alive through a throwing handler (inverse of the wedge repro)', () => { + // Why: xterm-write-buffer-stall.repro.test.ts proves an UNguarded throwing + // handler permanently wedges the WriteBuffer. This is the fix's proof: + // the same poison sequence through a GUARDED handler keeps completing + // writes. + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + vi.useFakeTimers() + const term = new Terminal({ allowProposedApi: true }) + const completed: string[] = [] + term.parser.registerCsiHandler( + { final: 'z' }, + guardParserHandler('poisoned-csi', () => { + throw new Error('synthetic parser handler failure') + }) + ) + + term.write('\x1b[z', () => { + completed.push('poisoned') + }) + term.write('after', () => { + completed.push('after') + }) + expect(() => vi.runAllTimers()).not.toThrow() + + term.write('later', () => { + completed.push('later') + }) + vi.runAllTimers() + expect(completed).toEqual(['poisoned', 'after', 'later']) + expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_parser_handler_error', + expect.objectContaining({ handler: 'poisoned-csi' }) + ) + } finally { + errorSpy.mockRestore() + } + }) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-parser-handler-guard.ts b/src/renderer/src/components/terminal-pane/terminal-parser-handler-guard.ts new file mode 100644 index 00000000000..ceade0005a5 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-parser-handler-guard.ts @@ -0,0 +1,44 @@ +import { recordRendererCrashBreadcrumb } from '@/lib/crash-diagnostics' + +// Why: xterm's EscapeSequenceParser invokes custom CSI/OSC handlers +// synchronously inside WriteBuffer._innerWrite, which has no try/catch. A +// handler throw skips the loop's tail re-schedule, and write() only re-arms +// on an EMPTY buffer — one throw permanently freezes the pane (output stops; +// a pending replay guard never releases and silently eats keystrokes). +// Verified against vendored xterm 6.1.0-beta.287 in +// xterm-write-buffer-stall.repro.test.ts. Same escape class as +// terminal-link-provider-guard.ts, applied to parser handlers. +const MAX_REPORTS_PER_HANDLER = 5 +const reportCountsByHandler = new Map() + +/** + * Wrap a custom parser handler so a synchronous throw is reported and + * degraded to "not handled" (xterm falls through to the previous/default + * handler) instead of wedging the terminal's write pipeline. + */ +export function guardParserHandler( + handlerName: string, + handler: (...args: HandlerArgs) => boolean +): (...args: HandlerArgs) => boolean { + return (...args: HandlerArgs): boolean => { + try { + return handler(...args) + } catch (error: unknown) { + const reported = reportCountsByHandler.get(handlerName) ?? 0 + if (reported < MAX_REPORTS_PER_HANDLER) { + reportCountsByHandler.set(handlerName, reported + 1) + console.error(`[terminal] parser handler "${handlerName}" threw`, error) + recordRendererCrashBreadcrumb('terminal_parser_handler_error', { + handler: handlerName, + errorName: error instanceof Error ? error.name : typeof error, + errorMessage: error instanceof Error ? error.message : String(error) + }) + } + return false + } + } +} + +export function _resetParserHandlerReportsForTests(): void { + reportCountsByHandler.clear() +} 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 a5d9172855b..933d5558982 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 @@ -58,6 +58,7 @@ import { import { handleOsc52ClipboardRequest } from './osc52-clipboard' import { showOsc52ClipboardBlockedToast } from './osc52-clipboard-blocked-toast' import { parseOsc7 } from './parse-osc7' +import { guardParserHandler } from './terminal-parser-handler-guard' import { resolveTerminalJisYenInput } from './terminal-jis-yen-input' import { installTerminalImeCompositionTracker } from './terminal-ime-composition-tracker' import { @@ -780,12 +781,15 @@ export function useTerminalPaneLifecycle({ // both the enabled and disabled paths so xterm doesn't fall // through to any other OSC 52 handler and so our intentional drop // in the disabled path is explicit. - const osc52Disposable = pane.terminal.parser.registerOscHandler(52, (data) => - handleOsc52ClipboardRequest(data, { - allowClipboardWrite: settingsRef.current?.terminalAllowOsc52Clipboard === true, - writeClipboardText: window.api.ui.writeClipboardText, - onBlockedWrite: showOsc52ClipboardBlockedToast - }) + const osc52Disposable = pane.terminal.parser.registerOscHandler( + 52, + guardParserHandler('osc-52-clipboard', (data) => + handleOsc52ClipboardRequest(data, { + allowClipboardWrite: settingsRef.current?.terminalAllowOsc52Clipboard === true, + writeClipboardText: window.api.ui.writeClipboardText, + onBlockedWrite: showOsc52ClipboardBlockedToast + }) + ) ) osc52DisposablesRef.current.set(pane.id, osc52Disposable) @@ -811,14 +815,17 @@ export function useTerminalPaneLifecycle({ confirmed: false }) } - const osc7Disposable = pane.terminal.parser.registerOscHandler(7, (data) => { - const parsedCwd = parseOsc7(data, { uncHost: osc7UncHost }) - if (parsedCwd) { - const confirmed = !isPaneReplaying(replayingPanesRef, pane.id) - paneCwdRef.current.set(pane.id, { cwd: parsedCwd, confirmed }) - } - return true - }) + const osc7Disposable = pane.terminal.parser.registerOscHandler( + 7, + guardParserHandler('osc-7-cwd', (data) => { + const parsedCwd = parseOsc7(data, { uncHost: osc7UncHost }) + if (parsedCwd) { + const confirmed = !isPaneReplaying(replayingPanesRef, pane.id) + paneCwdRef.current.set(pane.id, { cwd: parsedCwd, confirmed }) + } + return true + }) + ) osc7DisposablesRef.current.set(pane.id, osc7Disposable) // Why: let host-handled keys bypass xterm's kitty CSI-u encoder. diff --git a/src/renderer/src/lib/pane-manager/pane-terminal-foreground-render-settle.ts b/src/renderer/src/lib/pane-manager/pane-terminal-foreground-render-settle.ts index 93b848e7450..eab1caa497d 100644 --- a/src/renderer/src/lib/pane-manager/pane-terminal-foreground-render-settle.ts +++ b/src/renderer/src/lib/pane-manager/pane-terminal-foreground-render-settle.ts @@ -1,3 +1,5 @@ +import { runGuardedWriteCompletionStep } from './xterm-write-callback-guard' + export type ForegroundTerminalOutputTarget = { buffer?: { active?: { @@ -131,18 +133,24 @@ export function writeForegroundTerminalChunk( const beforeWriteViewport = options.forceViewportRefresh ? captureViewportSnapshot(terminal) : null - try { - terminal.write(data, () => { - if (beforeWriteViewport) { - settleForegroundRender(terminal, beforeWriteViewport, options) - } - options.onParsed?.() - }) - } catch { + // Why guarded steps: this callback runs inside xterm's WriteBuffer loop, + // where an escaping throw permanently wedges the terminal (see + // xterm-write-callback-guard.ts). Guard settle and onParsed separately so a + // renderer/WebGL failure during settle can't starve the replay-guard release. + const runCompletionSteps = (): void => { if (beforeWriteViewport) { - settleForegroundRender(terminal, beforeWriteViewport, options) + runGuardedWriteCompletionStep('foreground-render-settle', () => + settleForegroundRender(terminal, beforeWriteViewport, options) + ) } - options.onParsed?.() + if (options.onParsed) { + runGuardedWriteCompletionStep('foreground-on-parsed', options.onParsed) + } + } + try { + terminal.write(data, runCompletionSteps) + } catch { + runCompletionSteps() } } 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 c43fc8cf161..dfd67350fe0 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 @@ -11,6 +11,7 @@ import { captureTerminalWriteScrollIntent, enforceTerminalWriteScrollIntent } from './terminal-scroll-intent' +import { runGuardedWriteCompletionStep } from './xterm-write-callback-guard' type TerminalOutputTarget = ForegroundTerminalOutputTarget @@ -636,26 +637,34 @@ function writeBackgroundTerminalChunk( data: string, onParsed?: TerminalOutputParsedCallback ): void { + // Why guarded: these callbacks run inside xterm's WriteBuffer loop, where an + // escaping throw permanently wedges the terminal (see + // xterm-write-callback-guard.ts). + const runOnParsed = onParsed + ? (): void => runGuardedWriteCompletionStep('background-on-parsed', onParsed) + : undefined const scrollIntent = captureTerminalWriteScrollIntent(terminal) if (!scrollIntent) { - if (!onParsed || terminal.write.length < 2) { + if (!runOnParsed || terminal.write.length < 2) { terminal.write(data) - onParsed?.() + runOnParsed?.() return } - terminal.write(data, onParsed) + terminal.write(data, runOnParsed) return } + const runScrollIntentThenParsed = (): void => { + runGuardedWriteCompletionStep('background-scroll-intent', () => + enforceTerminalWriteScrollIntent(terminal, scrollIntent) + ) + runOnParsed?.() + } if (terminal.write.length < 2) { terminal.write(data) - enforceTerminalWriteScrollIntent(terminal, scrollIntent) - onParsed?.() + runScrollIntentThenParsed() return } - terminal.write(data, () => { - enforceTerminalWriteScrollIntent(terminal, scrollIntent) - onParsed?.() - }) + terminal.write(data, runScrollIntentThenParsed) } function writeForegroundTerminalChunkWithIntent( diff --git a/src/renderer/src/lib/pane-manager/xterm-write-callback-guard.test.ts b/src/renderer/src/lib/pane-manager/xterm-write-callback-guard.test.ts new file mode 100644 index 00000000000..d90f7358b8e --- /dev/null +++ b/src/renderer/src/lib/pane-manager/xterm-write-callback-guard.test.ts @@ -0,0 +1,110 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + _resetWriteCompletionReportsForTests, + runGuardedWriteCompletionStep +} from './xterm-write-callback-guard' +import { writeForegroundTerminalChunk } from './pane-terminal-foreground-render-settle' + +const mocks = vi.hoisted(() => ({ + recordRendererCrashBreadcrumb: vi.fn() +})) + +vi.mock('@/lib/crash-diagnostics', () => ({ + recordRendererCrashBreadcrumb: mocks.recordRendererCrashBreadcrumb +})) + +beforeEach(() => { + mocks.recordRendererCrashBreadcrumb.mockClear() + _resetWriteCompletionReportsForTests() +}) + +describe('runGuardedWriteCompletionStep', () => { + it('contains a synchronous throw and reports a breadcrumb', () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + expect(() => + runGuardedWriteCompletionStep('test-step', () => { + throw new RangeError('synthetic settle failure') + }) + ).not.toThrow() + expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_write_completion_error', + expect.objectContaining({ + context: 'test-step', + errorName: 'RangeError', + errorMessage: 'synthetic settle failure' + }) + ) + } finally { + errorSpy.mockRestore() + } + }) + + it('caps repeated reports per context so a throw-per-write loop cannot spam breadcrumbs', () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + for (let i = 0; i < 20; i++) { + runGuardedWriteCompletionStep('spammy-step', () => { + throw new Error('always fails') + }) + } + expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledTimes(5) + } finally { + errorSpy.mockRestore() + } + }) + + it('runs non-throwing steps transparently', () => { + const step = vi.fn() + runGuardedWriteCompletionStep('ok-step', step) + expect(step).toHaveBeenCalledTimes(1) + expect(mocks.recordRendererCrashBreadcrumb).not.toHaveBeenCalled() + }) +}) + +describe('writeForegroundTerminalChunk completion guarding', () => { + it('still releases onParsed when the settle step throws (replay-guard latch protection)', () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + const pendingCallbacks: (() => void)[] = [] + // Why a getter that throws only after the write is dispatched: it + // models renderer/buffer state failing between parse start and the + // post-parse viewport settle (refreshVisibleRowsNow self-catches, so + // the viewport comparison is the escaping surface). + let bufferAccessPoisoned = false + const realBuffer = { active: { cursorY: 0, baseY: 0, viewportY: 0 } } + const terminal = { + rows: 24, + get buffer() { + if (bufferAccessPoisoned) { + throw new Error('synthetic buffer access failure') + } + return realBuffer + }, + write: (_data: string, cb?: () => void) => { + if (cb) { + pendingCallbacks.push(cb) + } + } + } + const onParsed = vi.fn() + + writeForegroundTerminalChunk(terminal, 'restored bytes', { + forceViewportRefresh: true, + onParsed + }) + bufferAccessPoisoned = true + // Simulate xterm completing the parse: the completion callback must not + // let the settle throw escape into the WriteBuffer, and onParsed (the + // replay-guard release) must still run. + expect(() => pendingCallbacks.forEach((cb) => cb())).not.toThrow() + expect(onParsed).toHaveBeenCalledTimes(1) + expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_write_completion_error', + expect.objectContaining({ context: 'foreground-render-settle' }) + ) + } finally { + errorSpy.mockRestore() + } + }) +}) diff --git a/src/renderer/src/lib/pane-manager/xterm-write-callback-guard.ts b/src/renderer/src/lib/pane-manager/xterm-write-callback-guard.ts new file mode 100644 index 00000000000..fe1daa946c4 --- /dev/null +++ b/src/renderer/src/lib/pane-manager/xterm-write-callback-guard.ts @@ -0,0 +1,40 @@ +import { recordRendererCrashBreadcrumb } from '@/lib/crash-diagnostics' + +// Why: xterm's WriteBuffer._innerWrite invokes write-completion callbacks with +// no try/catch; a synchronous throw skips the loop's tail re-schedule, and +// write() only re-arms processing when the buffer is EMPTY — which a stalled +// buffer never is again. One escaping throw therefore permanently freezes the +// pane: output stops rendering and a pending replay guard never releases, so +// the pane silently eats every keystroke while the shell stays alive +// (Discord #performance / issue #2836). Verified against the vendored xterm +// 6.1.0-beta.287 in xterm-write-buffer-stall.repro.test.ts. +const MAX_REPORTS_PER_CONTEXT = 5 +const reportCountsByContext = new Map() + +/** + * Run one step of a write-completion callback so a synchronous throw cannot + * escape into xterm's WriteBuffer. Steps are guarded individually so an + * earlier step's failure (e.g. a WebGL refresh during viewport settle) cannot + * starve a later step (e.g. the replay-guard release). + */ +export function runGuardedWriteCompletionStep(context: string, step: () => void): void { + try { + step() + } catch (error: unknown) { + const reported = reportCountsByContext.get(context) ?? 0 + if (reported >= MAX_REPORTS_PER_CONTEXT) { + return + } + reportCountsByContext.set(context, reported + 1) + console.error(`[terminal] write-completion step "${context}" threw`, error) + recordRendererCrashBreadcrumb('terminal_write_completion_error', { + context, + errorName: error instanceof Error ? error.name : typeof error, + errorMessage: error instanceof Error ? error.message : String(error) + }) + } +} + +export function _resetWriteCompletionReportsForTests(): void { + reportCountsByContext.clear() +}