From c2bbbaaab6ff587a818fdfa5f2d34f35ea22c013 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 25 Jul 2026 00:41:18 -0700 Subject: [PATCH] perf(terminal): drop the JSON structural pre-scan from the history read path (#10499) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(terminal): drop the JSON structural pre-scan from the history read path readTerminalHistoryJson/Async walked every character of checkpoint.json in interpreted JS before handing the same string to native JSON.parse. On a 26.8MB checkpoint that scan measured 63-330ms — 2.7-12x the JSON.parse it guards, and 57% of the whole read path — all of it synchronous main-thread work. readTerminalHistoryJsonAsync exists so cold-restore reads do not block the main thread; running the scan inline right after the async read defeated its own stated purpose. The scan also had a correctness cost: TERMINAL_HISTORY_JSON_MAX_STRUCTURAL_TOKENS was 1_000_000, and oscLinks is unbounded at ~10 structural tokens per link, so roughly 100k OSC-8 hyperlinks tripped the assert, history-reader swallowed the throw to a null checkpoint, and the terminal restored blank — the same user-visible loss #10479 just fixed for the byte cap. checkpoint.json and meta.json are our own SerializeAddon output, not untrusted input: the byte cap still bounds the read, and a corrupt file fails JSON.parse into the same catch. The shared helper stays for the call sites that do handle untrusted JSON. * test(terminal): pin iterative JSON.parse for the dropped nesting-depth cap The retired pre-scan enforced two limits; the new tests only covered the structural-token half. Dropping the 128-level nesting cap is safe solely because V8 parses JSON iteratively — depth costs heap, not stack — so a deeply nested checkpoint parses instead of overflowing. Verified: 10M-deep arrays and objects parse without throwing on V8 14.6. That property is an engine guarantee this code now silently depends on, and a recursive parser would abort the daemon outright rather than throw into the callers' catch. The test fails on main with "JSON nesting exceeds 128 levels" and costs 4ms. * docs(terminal): drop the rot-prone benchmark figure from the reader comment Keeps the durable rationale for skipping the pre-scan (self-authored input, byte cap still bounds the read, corrupt files land in the callers' existing catch) and moves the "~57% of the read path" measurement to the PR body, where it cannot go stale against a later change to this path. Addresses the CodeRabbit comment-length nitpick against the repo's one-line-if-possible comment guideline. (cherry picked from commit d9aa919e3c1e02300c5d5c9028d1dccfa06e27fd) --- src/main/daemon/history-reader-memory.test.ts | 64 ++++++++++++++----- .../daemon/terminal-history-file-reader.ts | 24 ++----- 2 files changed, 54 insertions(+), 34 deletions(-) diff --git a/src/main/daemon/history-reader-memory.test.ts b/src/main/daemon/history-reader-memory.test.ts index 9a96e4e7cf8..cecb29b1bb8 100644 --- a/src/main/daemon/history-reader-memory.test.ts +++ b/src/main/daemon/history-reader-memory.test.ts @@ -9,7 +9,7 @@ import { } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { afterEach, describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it } from 'vitest' import { HistoryReader } from './history-reader' import { getHistorySessionDirName } from './history-paths' import { @@ -17,10 +17,7 @@ import { TERMINAL_HISTORY_LOG_MAX_BYTES, TERMINAL_HISTORY_META_MAX_BYTES } from './terminal-history-file-limits' -import { - readTerminalHistoryJson, - TERMINAL_HISTORY_JSON_MAX_STRUCTURAL_TOKENS -} from './terminal-history-file-reader' +import { readTerminalHistoryJson } from './terminal-history-file-reader' const RETIRED_CHECKPOINT_READ_CAP_BYTES = 16 * 1024 * 1024 @@ -120,21 +117,54 @@ describe('terminal history restore memory limits', () => { ).toBe(false) }) - it('rejects structurally amplified checkpoints before parsing', () => { - const { sessionPath } = createSession('amplified-checkpoint') + // Why: the retired 1M-structural-token pre-scan was reachable by ordinary content — + // ~100k OSC-8 hyperlinks cross it, the assert threw, detectColdRestore swallowed it, + // and the terminal restored blank. Same user-visible loss as the retired byte cap. + it('restores a checkpoint carrying more OSC links than the retired structural-token cap', async () => { + const { basePath, sessionPath } = createSession('many-osc-links') + const oscLinks = Array.from({ length: 150_000 }, (_, index) => ({ + row: index, + startCol: 0, + endCol: 40, + uri: `https://example.com/build/${index}` + })) + writeFileSync(join(sessionPath, 'checkpoint.json'), checkpoint({ oscLinks })) + + const restore = await new HistoryReader(basePath).detectColdRestore('many-osc-links') + expect(restore?.snapshotAnsi).toBe('safe checkpoint') + expect(restore?.oscLinks).toHaveLength(oscLinks.length) + }) + + // Why assert the reader directly too: detectColdRestore hides any reader throw as a + // null checkpoint, so a reinstated pre-scan would stay invisible above. + it('parses link-dense checkpoints without a structural pre-scan budget', () => { + const { sessionPath } = createSession('link-dense-checkpoint') const checkpointPath = join(sessionPath, 'checkpoint.json') + writeFileSync(checkpointPath, `{"snapshotAnsi":"","values":[${'0,'.repeat(2_000_000)}0]}`) + expect( + readTerminalHistoryJson<{ values: number[] }>( + checkpointPath, + TERMINAL_HISTORY_CHECKPOINT_MAX_BYTES + ).values + ).toHaveLength(2_000_001) + }) + + // Why: the pre-scan also carried a 128-level nesting cap, and dropping it is only safe + // because V8 parses JSON iteratively — depth costs heap, not stack. A future engine that + // recursed would abort the daemon rather than throw into the callers' catch, so pin it. + it('parses deeply nested checkpoints without the retired nesting-depth cap', () => { + const { sessionPath } = createSession('deeply-nested-checkpoint') + const checkpointPath = join(sessionPath, 'checkpoint.json') + const depth = 50_000 writeFileSync( checkpointPath, - `{"snapshotAnsi":"","values":[${'0,'.repeat(TERMINAL_HISTORY_JSON_MAX_STRUCTURAL_TOKENS)}0]}` + `{"snapshotAnsi":"","nested":${'['.repeat(depth)}${']'.repeat(depth)}}` ) - const parseSpy = vi.spyOn(JSON, 'parse') - try { - expect(() => - readTerminalHistoryJson(checkpointPath, TERMINAL_HISTORY_CHECKPOINT_MAX_BYTES) - ).toThrow(/JSON structure exceeds/) - expect(parseSpy).not.toHaveBeenCalled() - } finally { - parseSpy.mockRestore() - } + expect( + readTerminalHistoryJson<{ nested: unknown[] }>( + checkpointPath, + TERMINAL_HISTORY_CHECKPOINT_MAX_BYTES + ).nested + ).toHaveLength(1) }) }) diff --git a/src/main/daemon/terminal-history-file-reader.ts b/src/main/daemon/terminal-history-file-reader.ts index 3d273e95b9e..237eca57bdd 100644 --- a/src/main/daemon/terminal-history-file-reader.ts +++ b/src/main/daemon/terminal-history-file-reader.ts @@ -2,10 +2,6 @@ import { readNodeFileSyncWithinLimit, readNodeFileWithinLimit } from '../../shared/node-bounded-file-reader' -import { assertJsonTextStructureWithinLimits } from '../../shared/json-text-structure-limit' - -export const TERMINAL_HISTORY_JSON_MAX_STRUCTURAL_TOKENS = 1_000_000 -export const TERMINAL_HISTORY_JSON_MAX_NESTING_DEPTH = 128 export function readTerminalHistoryBuffer(filePath: string, maxBytes: number): Buffer { return readNodeFileSyncWithinLimit(filePath, maxBytes).buffer @@ -15,17 +11,16 @@ export function readTerminalHistoryText(filePath: string, maxBytes: number): str return readTerminalHistoryBuffer(filePath, maxBytes).toString('utf8') } +// Why no JSON structure pre-scan here: checkpoint.json and meta.json are our own +// writer's output, not untrusted input — the byte cap still bounds the read and a +// corrupt file fails JSON.parse into every caller's existing catch. Untrusted JSON +// still goes through assertJsonTextStructureWithinLimits. export function readTerminalHistoryJson(filePath: string, maxBytes: number): T { - const text = readTerminalHistoryText(filePath, maxBytes) - assertJsonTextStructureWithinLimits(text, { - structuralTokens: TERMINAL_HISTORY_JSON_MAX_STRUCTURAL_TOKENS, - nestingDepth: TERMINAL_HISTORY_JSON_MAX_NESTING_DEPTH - }) - return JSON.parse(text) as T + return JSON.parse(readTerminalHistoryText(filePath, maxBytes)) as T } // Why: cold-restore payload reads must not block the main thread, but need the -// same byte and JSON-structure bounds as the sync readers. +// same byte bound as the sync readers. export async function readTerminalHistoryBufferAsync( filePath: string, maxBytes: number @@ -44,10 +39,5 @@ export async function readTerminalHistoryJsonAsync( filePath: string, maxBytes: number ): Promise { - const text = await readTerminalHistoryTextAsync(filePath, maxBytes) - assertJsonTextStructureWithinLimits(text, { - structuralTokens: TERMINAL_HISTORY_JSON_MAX_STRUCTURAL_TOKENS, - nestingDepth: TERMINAL_HISTORY_JSON_MAX_NESTING_DEPTH - }) - return JSON.parse(text) as T + return JSON.parse(await readTerminalHistoryTextAsync(filePath, maxBytes)) as T }