From e4057f26f93ab2894d50f8af593eb6ee5f36fe40 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 24 Jul 2026 22:55:16 -0700 Subject: [PATCH] fix(terminal): stop cold restore dropping all scrollback on large checkpoints (#10479) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(terminal): stop cold restore dropping all scrollback on large checkpoints The checkpoint read cap shipped without its write bound. history-reader.ts reads checkpoint.json through a 16MiB cap that throws past the limit, and the catch swallows it to checkpoint=null; history-manager.ts still writes the checkpoint with an unbounded JSON.stringify. Every fallback then collapses (stale-generation log, unlinked legacy scrollback.bin), so the terminal reopens empty with nothing surfaced. Raise the read cap to cover the largest checkpoint the writer can legitimately emit, derived from the scrollback policy's 50k-row max preset so it cannot drift back under the writer. A bound is kept so a corrupt file still cannot OOM the main process. Bounding the writer instead would not recover the scrollback: the stringify throw lands in handleWriteError, which adds the session to disabledSessions and permanently stops history recording for it. * fix(terminal): anchor checkpoint read cap to its own reasoning The cap was derived as 2 * LEGACY_TERMINAL_SCROLLBACK_BYTES_100_MB, but that constant is a legacy byte-preset setting value with no other consumer, and the 50k-row bucket it was attributed to has no upper byte bound. Same value, stated without the false policy linkage. * fix(terminal): assert the checkpoint byte cap, correct its rationale The retained oversized-checkpoint test passed with the byte guard removed entirely — 200MB of NUL fails JSON.parse, so detectColdRestore returned null either way. Assert the bounded reader directly, as the amplification test does. Also: a 50k-row max preset of ordinary text measures ~14MB serialized, not 'far below' by an unbounded margin — per-cell-colored output can still exceed the cap, which is what a writer-side snapshot trim has to fix. (cherry picked from commit 6c03ecf8e1a485dc34e2929df2236881136c6f7e) --- src/main/daemon/history-reader-memory.test.ts | 30 +++++++++++++++---- .../daemon/terminal-history-file-limits.ts | 6 +++- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/main/daemon/history-reader-memory.test.ts b/src/main/daemon/history-reader-memory.test.ts index 36e86f5f413..9a96e4e7cf8 100644 --- a/src/main/daemon/history-reader-memory.test.ts +++ b/src/main/daemon/history-reader-memory.test.ts @@ -22,6 +22,8 @@ import { TERMINAL_HISTORY_JSON_MAX_STRUCTURAL_TOKENS } from './terminal-history-file-reader' +const RETIRED_CHECKPOINT_READ_CAP_BYTES = 16 * 1024 * 1024 + const directories: string[] = [] function createSession(sessionId: string): { basePath: string; sessionPath: string } { @@ -49,7 +51,7 @@ function createSparseFile(path: string, bytes: number): void { closeSync(descriptor) } -function checkpoint(): string { +function checkpoint(overrides?: Record): string { return JSON.stringify({ snapshotAnsi: 'safe checkpoint', scrollbackAnsi: '', @@ -62,7 +64,8 @@ function checkpoint(): string { mouseTracking: false, applicationCursor: false, alternateScreen: false - } + }, + ...overrides }) } @@ -82,12 +85,27 @@ describe('terminal history restore memory limits', () => { expect(restore?.snapshotAnsi).toBe('safe checkpoint') }) + // Why: the read cap once sat at 16MiB while the writer stayed unbounded, so a big-scrollback + // session cold-restored to an empty terminal — checkpoint nulled, and every fallback collapsed. + it('restores a checkpoint larger than the retired 16MiB read cap', async () => { + const { basePath, sessionPath } = createSession('large-checkpoint') + const scrollbackAnsi = 'x'.repeat(RETIRED_CHECKPOINT_READ_CAP_BYTES + 1) + writeFileSync(join(sessionPath, 'checkpoint.json'), checkpoint({ scrollbackAnsi })) + + const restore = await new HistoryReader(basePath).detectColdRestore('large-checkpoint') + expect(restore?.scrollbackAnsi).toBe(scrollbackAnsi) + }) + + // Why the cap survives at all: it bounds a corrupt/runaway file, well above legitimate output. it('ignores oversized checkpoint and metadata files before parsing', async () => { const checkpointSession = createSession('oversized-checkpoint') - createSparseFile( - join(checkpointSession.sessionPath, 'checkpoint.json'), - TERMINAL_HISTORY_CHECKPOINT_MAX_BYTES + 1 - ) + const oversizedCheckpoint = join(checkpointSession.sessionPath, 'checkpoint.json') + createSparseFile(oversizedCheckpoint, TERMINAL_HISTORY_CHECKPOINT_MAX_BYTES + 1) + // Why assert the reader directly too: detectColdRestore returns null for any unparseable + // checkpoint, so it would stay green with the byte cap removed entirely. + expect(() => + readTerminalHistoryJson(oversizedCheckpoint, TERMINAL_HISTORY_CHECKPOINT_MAX_BYTES) + ).toThrow(/File too large/) expect( await new HistoryReader(checkpointSession.basePath).detectColdRestore('oversized-checkpoint') ).toBeNull() diff --git a/src/main/daemon/terminal-history-file-limits.ts b/src/main/daemon/terminal-history-file-limits.ts index 3a8e4a1891e..5fc4523d785 100644 --- a/src/main/daemon/terminal-history-file-limits.ts +++ b/src/main/daemon/terminal-history-file-limits.ts @@ -1,4 +1,8 @@ export const TERMINAL_HISTORY_META_MAX_BYTES = 64 * 1024 export const TERMINAL_HISTORY_LOG_MAX_BYTES = 5 * 1024 * 1024 -export const TERMINAL_HISTORY_CHECKPOINT_MAX_BYTES = 16 * 1024 * 1024 +// Why deliberately generous: the checkpoint writer is unbounded, and a read cap under what it +// can emit silently drops ALL scrollback on cold restore. This guards a corrupt/runaway file, +// not retention — a 50k-row max preset of ordinary text serializes to ~14MB, ~15x under. Output +// colored per cell can still exceed this; trimming the snapshot writer-side is the real fix. +export const TERMINAL_HISTORY_CHECKPOINT_MAX_BYTES = 200_000_000 export const TERMINAL_HISTORY_LEGACY_SCROLLBACK_MAX_BYTES = 16 * 1024 * 1024