From c4db3302259922a882ea24145977a1bdb7c8a325 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 7 Aug 2026 06:15:14 -0700 Subject: [PATCH] fix(terminal): release parent chunks pinned by drained output slices takeQueuedChunk stored the unconsumed remainder back into the queue as chunk.data.slice(remaining). V8 backs that with a SlicedString pointing at the whole parent, so a nearly drained 2 MB chunk kept all 2 MB alive to serve a few KB tail while queuedChars was charged only for the remainder. Fully consumed chunks were also left referenced until the 64-chunk compaction, so they retained their strings and callbacks while charging nothing at all. Track what each chunk really pins in retainedChars and copy the residual flat once it pins twice its own length. Halving bounds retention to ~2x the honest queued chars and keeps total copying linear in the original chunk, and the drain path measured no slower. Drained slots are replaced with an empty placeholder, which nothing below chunkIndex ever reads. Measured with 8 background terminals drained to a <=64 KB tail: 8,710,264 bytes retained before, 342,776 after. --- config/vitest.config.ts | 3 +- .../pane-terminal-output-scheduler.test.ts | 75 +++++++++++++++++++ .../pane-terminal-output-scheduler.ts | 27 ++++++- 3 files changed, 103 insertions(+), 2 deletions(-) diff --git a/config/vitest.config.ts b/config/vitest.config.ts index 735e1b2a0af..d23bee128c9 100644 --- a/config/vitest.config.ts +++ b/config/vitest.config.ts @@ -16,7 +16,8 @@ export default defineConfig({ test: { environment: 'node', // Why: Node 26's undefined Web Storage globals prevent Vitest from installing happy-dom's. - execArgv: ['--no-experimental-webstorage'], + // Why --expose-gc: retention tests need a deterministic collection point to measure what a queue really holds. + execArgv: ['--no-experimental-webstorage', '--expose-gc'], // Why: happy-dom drops MutationObserver callbacks on GC; keep them alive like a browser does. setupFiles: [resolve('config/scripts/happy-dom-mutation-observer-retention.ts')], include: [ 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 aaefe89f872..667b3f6555a 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 @@ -1677,4 +1677,79 @@ describe('pane terminal output scheduler', () => { vi.advanceTimersByTime(100) expect(throwing.write).toHaveBeenCalledTimes(1) }) + + describe('queue memory retention (STA-3567)', () => { + // Why 2 MB: comfortably above BACKGROUND_CHUNK_CHARS (16 K), so every drain leaves a residual slice. + const CHUNK_CHARS = 2 * 1024 * 1024 + const TERMINALS = 8 + + function collect(): number { + const gc = (globalThis as { gc?: () => void }).gc + if (!gc) { + throw new Error('global.gc unavailable - config/vitest.config.ts must pass --expose-gc') + } + gc() + gc() + return process.memoryUsage().heapUsed + } + + it('drops the parent chunk once only a small tail is still queued', async () => { + vi.useFakeTimers() + const { writeTerminalOutput } = await loadScheduler() + + // Why a hand-rolled write: vi.fn() retains every argument in mock.calls, which would + // dominate the measurement with the very bytes the queue is supposed to have released. + let writtenChars = 0 + const makeSink = (): { write: (data: string, callback?: () => void) => void } => ({ + write: (data: string, callback?: () => void) => { + writtenChars += data.length + callback?.() + } + }) + + // Distinct leading chars keep V8 from sharing any backing store between terminals. + const terminals = Array.from({ length: TERMINALS }, (_unused, index) => { + const terminal = makeSink() + writeTerminalOutput( + terminal, + String.fromCharCode(65 + index) + 'q'.repeat(CHUNK_CHARS - 1), + { foreground: false, latencySensitive: false } + ) + return terminal + }) + + const debug = ( + globalThis as { + __terminalOutputSchedulerDebug?: { snapshot: () => { queuedChars: number } } + } + ).__terminalOutputSchedulerDebug + if (!debug) { + throw new Error('scheduler debug API unavailable') + } + + const baseline = collect() + + // Why stop early: the leak is what a PARTIALLY drained queue pins. Draining to empty + // frees the chunks either way, so a full drain cannot observe the defect. + const TAIL_CHARS = 64 * 1024 + let ticks = 0 + while (debug.snapshot().queuedChars > TAIL_CHARS && ticks < 20000) { + vi.advanceTimersByTime(4) + ticks += 1 + } + + const queuedChars = debug.snapshot().queuedChars + const retainedBytes = collect() - baseline + + // Sanity: nearly everything drained, but a real tail is still queued - otherwise + // there is no residual slice to hold a parent chunk and the measurement is meaningless. + expect(writtenChars).toBeGreaterThan(TERMINALS * CHUNK_CHARS * 0.9) + expect(queuedChars).toBeGreaterThan(0) + expect(queuedChars).toBeLessThanOrEqual(TAIL_CHARS) + + // The defect: those few queued KB pinned 8 x 2 MB of parent chunks (~16 MB). + expect(retainedBytes).toBeLessThan(4 * 1024 * 1024) + expect(terminals).toHaveLength(TERMINALS) + }) + }) }) 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 4272d4c0913..e05d22a1251 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 @@ -49,6 +49,8 @@ type WriteTerminalOutputOptions = { type QueueChunk = { data: string + // Why: `data` may be a V8 SlicedString pinning a much larger parent; this is what the queue really holds alive. + retainedChars: number foreground: boolean forceForegroundRefresh: boolean followupForegroundRefresh: boolean @@ -542,6 +544,11 @@ function coalescedQueuedDataNeedsCursorRestore(entry: QueueEntry): boolean { ) } +// Why: V8 backs `slice` with a SlicedString that points at the whole parent, so a small tail keeps a huge chunk alive. Concatenating forces a ConsString whose flattening allocates a fresh sequential string of just this length. +function flattenRetainedSlice(value: string): string { + return value.length === 0 ? value : `${value} `.slice(0, -1) +} + function takeQueuedChunk(entry: QueueEntry, limit: number): QueuedWrite | null { let remaining = limit let data = '' @@ -591,6 +598,18 @@ function takeQueuedChunk(entry: QueueEntry, limit: number): QueuedWrite | null { data += chunk.data remaining -= chunk.data.length entry.queuedChars -= chunk.data.length + // Why: compaction only splices every 64 chunks, so without this a fully consumed chunk keeps + // its (possibly parent-pinning) string and callbacks alive while charging nothing. Nothing + // reads below chunkIndex, so the drained slot only has to stay type-shaped. + entry.chunks[entry.chunkIndex] = { + data: '', + retainedChars: 0, + foreground: chunk.foreground, + forceForegroundRefresh: false, + followupForegroundRefresh: false, + shouldRefreshForegroundSynchronously: ALWAYS_REFRESH_FOREGROUND_SYNCHRONOUSLY, + stripTransientCursorShows: false + } entry.chunkIndex += 1 if (chunk.onParsed) { parsedCallbacks.push(chunk.onParsed) @@ -602,9 +621,13 @@ function takeQueuedChunk(entry: QueueEntry, limit: number): QueuedWrite | null { } data += chunk.data.slice(0, remaining) + const residual = chunk.data.slice(remaining) + // Why: the residual is a SlicedString still pinning `retainedChars`; copy it flat once it pins 2x its own length so a drained 2MB chunk stops holding 2MB to serve a 1KB tail. Halving keeps total copying linear in the original chunk. + const flatten = residual.length * 2 <= chunk.retainedChars entry.chunks[entry.chunkIndex] = { ...chunk, - data: chunk.data.slice(remaining) + data: flatten ? flattenRetainedSlice(residual) : residual, + retainedChars: flatten ? residual.length : chunk.retainedChars } entry.queuedChars -= remaining remaining = 0 @@ -681,6 +704,7 @@ function enqueueChunk( ): void { entry.chunks.push({ data, + retainedChars: data.length, foreground: options?.foreground === true, forceForegroundRefresh: options?.forceForegroundRefresh === true, followupForegroundRefresh: options?.followupForegroundRefresh === true, @@ -744,6 +768,7 @@ function replaceBacklogWithWarning( entry.chunks = [ { data: warning, + retainedChars: warning.length, foreground: false, forceForegroundRefresh: false, followupForegroundRefresh: false,