fix(terminal): release retained output slice parents (#13402)

Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
This commit is contained in:
Neil
2026-08-10 18:19:10 -07:00
committed by GitHub
co-authored by Brennan Benson
parent 96c2cebfa6
commit 868bfbe8ce
5 changed files with 250 additions and 3 deletions
+2 -1
View File
@@ -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: [
@@ -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)
})
})
@@ -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)
}
@@ -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)
})
})
})
@@ -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,