From f286702c6234648629939609a7ca474b693bebe1 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 23 May 2026 13:19:38 -0700 Subject: [PATCH] fix: hold pty batches during reveal snapshots (#2707) --- src/main/ipc/pty.test.ts | 130 +++++++++++++++--- src/main/ipc/pty.ts | 66 ++++++--- .../hidden-terminal-output-state.test.ts | 27 ++++ .../hidden-terminal-output-state.ts | 44 ++++-- .../use-terminal-pane-global-effects.ts | 6 +- 5 files changed, 223 insertions(+), 50 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/hidden-terminal-output-state.test.ts diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 4e1778ccc..dadb41f77 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -3381,24 +3381,23 @@ describe('registerPtyHandlers', () => { ) }) - it('flushes pending renderer PTY batches around headless serialization', async () => { + it('discards pending renderer PTY batches when the headless snapshot includes them', async () => { vi.useFakeTimers() try { const mockProc = createMockProc() spawnMock.mockReturnValue(mockProc.proc) + let resolveSnapshot!: (snapshot: { data: string; cols: number; rows: number }) => void const runtime = { setPtyController: vi.fn(), onPtySpawned: vi.fn(), onPtyData: vi.fn(), onPtyExit: vi.fn(), preAllocateHandleForPty: vi.fn(), - serializeHeadlessTerminalBufferForRenderer: vi.fn(async () => { + serializeHeadlessTerminalBufferForRenderer: vi.fn(() => { mockProc.emitData('during-serialize') - return { - data: 'headless', - cols: 100, - rows: 30 - } + return new Promise<{ data: string; cols: number; rows: number }>((resolve) => { + resolveSnapshot = resolve + }) }) } handlers.clear() @@ -3411,16 +3410,23 @@ describe('registerPtyHandlers', () => { mainWindow.webContents.send.mockClear() mockProc.emitData('before-serialize') - await handlers.get('pty:serializeHeadlessBuffer')!(null, { id: spawnResult.id }) + const pending = handlers.get('pty:serializeHeadlessBuffer')!(null, { id: spawnResult.id }) + vi.advanceTimersByTime(8) + expect(mainWindow.webContents.send).not.toHaveBeenCalledWith( + 'pty:data', + expect.objectContaining({ id: spawnResult.id }) + ) + resolveSnapshot({ + data: 'headless', + cols: 100, + rows: 30 + }) + await pending - expect(mainWindow.webContents.send).toHaveBeenCalledWith('pty:data', { - id: spawnResult.id, - data: 'before-serialize' - }) - expect(mainWindow.webContents.send).toHaveBeenCalledWith('pty:data', { - id: spawnResult.id, - data: 'during-serialize' - }) + expect(mainWindow.webContents.send).not.toHaveBeenCalledWith( + 'pty:data', + expect.objectContaining({ id: spawnResult.id }) + ) expect(runtime.serializeHeadlessTerminalBufferForRenderer).toHaveBeenCalledWith( spawnResult.id, {} @@ -3432,5 +3438,97 @@ describe('registerPtyHandlers', () => { vi.useRealTimers() } }) + + it('releases pending renderer PTY batches when headless serialization cannot snapshot', async () => { + vi.useFakeTimers() + try { + const mockProc = createMockProc() + spawnMock.mockReturnValue(mockProc.proc) + let resolveSnapshot!: (snapshot: null) => void + const runtime = { + setPtyController: vi.fn(), + onPtySpawned: vi.fn(), + onPtyData: vi.fn(), + onPtyExit: vi.fn(), + preAllocateHandleForPty: vi.fn(), + serializeHeadlessTerminalBufferForRenderer: vi.fn(() => { + mockProc.emitData('during-serialize') + return new Promise((resolve) => { + resolveSnapshot = resolve + }) + }) + } + handlers.clear() + registerPtyHandlers(mainWindow as never, runtime as never) + const spawnResult = (await handlers.get('pty:spawn')!(null, { + cols: 80, + rows: 24, + cwd: '/tmp' + })) as { id: string } + mainWindow.webContents.send.mockClear() + mockProc.emitData('before-serialize') + + const pending = handlers.get('pty:serializeHeadlessBuffer')!(null, { id: spawnResult.id }) + vi.advanceTimersByTime(8) + expect(mainWindow.webContents.send).not.toHaveBeenCalledWith( + 'pty:data', + expect.objectContaining({ id: spawnResult.id }) + ) + resolveSnapshot(null) + await pending + + expect(mainWindow.webContents.send).toHaveBeenCalledWith('pty:data', { + id: spawnResult.id, + data: 'before-serializeduring-serialize' + }) + mainWindow.webContents.send.mockClear() + vi.advanceTimersByTime(8) + expect(mainWindow.webContents.send).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('releases pending renderer PTY batches when headless serialization throws', async () => { + vi.useFakeTimers() + try { + const mockProc = createMockProc() + spawnMock.mockReturnValue(mockProc.proc) + const runtime = { + setPtyController: vi.fn(), + onPtySpawned: vi.fn(), + onPtyData: vi.fn(), + onPtyExit: vi.fn(), + preAllocateHandleForPty: vi.fn(), + serializeHeadlessTerminalBufferForRenderer: vi.fn(async () => { + mockProc.emitData('during-serialize') + throw new Error('snapshot failed') + }) + } + handlers.clear() + registerPtyHandlers(mainWindow as never, runtime as never) + const spawnResult = (await handlers.get('pty:spawn')!(null, { + cols: 80, + rows: 24, + cwd: '/tmp' + })) as { id: string } + mainWindow.webContents.send.mockClear() + mockProc.emitData('before-serialize') + + await expect( + handlers.get('pty:serializeHeadlessBuffer')!(null, { id: spawnResult.id }) + ).rejects.toThrow('snapshot failed') + + expect(mainWindow.webContents.send).toHaveBeenCalledWith('pty:data', { + id: spawnResult.id, + data: 'before-serializeduring-serialize' + }) + mainWindow.webContents.send.mockClear() + vi.advanceTimersByTime(8) + expect(mainWindow.webContents.send).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) }) }) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 6bb5a1cb9..d75f3c3ea 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -659,6 +659,7 @@ export function registerPtyHandlers( // throughput. Keystroke echo/redraws bypass this below because agent TUIs // already spend tens of ms producing their redraw. const pendingData = new Map() + const headlessSnapshotHeldPtyIds = new Set() const trustedTerminalHandleEnv = new Set() let flushTimer: ReturnType | null = null const PTY_BATCH_INTERVAL_MS = 8 @@ -667,6 +668,15 @@ export function registerPtyHandlers( const INTERACTIVE_OUTPUT_WINDOW_MS = 100 const INTERACTIVE_OUTPUT_MAX_CHARS = 1024 + const hasFlushablePendingData = (): boolean => { + for (const id of pendingData.keys()) { + if (!headlessSnapshotHeldPtyIds.has(id)) { + return true + } + } + return false + } + const flushPendingData = (): void => { flushTimer = null if (mainWindow.isDestroyed()) { @@ -674,29 +684,40 @@ export function registerPtyHandlers( return } for (const [id, data] of pendingData) { + if (headlessSnapshotHeldPtyIds.has(id)) { + continue + } mainWindow.webContents.send('pty:data', { id, data }) + pendingData.delete(id) + } + if (hasFlushablePendingData()) { + flushTimer = setTimeout(flushPendingData, PTY_BATCH_INTERVAL_MS) } - pendingData.clear() } const clearFlushTimerIfIdle = (): void => { - if (pendingData.size > 0 || flushTimer === null) { + if (hasFlushablePendingData() || flushTimer === null) { return } clearTimeout(flushTimer) flushTimer = null } - const flushPendingDataForPty = (id: string): void => { - const data = pendingData.get(id) - if (!data) { + const sendPtyDataToRenderer = (id: string, data: string): void => { + if (!data || mainWindow.isDestroyed()) { return } - pendingData.delete(id) - if (!mainWindow.isDestroyed()) { - mainWindow.webContents.send('pty:data', { id, data }) + mainWindow.webContents.send('pty:data', { id, data }) + } + + const takePendingDataForPty = (id: string): string => { + const data = pendingData.get(id) + if (!data) { + return '' } + pendingData.delete(id) clearFlushTimerIfIdle() + return data } // Why: extracted so the "Restart daemon" flow can rebind against the fresh @@ -737,7 +758,7 @@ export function registerPtyHandlers( nextData.length <= INTERACTIVE_OUTPUT_MAX_CHARS && lastInputAt !== undefined && performance.now() - lastInputAt <= INTERACTIVE_OUTPUT_WINDOW_MS - if (isInteractiveOutput) { + if (isInteractiveOutput && !headlessSnapshotHeldPtyIds.has(payload.id)) { pendingData.delete(payload.id) clearFlushTimerIfIdle() // Why: agent TUIs redraw small prompt regions after every keystroke. @@ -749,7 +770,7 @@ export function registerPtyHandlers( return } pendingData.set(payload.id, nextData) - if (!flushTimer) { + if (!flushTimer && hasFlushablePendingData()) { flushTimer = setTimeout(flushPendingData, PTY_BATCH_INTERVAL_MS) } }) @@ -1848,12 +1869,25 @@ export function registerPtyHandlers( opts.scrollbackRows = Math.floor(args.scrollbackRows) } // Why: hidden-pane reveal uses the headless snapshot as the authoritative - // paint. Flush any ≤8ms main-process PTY batch before and after the - // snapshot so renderer-side hydration can drop its duplicate fallback - // queue without losing bytes that already reached the headless model. - flushPendingDataForPty(args.id) - const snapshot = await runtime.serializeHeadlessTerminalBufferForRenderer(args.id, opts) - flushPendingDataForPty(args.id) + // paint. Hold any ≤8ms main-process PTY batches while serializing: a + // successful headless snapshot already contains them, while a null + // snapshot must release them so the renderer fallback can replay them. + const pendingBeforeSnapshot = takePendingDataForPty(args.id) + headlessSnapshotHeldPtyIds.add(args.id) + let snapshot: { data: string; cols: number; rows: number } | null + try { + snapshot = await runtime.serializeHeadlessTerminalBufferForRenderer(args.id, opts) + } catch (err) { + const pendingDuringSnapshot = takePendingDataForPty(args.id) + headlessSnapshotHeldPtyIds.delete(args.id) + sendPtyDataToRenderer(args.id, pendingBeforeSnapshot + pendingDuringSnapshot) + throw err + } + const pendingDuringSnapshot = takePendingDataForPty(args.id) + headlessSnapshotHeldPtyIds.delete(args.id) + if (!snapshot) { + sendPtyDataToRenderer(args.id, pendingBeforeSnapshot + pendingDuringSnapshot) + } return snapshot } ) diff --git a/src/renderer/src/components/terminal-pane/hidden-terminal-output-state.test.ts b/src/renderer/src/components/terminal-pane/hidden-terminal-output-state.test.ts new file mode 100644 index 000000000..077c43a05 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/hidden-terminal-output-state.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it, vi } from 'vitest' + +import { + consumeHiddenTerminalHydration, + markHiddenTerminalFallbackReplayed, + queueHiddenTerminalOutput +} from './hidden-terminal-output-state' + +function createTerminal() { + return { + write: vi.fn() + } +} + +describe('hidden terminal output state', () => { + it('preserves reveal-time fallback output when trimming drops pre-hydration chunks', () => { + const terminal = createTerminal() + queueHiddenTerminalOutput(terminal, 'pty-1', 'before-reveal') + const hydration = consumeHiddenTerminalHydration(terminal) + expect(hydration).not.toBeNull() + + const retainedDuringHydration = 'x'.repeat(512 * 1024) + queueHiddenTerminalOutput(terminal, 'pty-1', `dropped-prefix${retainedDuringHydration}`) + + expect(markHiddenTerminalFallbackReplayed(terminal, hydration!)).toBe(retainedDuringHydration) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/hidden-terminal-output-state.ts b/src/renderer/src/components/terminal-pane/hidden-terminal-output-state.ts index 5df8494c9..dc07509c7 100644 --- a/src/renderer/src/components/terminal-pane/hidden-terminal-output-state.ts +++ b/src/renderer/src/components/terminal-pane/hidden-terminal-output-state.ts @@ -6,11 +6,17 @@ type TerminalOutputTarget = { type HiddenTerminalState = { ptyId: string - chunks: string[] + chunks: HiddenTerminalChunk[] bytes: number needsHydration: boolean hydrating: boolean hydrationToken: number + nextChunkSeq: number +} + +type HiddenTerminalChunk = { + seq: number + data: string } type HiddenTerminalOutputDebugSnapshot = { @@ -31,7 +37,7 @@ export type HiddenTerminalHydration = { ptyId: string fallbackData: string token: number - fallbackChunkCount: number + fallbackLastSeq: number } const MAX_FALLBACK_BYTES = 512 * 1024 @@ -78,17 +84,17 @@ function trimFallback(state: HiddenTerminalState): void { if (!dropped) { continue } - state.bytes -= dropped.length + state.bytes -= dropped.data.length if (debugEnabled) { - debugState.droppedBytes += dropped.length + debugState.droppedBytes += dropped.data.length } } if (state.bytes > MAX_FALLBACK_BYTES && state.chunks.length === 1) { const chunk = state.chunks[0] - const keepFrom = Math.max(0, chunk.length - MAX_FALLBACK_BYTES) + const keepFrom = Math.max(0, chunk.data.length - MAX_FALLBACK_BYTES) if (keepFrom > 0) { - state.chunks[0] = chunk.slice(keepFrom) - state.bytes = state.chunks[0].length + state.chunks[0] = { ...chunk, data: chunk.data.slice(keepFrom) } + state.bytes = state.chunks[0].data.length if (debugEnabled) { debugState.droppedBytes += keepFrom } @@ -113,12 +119,13 @@ export function queueHiddenTerminalOutput( bytes: 0, needsHydration: false, hydrating: false, - hydrationToken: 0 + hydrationToken: 0, + nextChunkSeq: 1 } hiddenStateByTerminal.set(terminal, state) } state.needsHydration = true - state.chunks.push(data) + state.chunks.push({ seq: state.nextChunkSeq++, data }) state.bytes += data.length trimFallback(state) if (debugEnabled) { @@ -143,23 +150,29 @@ export function consumeHiddenTerminalHydration( } return { ptyId: state.ptyId, - fallbackData: state.chunks.join(''), + fallbackData: state.chunks.map((chunk) => chunk.data).join(''), token: state.hydrationToken, - fallbackChunkCount: state.chunks.length + fallbackLastSeq: state.chunks.at(-1)?.seq ?? 0 } } function finishHydration( terminal: TerminalOutputTarget, token: number, - consumedChunkCount: number + fallbackLastSeq: number, + collectQueuedDuringHydration: boolean ): string { exposeDebugApi() const state = hiddenStateByTerminal.get(terminal) if (!state || state.hydrationToken !== token) { return '' } - const queuedDuringHydration = state.chunks.slice(consumedChunkCount).join('') + const queuedDuringHydration = collectQueuedDuringHydration + ? state.chunks + .filter((chunk) => chunk.seq > fallbackLastSeq) + .map((chunk) => chunk.data) + .join('') + : '' state.chunks.length = 0 state.bytes = 0 state.needsHydration = false @@ -175,7 +188,8 @@ export function markHiddenTerminalFallbackReplayed( const queuedDuringHydration = finishHydration( terminal, hydration.token, - hydration.fallbackChunkCount + hydration.fallbackLastSeq, + true ) if (debugEnabled) { debugState.fallbackReplayCount++ @@ -187,7 +201,7 @@ export function markHiddenTerminalHydrated( terminal: TerminalOutputTarget, hydration: HiddenTerminalHydration ): string { - return finishHydration(terminal, hydration.token, hydration.fallbackChunkCount) + return finishHydration(terminal, hydration.token, hydration.fallbackLastSeq, false) } export function cancelHiddenTerminalHydration( diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.ts index 145517456..e549fe2d1 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.ts @@ -122,9 +122,9 @@ export function useTerminalPaneGlobalEffects({ // guard, so terminal query replies do not leak into the shell. replayIntoTerminal(pane, replayingPanesRef, '\x1b[2J\x1b[3J\x1b[H') replayIntoTerminal(pane, replayingPanesRef, snapshot.data) - // Why: the main serializer flushes pending PTY batches around the - // headless snapshot. Any renderer bytes queued during this window - // are already represented in the authoritative snapshot. + // Why: the main serializer holds pending PTY batches while taking + // the headless snapshot and discards them only after a successful + // snapshot, so this fallback queue would duplicate the replay. markHiddenTerminalHydrated(pane.terminal, hydration) return }