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/flatten-retained-slice.test.ts b/src/renderer/src/lib/flatten-retained-slice.test.ts new file mode 100644 index 00000000000..2eedfc5c6ad --- /dev/null +++ b/src/renderer/src/lib/flatten-retained-slice.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from 'vitest' + +import { flattenRetainedSlice } from './flatten-retained-slice' + +// Why 1 MB: comfortably above V8's SlicedString threshold, so a raw slice really does keep the +// parent alive and the difference between flattened and not is unmistakable in heapUsed. +const PARENT_CHARS = 1024 * 1024 +const TAIL_CHARS = 512 +const PARENTS = 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 +} + +// Distinct leading chars stop V8 sharing one backing store across the parents. +function makeParent(index: number, filler: string): string { + return String.fromCharCode(0x41 + index) + filler.repeat(PARENT_CHARS - 1) +} + +function retainedBytesForTails(filler: string, transform: (value: string) => string): number { + const parents = Array.from({ length: PARENTS }, (_unused, index) => makeParent(index, filler)) + const baseline = collect() + const tails = parents.map((parent) => transform(parent.slice(parent.length - TAIL_CHARS))) + // Drop the parents; only the tails stay reachable. + parents.length = 0 + const retained = collect() - baseline + expect(tails).toHaveLength(PARENTS) + expect(tails.every((tail) => tail.length === TAIL_CHARS)).toBe(true) + return retained +} + +describe('flattenRetainedSlice', () => { + it('preserves content exactly', () => { + expect(flattenRetainedSlice('')).toBe('') + expect(flattenRetainedSlice('a')).toBe('a') + const source = 'hello world, 漢字, , \u{1f600}' + expect(flattenRetainedSlice(source.slice(3))).toBe(source.slice(3)) + }) + + it.each([ + ['one-byte', 'a'], + ['two-byte', '漢'] + ])('drops the %s parent a raw slice would pin', (_label, filler) => { + const raw = retainedBytesForTails(filler, (value) => value) + const flattened = retainedBytesForTails(filler, flattenRetainedSlice) + + // A raw slice pins all 8 parents; flattening must keep only the tails. + expect(raw).toBeGreaterThan(PARENTS * PARENT_CHARS * 0.5) + expect(flattened).toBeLessThan(PARENTS * PARENT_CHARS * 0.05) + }) +}) diff --git a/src/renderer/src/lib/flatten-retained-slice.ts b/src/renderer/src/lib/flatten-retained-slice.ts new file mode 100644 index 00000000000..45585c3a75e --- /dev/null +++ b/src/renderer/src/lib/flatten-retained-slice.ts @@ -0,0 +1,4 @@ +// V8 slices can retain their full parent; force a standalone copy for values that outlive it. +export function flattenRetainedSlice(value: string): string { + return value.length === 0 ? value : `${value} `.slice(0, -1) +} 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 c3f0c4df330..2a9d7d023ac 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 @@ -1721,4 +1721,168 @@ 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('releases chunks it has already drained past', async () => { + vi.useFakeTimers() + const { writeTerminalOutput } = await loadScheduler() + + // Why many medium chunks: compactConsumedChunks only splices once chunkIndex reaches 64, + // so below that the drained slots stay in the array and must be cleared individually. + const CHUNKS_PER_TERMINAL = 40 + const CHUNK = 48 * 1024 + const SINKS = 4 + + let writtenChars = 0 + const terminals = Array.from({ length: SINKS }, () => ({ + write: (data: string, callback?: () => void) => { + writtenChars += data.length + callback?.() + } + })) + + const baseline = collect() + + for (const [index, terminal] of terminals.entries()) { + for (let chunk = 0; chunk < CHUNKS_PER_TERMINAL; chunk += 1) { + writeTerminalOutput( + terminal, + String.fromCharCode(65 + index) + + String.fromCharCode(48 + (chunk % 10)) + + 'q'.repeat(CHUNK - 2), + { foreground: false, latencySensitive: false } + ) + } + } + + const debug = ( + globalThis as { + __terminalOutputSchedulerDebug?: { snapshot: () => { queuedChars: number } } + } + ).__terminalOutputSchedulerDebug + if (!debug) { + throw new Error('scheduler debug API unavailable') + } + + const TAIL_CHARS = 64 * 1024 + let ticks = 0 + while (debug.snapshot().queuedChars > TAIL_CHARS && ticks < 40000) { + vi.advanceTimersByTime(4) + ticks += 1 + } + + const queuedChars = debug.snapshot().queuedChars + const retainedBytes = collect() - baseline + + // Sanity: the queues really drained down, and none hit the backlog cap. + expect(writtenChars).toBeGreaterThan(SINKS * CHUNKS_PER_TERMINAL * CHUNK * 0.9) + expect(queuedChars).toBeGreaterThan(0) + expect(queuedChars).toBeLessThanOrEqual(TAIL_CHARS) + + // The defect: ~39 drained-past slots per terminal kept their strings alive uncharged. + expect(retainedBytes).toBeLessThan(2 * 1024 * 1024) + }) + + it('does not pin the parent when a producer enqueues a slice', async () => { + vi.useFakeTimers() + const { writeTerminalOutput } = await loadScheduler() + + // Why a slice: agent-status-osc.ts and the restore-overlap trims hand the scheduler + // strings cut from a much larger buffer. The queue must own its copy rather than + // trusting every producer to flatten first (STA-3567 review round 2). + const KEEP_CHARS = 64 * 1024 + const sinks = Array.from({ length: TERMINALS }, () => ({ + write: (_data: string, callback?: () => void) => callback?.() + })) + + const baseline = collect() + + for (const [index, sink] of sinks.entries()) { + const parent = String.fromCharCode(65 + index) + 'q'.repeat(CHUNK_CHARS - 1) + writeTerminalOutput(sink, parent.slice(parent.length - KEEP_CHARS), { + foreground: false, + latencySensitive: false + }) + } + + const retainedBytes = collect() - baseline + + // 8 x 64 KB of real payload must not keep 8 x 2 MB of parents alive. + expect(retainedBytes).toBeLessThan(4 * 1024 * 1024) + }) + + 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?.() + } + }) + const terminals = Array.from({ length: TERMINALS }, makeSink) + + // Why baseline BEFORE enqueueing: the queue owns a copy of every chunk, so a baseline + // taken after enqueue already contains the parents this test must prove get released, + // and the assertion would hold even with residual flattening disabled. + const baseline = collect() + + for (const [index, terminal] of terminals.entries()) { + // Built and dropped inline so only the queue's own copy stays reachable. + writeTerminalOutput( + terminal, + String.fromCharCode(65 + index) + 'q'.repeat(CHUNK_CHARS - 1), + { foreground: false, latencySensitive: false } + ) + } + + const debug = ( + globalThis as { + __terminalOutputSchedulerDebug?: { snapshot: () => { queuedChars: number } } + } + ).__terminalOutputSchedulerDebug + if (!debug) { + throw new Error('scheduler debug API unavailable') + } + + // 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 chunks (~16 MB). + expect(retainedBytes).toBeLessThan(4 * 1024 * 1024) + }) + }) }) 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 8acb768e635..db826781812 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 @@ -7,6 +7,7 @@ import { } from './pane-terminal-foreground-render-settle' import { runGuardedWriteCompletionStep } from './xterm-write-callback-guard' import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder' +import { flattenRetainedSlice } from '@/lib/flatten-retained-slice' import { discardInFlightTerminalOutputAckCredits, registerTerminalOutputAckCredits @@ -49,6 +50,8 @@ type WriteTerminalOutputOptions = { type QueueChunk = { data: string + // Tracks the backing data still reachable through this queue slot. + retainedChars: number foreground: boolean forceForegroundRefresh: boolean followupForegroundRefresh: boolean @@ -631,6 +634,16 @@ function takeQueuedChunk(entry: QueueEntry, limit: number): QueuedWrite | null { data += chunk.data remaining -= chunk.data.length entry.queuedChars -= chunk.data.length + // Clear drained slots before the 64-chunk compaction can release them. + 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) @@ -642,9 +655,13 @@ function takeQueuedChunk(entry: QueueEntry, limit: number): QueuedWrite | null { } data += chunk.data.slice(0, remaining) + const residual = chunk.data.slice(remaining) + // Geometric flattening bounds retained parents while keeping total copy work linear. + 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 @@ -719,8 +736,11 @@ function enqueueChunk( ackCredit?: () => void } ): void { + // Own queued data so producer slices cannot pin larger PTY or restore buffers. + const owned = flattenRetainedSlice(data) entry.chunks.push({ - data, + data: owned, + retainedChars: owned.length, foreground: options?.foreground === true, forceForegroundRefresh: options?.forceForegroundRefresh === true, followupForegroundRefresh: options?.followupForegroundRefresh === true, @@ -783,6 +803,7 @@ function replaceBacklogWithWarning( entry.chunks = [ { data: warning, + retainedChars: warning.length, foreground: false, forceForegroundRefresh: false, followupForegroundRefresh: false,