From cfb41515246d63589c8d7e99c866c54991eff4b2 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 25 Jul 2026 00:04:34 -0700 Subject: [PATCH] test(terminal): pin checkpoint-only cold restore of a large checkpoint (#10500) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(terminal): pin checkpoint-only cold restore of a large checkpoint Triaging a report that checkpoint-only cold restore renders blank at every checkpoint size found no v1.4.156 regression: an A/B of v1.4.155 against origin/main returned byte-identical ColdRestoreInfo at 1.01, 5.56, 11.30 and 20.61 MiB, all 300 marker lines intact on both refs. Blankness tracks the meta.endedAt eligibility gate, not size — a cleanly ended session refuses to cold-restore at any size, which is by design and unchanged between the refs. What the triage did surface is a coverage gap. #10179's 16MiB checkpoint read cap sat under what the unbounded writer emits, so a large checkpoint threw, was swallowed to checkpoint=null, and the pane reopened empty; #10479 raised the cap but nothing pinned the round trip it had broken. history-reader-memory covers the bounded reader at its limit, not writer→reader recovery. Adds that round trip through the real HistoryManager writer and HistoryReader over a header-only log, and pins the endedAt gate that has now been mistaken for a size regression twice. Fails at the pre-#10479 cap with the exact production symptom (detectColdRestore returns null). * test(terminal): fail loudly when writeSync is unavailable writeLargeScrollback ignored writeSync's boolean, which is false when xterm's private _core.writeSync goes away. The size assertions catch that in the large-checkpoint test (416 bytes vs 16MiB), but the endedAt gate is size-independent, so that test silently passed on an empty snapshot — pinning the gate over no scrollback at all. * test(terminal): cut large-checkpoint fixture peak RSS from 1.2GiB to 800MiB The filler colored per line, not per cell as its comment claimed, so the serialized seed only tracked plain-text size and needed 26k buffer rows to clear 16MiB. The xterm buffer costs rows x cols, which made this single file raise the whole src/main/daemon/ suite's peak RSS 5.2x (241MiB -> 1244MiB) — a real OOM risk on CI right after #10299 bounded readers for that reason. Carrying the bytes in SGR runs instead of rows reaches a larger seed from 5.5k rows: suite peak RSS 1244MiB -> 793MiB, and the margins improve too (seed 25.16MiB = 1.57x the threshold vs 1.19x before, checkpoint.json 46.1MiB = 2.88x vs 1.20x). Also makes the fixture match its own comment and be more representative of real colored agent output. Re-verified all three mutations still fail: cap at 16MiB -> "expected null not to be null"; endedAt gate deleted -> test 2 fails; writeSync unavailable -> both fail. * test(terminal): tighten timeout, reuse prod dir-name helper, dispose first Review follow-ups, all test-only: - Import getHistorySessionDirName instead of hand-rolling encodeURIComponent. Equivalent today, but that helper exists to absorb encoding changes, so the hand-rolled copy would silently diverge from the writer it is checking. - Index FILLER_ROWS by its own length so growing the array cannot leave rows unused. - 300_000ms -> 60_000ms. The file runs in ~2.8s and vitest's own default is 30s; a 5-minute ceiling turns a hung regression into a stalled job rather than a failure. 60s keeps Windows headroom. - Take the snapshot, dispose, then checkpoint, so a throwing checkpoint() cannot leak the buffer. Note this is memory-neutral, not a saving: peak RSS is set at getSnapshot(), where the buffer and the serialized strings coexist, and measured ~800MiB either way. Mutations re-verified: cap at 16MiB -> "expected null not to be null"; endedAt gate deleted -> test 2 fails; writeSync unavailable -> both fail. (cherry picked from commit a0971e0f9baf8d12f343854712b7bb84167f4470) --- ...tory-large-checkpoint-cold-restore.test.ts | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 src/main/daemon/terminal-history-large-checkpoint-cold-restore.test.ts diff --git a/src/main/daemon/terminal-history-large-checkpoint-cold-restore.test.ts b/src/main/daemon/terminal-history-large-checkpoint-cold-restore.test.ts new file mode 100644 index 00000000000..a4b4fbe4ba3 --- /dev/null +++ b/src/main/daemon/terminal-history-large-checkpoint-cold-restore.test.ts @@ -0,0 +1,120 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { mkdtempSync, rmSync, statSync } from 'node:fs' +import { HistoryManager } from './history-manager' +import { HistoryReader } from './history-reader' +import { HeadlessEmulator } from './headless-emulator' +import { LOG_HEADER_BYTES } from './terminal-history-log' +import { getHistorySessionDirName } from './history-paths' + +// Guards checkpoint-only cold restore: the route taken when the daemon dies right after a +// checkpoint, so output.log is header-only and checkpoint.json is the sole recovery source. +// #10179 shipped a read cap (16MiB) under what the unbounded checkpoint writer can emit; past it +// the read threw, the catch swallowed it to checkpoint=null, and the pane reopened empty (#10479 +// raised the cap; nothing pinned the round trip that the cap silently broke). The +// reader-level bound is covered in history-reader-memory.test.ts — this asserts the round trip +// through the real writer, which is what actually decides whether a user sees their scrollback. + +const MARKERS = 300 +const SESSION_ID = 'repo-1::/Users/dev/large-scrollback' +const COLS = 800 +const ROWS = 40 + +// Why per-cell color: the restore seed must clear 16MiB to cover the pre-#10479 cap, and +// SGR-per-cell is how real agent/build output inflates a snapshot well past its plain-text size. +// It is also what keeps the fixture affordable — the xterm buffer costs rows x cols, so carrying +// the bytes in SGR runs instead of rows reaches 25MiB of seed from 5.5k rows rather than 26k, +// cutting this file's peak RSS from ~1.2GiB to ~800MiB. Only 8 distinct rows exist under +// (row + col) % 8, so build them once rather than per line. +const FILLER_ROWS = Array.from( + { length: 8 }, + (_unused, row) => + `${Array.from({ length: COLS - 1 }, (_cell, col) => `\x1b[3${(row + col) % 8}mx`).join('')}\x1b[0m\r\n` +) + +function writeLargeScrollback(emulator: HeadlessEmulator, fillerLines: number): void { + let written = true + for (let index = 0; index < fillerLines; index += 1) { + written = emulator.writeSync(FILLER_ROWS[index % FILLER_ROWS.length]) && written + } + for (let index = 0; index < MARKERS; index += 1) { + written = emulator.writeSync(`MARKER-${index}\r\n`) && written + } + // Why assert: writeSync returns false if xterm's private _core.writeSync ever goes away, which + // would leave an empty snapshot. The size assertions below catch that, but the endedAt gate is + // size-independent and would still pass — pinning the gate over no scrollback at all. + expect(written).toBe(true) +} + +describe('checkpoint-only cold restore of a large checkpoint', () => { + let dir: string + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'large-checkpoint-restore-')) + }) + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }) + }) + + it('recovers the full scrollback from a checkpoint larger than the pre-fix 16MiB read cap', async () => { + const manager = new HistoryManager(dir) + const reader = new HistoryReader(dir) + const emulator = new HeadlessEmulator({ cols: COLS, rows: ROWS, scrollback: 100_000 }) + writeLargeScrollback(emulator, 5_500) + + await manager.openSession(SESSION_ID, { + cwd: '/Users/dev/large-scrollback', + cols: COLS, + rows: ROWS + }) + // Why dispose first: a throwing checkpoint() must not leak the buffer for the worker's life. + const snapshot = emulator.getSnapshot() + emulator.dispose() + await manager.checkpoint(SESSION_ID, snapshot) + + const sessionDir = join(dir, getHistorySessionDirName(SESSION_ID)) + // checkpoint() resets the log to its header, so this is genuinely checkpoint-only: any + // recovered content had to come through the checkpoint read, not incremental log replay. + expect(statSync(join(sessionDir, 'output.log')).size).toBe(LOG_HEADER_BYTES) + expect(statSync(join(sessionDir, 'checkpoint.json')).size).toBeGreaterThan(16 * 1024 * 1024) + + const info = await reader.detectColdRestore(SESSION_ID) + expect(info).not.toBeNull() + // The adapter seeds a non-alt-screen restore with rehydrateSequences + snapshotAnsi. + const seed = info!.rehydrateSequences + info!.snapshotAnsi + expect(info!.modes.alternateScreen).toBe(false) + for (const index of [0, MARKERS - 1]) { + expect(seed).toContain(`MARKER-${index}`) + } + expect(seed.length).toBeGreaterThan(16 * 1024 * 1024) + }, 60_000) + + // Why pinned: this gate, not any size cap, is what makes a checkpoint-only restore come back + // blank. Two separate investigations mistook a cleanly-ended session for a size regression. + it('restores after an unclean exit but not after a clean one, at the same checkpoint size', async () => { + const restoreAfter = async (endCleanly: boolean): Promise => { + const manager = new HistoryManager(dir) + const emulator = new HeadlessEmulator({ cols: COLS, rows: ROWS, scrollback: 100_000 }) + writeLargeScrollback(emulator, 50) + await manager.openSession(SESSION_ID, { + cwd: '/Users/dev/large-scrollback', + cols: COLS, + rows: ROWS + }) + const snapshot = emulator.getSnapshot() + emulator.dispose() + await manager.checkpoint(SESSION_ID, snapshot) + if (endCleanly) { + await manager.closeSession(SESSION_ID, 0) + } + const info = await new HistoryReader(dir).detectColdRestore(SESSION_ID) + rmSync(join(dir, getHistorySessionDirName(SESSION_ID)), { recursive: true, force: true }) + return info !== null + } + + expect(await restoreAfter(false)).toBe(true) + expect(await restoreAfter(true)).toBe(false) + }, 60_000) +})