mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 00:03:45 +00:00
perf(terminal): drop the JSON structural pre-scan from the history read path (#10499)
* 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 d9aa919e3c)
This commit is contained in:
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<T>(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<T>(
|
||||
filePath: string,
|
||||
maxBytes: number
|
||||
): Promise<T> {
|
||||
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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user