From 2b2d64ce7b2d7588e0c756502356740a96c2e722 Mon Sep 17 00:00:00 2001 From: Neil Date: Sat, 19 Sep 2026 15:05:38 -0700 Subject: [PATCH] fix(terminal): keep budget-free terminal flushes exhaustive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dense SGR pacing made flushTerminalOutput() stop early — it refused to write while a batch was in flight and stopped after one 4 KiB batch. The replay and shutdown-capture callers pass no char budget precisely because they write straight to xterm next, so a partial flush let stale queued bytes land on top of a freshly restored screen. Treat a flush with no maxChars as an ordering barrier: skip both pacing exits and drain the whole queue. Budgeted callers keep dense pacing. --- .../pane-terminal-output-flusher.ts | 8 ++- .../pane-terminal-output-scheduler.test.ts | 64 +++++++++++++++++-- .../pane-terminal-output-writer.ts | 5 +- 3 files changed, 65 insertions(+), 12 deletions(-) diff --git a/src/renderer/src/lib/pane-manager/pane-terminal-output-flusher.ts b/src/renderer/src/lib/pane-manager/pane-terminal-output-flusher.ts index d1b00eb0a6e..adab9cc7a23 100644 --- a/src/renderer/src/lib/pane-manager/pane-terminal-output-flusher.ts +++ b/src/renderer/src/lib/pane-manager/pane-terminal-output-flusher.ts @@ -42,6 +42,8 @@ export function flushTerminalOutputImpl( if (!entry) { return } + // Why: a budget-free flush is an ordering barrier (replay paint, shutdown capture, parse settle) whose caller writes straight to xterm next, so it must submit every queued byte; only budgeted callers tolerate dense pacing. + const explicitFullDrain = options?.maxChars === undefined queuedByTerminal.delete(terminal) if (isTerminalWritePipelineCertifiedDead(terminal)) { discardDetachedQueueEntry(entry) @@ -52,7 +54,7 @@ export function flushTerminalOutputImpl( queuedByTerminal.set(terminal, entry) return } - if (!canDrainQueueEntry(entry)) { + if (!explicitFullDrain && !canDrainQueueEntry(entry)) { queuedByTerminal.set(terminal, entry) scheduleDrain(0) return @@ -71,7 +73,7 @@ export function flushTerminalOutputImpl( let flushedChars = 0 let queuedWrite = takeQueuedChunk( entry, - entry.denseSgr ? DENSE_SGR_CHUNK_CHARS : BACKGROUND_CHUNK_CHARS + !explicitFullDrain && entry.denseSgr ? DENSE_SGR_CHUNK_CHARS : BACKGROUND_CHUNK_CHARS ) while (queuedWrite) { flushedChars += queuedWrite.data.length @@ -140,7 +142,7 @@ export function flushTerminalOutputImpl( if (options?.maxChars !== undefined && flushedChars >= options.maxChars) { break } - if (entry.denseSgr) { + if (!explicitFullDrain && entry.denseSgr) { break } queuedWrite = takeQueuedChunk(entry, BACKGROUND_CHUNK_CHARS) 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 a01e02cc79e..09f0d6939f1 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 @@ -455,15 +455,67 @@ describe('pane terminal output scheduler', () => { writeTerminalOutput(terminal, input, { foreground: false }) vi.advanceTimersByTime(50) + expect(terminal.write).toHaveBeenCalledTimes(1) writeTerminalOutput(terminal, 'echo', { foreground: true }) + // The foreground write's budget-free flush submits the retained dense tail first. + expect(terminal.write.mock.calls.map(([data]) => data)).toEqual([ + input.slice(0, 4 * 1024), + input.slice(4 * 1024), + 'echo' + ]) + }) + + it('drains a dense entry completely when the flush carries no char budget', async () => { + vi.useFakeTimers() + const { flushTerminalOutput, queuedByTerminal, writeTerminalOutput } = await loadScheduler() + const terminal = createTerminal() + const parsed: (() => void)[] = [] + terminal.write.mockImplementation((_data: string, callback?: () => void) => { + if (callback) { + parsed.push(callback) + } + }) + const dense = Array.from( + { length: 1_300 }, + (_, index) => `\x1b[${30 + (index % 8)}mX\x1b[0m` + ).join('') + + writeTerminalOutput(terminal, dense, { foreground: false }) + vi.advanceTimersByTime(50) expect(terminal.write).toHaveBeenCalledTimes(1) - parsed.shift()?.() - vi.advanceTimersByTime(0) - expect(terminal.write.mock.calls[1]?.[0]).toBe(input.slice(4 * 1024)) - parsed.shift()?.() - vi.advanceTimersByTime(0) - expect(terminal.write.mock.calls[2]?.[0]).toBe('echo') + + // The replay/shutdown-capture callers write straight to xterm next, so the + // flush must leave nothing queued behind the batch still in flight. + flushTerminalOutput(terminal) + + expect(queuedByTerminal.has(terminal)).toBe(false) + expect(terminal.write.mock.calls.map(([data]) => data).join('')).toBe(dense) + }) + + it('keeps pacing a dense entry when the flush carries a char budget', async () => { + vi.useFakeTimers() + const { flushTerminalOutput, queuedByTerminal, writeTerminalOutput } = await loadScheduler() + const terminal = createTerminal() + const parsed: (() => void)[] = [] + terminal.write.mockImplementation((_data: string, callback?: () => void) => { + if (callback) { + parsed.push(callback) + } + }) + const dense = Array.from( + { length: 1_300 }, + (_, index) => `\x1b[${30 + (index % 8)}mX\x1b[0m` + ).join('') + + writeTerminalOutput(terminal, dense, { foreground: false }) + vi.advanceTimersByTime(50) + expect(terminal.write).toHaveBeenCalledTimes(1) + + flushTerminalOutput(terminal, { maxChars: 64 * 1024 }) + + expect(terminal.write).toHaveBeenCalledTimes(1) + expect(queuedByTerminal.has(terminal)).toBe(true) }) it('promotes large background backlogs to high-priority drains', async () => { diff --git a/src/renderer/src/lib/pane-manager/pane-terminal-output-writer.ts b/src/renderer/src/lib/pane-manager/pane-terminal-output-writer.ts index 5d6347c58a0..ba278a76843 100644 --- a/src/renderer/src/lib/pane-manager/pane-terminal-output-writer.ts +++ b/src/renderer/src/lib/pane-manager/pane-terminal-output-writer.ts @@ -212,9 +212,8 @@ export function writeTerminalOutputImpl( flushTerminalOutputImpl(terminal) const remaining = queuedByTerminal.get(terminal) if (remaining) { - // A dense batch may still be parsing when an explicit flush returns. - // Keep the new foreground bytes behind its retained tail so terminal - // byte order remains intact. + // Keep the new bytes behind anything the flush could not submit, so + // terminal byte order survives a retained tail. remaining.onBackgroundBacklogDropped = options.onBackgroundBacklogDropped remaining.highPriority = true enqueueChunk(remaining, data, {