From 091df2b6d8a2c9ef2c0555980c6f070a7ab01417 Mon Sep 17 00:00:00 2001 From: lior Date: Wed, 29 Jul 2026 11:11:45 +0300 Subject: [PATCH] fix(terminal): drain deferred output without a wake event --- docs/terminal-anti-flicker.md | 38 +++++++----------------- src/web/public/terminal-ui.js | 47 ++++++++++++++---------------- test/terminal-flush-budget.test.ts | 20 +++++++++++++ 3 files changed, 52 insertions(+), 53 deletions(-) diff --git a/docs/terminal-anti-flicker.md b/docs/terminal-anti-flicker.md index 82f45b1b9..ad7c38bdf 100644 --- a/docs/terminal-anti-flicker.md +++ b/docs/terminal-anti-flicker.md @@ -43,38 +43,22 @@ const syncData = DEC_SYNC_START + data + DEC_SYNC_END; this.broadcast('session:terminal', { id: sessionId, data: syncData }); ``` -## Client-Side Implementation (`app.js`) +## Client-Side Implementation (`terminal-ui.js`) ### `batchTerminalWrite(data)` 1. Checks if flicker filter is enabled (optional, per-session) 2. If flicker filter active: buffers screen-clear patterns (`ESC[2J`, `ESC[H ESC[J`, `ESC[nA`) 3. Accumulates data in `pendingWrites` -4. Schedules `requestAnimationFrame` if not already scheduled -5. On rAF callback: checks for incomplete sync blocks (start without end) -6. If incomplete: waits up to 50ms via `syncWaitTimeout` -7. Calls `flushPendingWrites()` when complete - -### `extractSyncSegments(data)` - -- Parses DEC 2026 markers, returns array of content segments -- Content before sync blocks returned as-is -- Content inside sync blocks returned without markers -- Incomplete blocks (start without end) returned with marker for next chunk +4. Calls `_scheduleTerminalWriteFlush()` if no flush is pending +5. The yielded callback clears its scheduled flag before calling `flushPendingWrites()` +6. Large batches schedule their own next chunk until the queue is empty ### `flushPendingWrites()` -```javascript -const segments = extractSyncSegments(this.pendingWrites); -this.pendingWrites = ''; // Clear before writing -for (const segment of segments) { - if (segment && !segment.startsWith(DEC_SYNC_START)) { - terminal.write(segment); // Skip incomplete blocks (start with marker) - } -} -``` - -Note: Segments starting with `DEC_SYNC_START` are incomplete blocks awaiting more data. These are skipped (discarded if timeout forces flush). +- Joins the queued terminal data and passes DEC 2026 markers through to xterm.js 6, which handles synchronized output natively. +- Writes at most 32KB per yield for Codex and 64KB for other modes. +- Requeues the remainder and immediately schedules another safe yield. A final large response therefore drains without waiting for another SSE event. ### `chunkedTerminalWrite(buffer, chunkSize=128KB)` @@ -116,17 +100,15 @@ When detected, buffers 50ms of subsequent output before flushing atomically. ## Edge Cases -- **Incomplete sync blocks**: 50ms timeout forces flush (content discarded to prevent freeze) +- **Incomplete sync blocks**: xterm.js retains synchronized output until its closing marker - **Large buffers**: Chunked writing prevents UI freeze - **Server shutdown**: Skips batching via `_isStopping` flag - **Session switch**: Clears flicker filter state, pending writes, and sync timeout (prevents cross-session data bleed) - **SSE reconnect**: `handleInit()` clears all pending write state -**Trade-off:** If a sync block is split across SSE packets and the end marker doesn't arrive within 50ms, the incomplete content is discarded. This prioritizes responsiveness over completeness. In practice this is rare since the server always sends complete `SYNC_START...SYNC_END` pairs and SSE typically delivers them atomically. - ## DEC Mode 2026 Compatibility -Terminals that natively support DEC 2026 will buffer and render atomically. Terminals that don't support it ignore the escape sequences harmlessly. xterm.js doesn't support DEC 2026 natively, so the client implements its own buffering by parsing the markers. +Terminals that natively support DEC 2026 buffer and render atomically. Codeman uses xterm.js 6, so the client passes the markers through instead of parsing or discarding partial blocks. **Supporting terminals:** WezTerm, Kitty, Ghostty, iTerm2 3.5+, Windows Terminal, VSCode terminal @@ -135,4 +117,4 @@ Terminals that natively support DEC 2026 will buffer and render atomically. Term | File | Key Functions | |------|---------------| | `src/web/server.ts` | `batchTerminalData()`, `flushTerminalBatches()`, `broadcast()` | -| `src/web/public/app.js` | `batchTerminalWrite()`, `extractSyncSegments()`, `flushPendingWrites()`, `flushFlickerBuffer()`, `chunkedTerminalWrite()` | +| `src/web/public/terminal-ui.js` | `batchTerminalWrite()`, `_scheduleTerminalWriteFlush()`, `flushPendingWrites()`, `flushFlickerBuffer()`, `chunkedTerminalWrite()` | diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index a369ac4bd..fdbffe6cf 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -341,7 +341,7 @@ Object.assign(CodemanApp.prototype, { // WebGL renderer for GPU-accelerated terminal rendering. // Previously caused "page unresponsive" crashes from synchronous GPU stalls, - // but the 48KB/frame flush cap in flushPendingWrites() now prevents + // but the mode-aware 32/64KB frame cap in flushPendingWrites() now prevents // oversized terminal.write() calls that triggered the stalls. // Disable with ?nowebgl URL param if GPU issues return. // Auto-fallback: _initWebGL installs a long-task watchdog that disables @@ -2349,17 +2349,26 @@ Object.assign(CodemanApp.prototype, { // Accumulate raw data (may contain DEC 2026 markers) this.pendingWrites.push(data); + this._scheduleTerminalWriteFlush(); + }, - if (!this.writeFrameScheduled) { - this.writeFrameScheduled = true; - this._safeYield(() => { - // xterm.js 6.0 handles DEC 2026 sync markers natively — it buffers - // content between 2026h/2026l and renders atomically. No need for - // client-side incomplete-block detection; just flush every frame. - this.flushPendingWrites(); - this.writeFrameScheduled = false; - }); - } + /** + * Schedule one render-budgeted terminal flush. + * + * Clear the scheduled flag before flushing so flushPendingWrites() can queue + * another yield when a large final batch leaves bytes behind. Keeping the + * flag set through the flush stranded that remainder until unrelated output + * arrived, which looked like truncated responses and idle shell commands. + */ + _scheduleTerminalWriteFlush() { + if (this.writeFrameScheduled || this.pendingWrites.length === 0) return; + this.writeFrameScheduled = true; + this._safeYield(() => { + this.writeFrameScheduled = false; + // xterm.js 6.0 handles DEC 2026 sync markers natively — it buffers + // content between 2026h/2026l and renders atomically. + this.flushPendingWrites(); + }); }, /** @@ -2375,13 +2384,7 @@ Object.assign(CodemanApp.prototype, { this.flickerFilterActive = false; // Trigger a normal flush - if (!this.writeFrameScheduled) { - this.writeFrameScheduled = true; - this._safeYield(() => { - this.flushPendingWrites(); - this.writeFrameScheduled = false; - }); - } + this._scheduleTerminalWriteFlush(); }, /** @@ -2530,13 +2533,7 @@ Object.assign(CodemanApp.prototype, { this.terminal.write(joined.slice(0, MAX_FRAME_BYTES)); this.pendingWrites.push(joined.slice(MAX_FRAME_BYTES)); deferred = true; - if (!this.writeFrameScheduled) { - this.writeFrameScheduled = true; - this._safeYield(() => { - this.flushPendingWrites(); - this.writeFrameScheduled = false; - }); - } + this._scheduleTerminalWriteFlush(); } if ( preserveViewportY !== null && diff --git a/test/terminal-flush-budget.test.ts b/test/terminal-flush-budget.test.ts index 1af721ab7..229cebff4 100644 --- a/test/terminal-flush-budget.test.ts +++ b/test/terminal-flush-budget.test.ts @@ -47,6 +47,26 @@ function loadTerminalUiHarness(mode: string) { } describe('terminal flush budget', () => { + it('drains a large final batch without waiting for unrelated terminal output', () => { + const { app, writes } = loadTerminalUiHarness('codex'); + const scheduled: Array<() => void> = []; + app._safeYield = (callback: () => void) => { + scheduled.push(callback); + }; + app.isTerminalAtBottom = () => true; + + app.batchTerminalWrite('x'.repeat(96 * 1024)); + expect(scheduled).toHaveLength(1); + + while (scheduled.length > 0) { + scheduled.shift()?.(); + } + + expect(writes.map((write) => write.length)).toEqual([32 * 1024, 32 * 1024, 32 * 1024]); + expect(app.pendingWrites).toEqual([]); + expect(app.writeFrameScheduled).toBe(false); + }); + it('uses a smaller first-frame write budget for Codex output to reduce renderer stalls', () => { const { app, writes } = loadTerminalUiHarness('codex'); app.pendingWrites.push('x'.repeat(96 * 1024));