From 70ce2f54362df71e777dd93042a56f957a7cb557 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 1 Aug 2026 07:37:30 -0700 Subject: [PATCH] fix(crash): count the terminal buffer that holds the scrollback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The live-terminal census read `buffer.active`, which is the *alternate* buffer whenever an alt-screen app is up — vim, less, htop, an agent TUI. That buffer is viewport-sized and holds no scrollback: measured on @xterm/headless, 24 rows against `normal`'s 5001. So the reader cleared terminals of exactly the retention it exists to find, and did so hardest under the workloads most likely to be leaking. Read `buffer.normal` instead, and report `altScreenPanes` as its own count rather than folding the two facts into one number. Also commit `panes`/`lines`/`cells` together at the end of the read, so a throw partway cannot leave a pane counted with its lines missing — which reads as a pane retaining nothing. The catch stays, with a corrected reason: a disposed xterm does not throw (measured — it still returns `normal.length` 5001), but the pane is reached through a caller that may be tearing it down, and one bad pane must not sink its siblings. Tests: `pane-manager-registry.test.ts` fixtures moved from a bare `{ buffer: { active: { length } } }` to `normal` plus an `active.type` discriminator, and gained alt-screen cases. Three assertions moved because they pinned the wrong buffer; two more moved on shape alone, from the added `altScreenPanes` return key. --- .../pane-manager-registry.test.ts | 63 +++++++++++++++---- .../lib/pane-manager/pane-manager-registry.ts | 7 ++- .../pane-terminal-buffer-census.ts | 36 ++++++++--- 3 files changed, 86 insertions(+), 20 deletions(-) diff --git a/src/renderer/src/lib/pane-manager/pane-manager-registry.test.ts b/src/renderer/src/lib/pane-manager/pane-manager-registry.test.ts index 531d9c8409e..635c868b011 100644 --- a/src/renderer/src/lib/pane-manager/pane-manager-registry.test.ts +++ b/src/renderer/src/lib/pane-manager/pane-manager-registry.test.ts @@ -252,8 +252,14 @@ describe('live terminal buffer census', () => { registeredManagers.push(manager) } - function terminal(cols: number, lines: number): unknown { - return { cols, buffer: { active: { length: lines } } } + function terminal(cols: number, lines: number, altScreen = false): unknown { + return { + cols, + buffer: { + normal: { length: lines }, + active: { type: altScreen ? 'alternate' : 'normal' } + } + } } afterEach(() => { @@ -273,52 +279,87 @@ describe('live terminal buffer census', () => { expect(getLiveTerminalBufferCensus()).toEqual({ panes: 2, lines: 50_024, - cells: 6_002_880 + cells: 6_002_880, + altScreenPanes: 0 + }) + }) + + // Why: `buffer.active` is the alternate buffer while vim/less/an agent TUI is up, + // and that buffer is viewport-sized — measured on @xterm/headless at 24 rows while + // `normal` still held 5001. Reading `active` reports a pane retaining 50k lines as + // retaining a screenful, which clears terminals of a leak they are causing. + it('counts scrollback held behind an alt-screen app, not its viewport', () => { + registerPanes( + { id: 1, terminal: terminal(120, 50_000, true) }, + { id: 2, terminal: terminal(120, 24) } + ) + + expect(getLiveTerminalBufferCensus()).toEqual({ + panes: 2, + lines: 50_024, + cells: 6_002_880, + altScreenPanes: 1 }) }) it('reports into the renderer memory profile', () => { - registerPanes({ id: 1, terminal: terminal(80, 5_000) }) + registerPanes({ id: 1, terminal: terminal(80, 5_000, true) }) expect(collectRendererMemoryProfileCounts()).toMatchObject({ 'liveTerminalBuffers.panes': 1, 'liveTerminalBuffers.lines': 5_000, - 'liveTerminalBuffers.cells': 400_000 + 'liveTerminalBuffers.cells': 400_000, + 'liveTerminalBuffers.altScreenPanes': 1 }) }) - it('keeps counting siblings when a disposed terminal throws on buffer access', () => { + it('keeps counting siblings when a terminal throws on buffer access', () => { registerPanes( { id: 1, terminal: { get buffer(): never { - throw new Error('disposed') + throw new Error('torn down') } } }, { id: 2, terminal: terminal(100, 10) } ) - expect(getLiveTerminalBufferCensus()).toEqual({ panes: 1, lines: 10, cells: 1_000 }) + expect(getLiveTerminalBufferCensus()).toEqual({ + panes: 1, + lines: 10, + cells: 1_000, + altScreenPanes: 0 + }) }) it('prefers a manager census over materializing public pane views', () => { const counted = { resetWebglTextureAtlases: vi.fn<() => void>(), - getPaneBufferCensus: () => ({ panes: 2, lines: 30, cells: 3_000 }), + getPaneBufferCensus: () => ({ panes: 2, lines: 30, cells: 3_000, altScreenPanes: 1 }), getPanes: vi.fn(() => [{ id: 1, terminal: terminal(80, 10) }]) } registerLivePaneManager(counted) registeredManagers.push(counted) - expect(getLiveTerminalBufferCensus()).toEqual({ panes: 2, lines: 30, cells: 3_000 }) + expect(getLiveTerminalBufferCensus()).toEqual({ + panes: 2, + lines: 30, + cells: 3_000, + altScreenPanes: 1 + }) expect(counted.getPanes).not.toHaveBeenCalled() }) it('tolerates a terminal with no buffer rather than emitting NaN', () => { registerPanes({ id: 1, terminal: {} }) - expect(getLiveTerminalBufferCensus()).toEqual({ panes: 1, lines: 0, cells: 0 }) + expect(getLiveTerminalBufferCensus()).toEqual({ + panes: 1, + lines: 0, + cells: 0, + altScreenPanes: 0 + }) }) }) diff --git a/src/renderer/src/lib/pane-manager/pane-manager-registry.ts b/src/renderer/src/lib/pane-manager/pane-manager-registry.ts index 9630c884f39..d89659c5ae9 100644 --- a/src/renderer/src/lib/pane-manager/pane-manager-registry.ts +++ b/src/renderer/src/lib/pane-manager/pane-manager-registry.ts @@ -122,11 +122,15 @@ export function getLivePaneCensus(): { managers: number; panes: number } { * live terminals is therefore consistent with anything from megabytes to gigabytes * — the same ambiguity the scrollback census resolves for stored buffers, for the * larger live copy. Safe to run inside a near-OOM highwater breadcrumb. + * + * `lines`/`cells` count the normal buffer only; `altScreenPanes` says how many panes + * were additionally holding a viewport-sized alternate buffer at collection time. */ export function getLiveTerminalBufferCensus(): TerminalBufferCensus { let panes = 0 let lines = 0 let cells = 0 + let altScreenPanes = 0 for (const manager of liveManagers) { let census: TerminalBufferCensus try { @@ -139,8 +143,9 @@ export function getLiveTerminalBufferCensus(): TerminalBufferCensus { panes += census.panes lines += census.lines cells += census.cells + altScreenPanes += census.altScreenPanes } - return { panes, lines, cells } + return { panes, lines, cells, altScreenPanes } } registerRendererMemoryProfileContributor('liveTerminalBuffers', getLiveTerminalBufferCensus) diff --git a/src/renderer/src/lib/pane-manager/pane-terminal-buffer-census.ts b/src/renderer/src/lib/pane-manager/pane-terminal-buffer-census.ts index 0725309be86..c8a319e4457 100644 --- a/src/renderer/src/lib/pane-manager/pane-terminal-buffer-census.ts +++ b/src/renderer/src/lib/pane-manager/pane-terminal-buffer-census.ts @@ -6,9 +6,17 @@ * do not expose one. Both funnel through here so the two paths cannot drift. */ -type XtermBufferShape = { cols?: number; buffer?: { active?: { length?: number } } } +type XtermBufferShape = { + cols?: number + buffer?: { active?: { type?: string }; normal?: { length?: number } } +} -export type TerminalBufferCensus = { panes: number; lines: number; cells: number } +export type TerminalBufferCensus = { + panes: number + lines: number + cells: number + altScreenPanes: number +} export function sumTerminalBufferSizes( managedPanes: Iterable<{ terminal: unknown }> @@ -16,17 +24,29 @@ export function sumTerminalBufferSizes( let panes = 0 let lines = 0 let cells = 0 + let altScreenPanes = 0 for (const pane of managedPanes) { try { const terminal = pane.terminal as XtermBufferShape | undefined - // `buffer.active.length` and `cols` are both O(1); safe on the crash path. - const length = terminal?.buffer?.active?.length ?? 0 + // Why `normal` and not `active`: `active` is the alternate buffer whenever an + // alt-screen app is up (vim, less, an agent TUI). That buffer is viewport-sized + // and holds no scrollback — measured on @xterm/headless as 24 rows against + // `normal`'s 5001 — so reading it clears terminals of a leak they are causing. + const retained = terminal?.buffer?.normal?.length ?? 0 + // Why counted separately rather than folded in: one number cannot say both how + // much is retained and which pane shape retained it. + const onAltScreen = terminal?.buffer?.active?.type === 'alternate' + // Why commit together: a throw mid-read must not leave a pane counted with its + // lines missing, which reads as a pane retaining nothing. panes += 1 - lines += length - cells += length * (terminal?.cols ?? 0) + lines += retained + cells += retained * (terminal?.cols ?? 0) + altScreenPanes += onAltScreen ? 1 : 0 } catch { - // Why: a disposed terminal throws on buffer access; siblings still count. + // Why kept although a disposed xterm returns values rather than throwing + // (measured): `terminal` is reached through a pane the caller may be tearing + // down, and one bad pane must not sink the census for its siblings. } } - return { panes, lines, cells } + return { panes, lines, cells, altScreenPanes } }