Repository navigation
test(terminal): pin checkpoint-only cold restore of a large checkpoint - #10500
Merged
Merged
Conversation
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).
Contributor
📝 WalkthroughWalkthroughAdds a Vitest suite for large checkpoint-only cold restoration. The tests generate colorized scrollback with marker lines, checkpoint the session, verify 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3215f050-dd06-4a10-b3a4-25f0ef3db755
📒 Files selected for processing (1)
src/main/daemon/terminal-history-large-checkpoint-cold-restore.test.ts
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.
…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.
…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.
This was referenced Jul 27, 2026
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
stablyai#10500) * 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. stablyai#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; stablyai#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-stablyai#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 stablyai#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test only — no production code. Pins two things about checkpoint-only cold restore that had no coverage and have now been misdiagnosed twice.
Triage first: this is not a v1.4.156 regression. A live test reported that checkpoint-only cold restore comes back visually blank at every size tried (1.0, 5.5, 11.2, 20.3 MiB), which would have been a release blocker for v1.4.156. It isn't. The baseline in that test was the PR base (
origin/main= v1.4.156); v1.4.155 was never run.I ran a real A/B: a
git worktreechecked out atv1.4.155againstorigin/main, driving the realHistoryManager.checkpoint()writer and the realHistoryReader.detectColdRestore()reader over a genuinely header-onlyoutput.log(9 bytes =LOG_HEADER_BYTES), which is exactly the checkpoint-only route.Byte-identical on both refs, at the reporter's own sizes, with a 3.5 KB control passing on both to prove the harness. #10179's bounded reader, #9990's async/semaphore rewrite and #10479's 200MB cap are output-equivalent to v1.4.155's bare
JSON.parse(readFileSync()).What actually produces a blank
Blankness tracks the
meta.endedAteligibility gate, not size. Same probe, varying only how the daemon went away:endedAtnull)dispose()/closeSession()(endedAtset)Identical on both refs. A cleanly ended session refuses to cold-restore at any size — by design (
history-manager.ts:277: "an unwritten endedAt looks like an unclean shutdown → false cold restore next launch"). That is the size-independent blank that was mistaken for a size cap. Cold restore requires the daemon to die uncleanly; a graceful shutdown runsHistoryManager.dispose()and correctly disqualifies every session.What this PR adds
The triage surfaced a real coverage gap: #10179's 16MiB read cap sat under what the unbounded checkpoint writer can emit, so a large checkpoint threw, got swallowed to
checkpoint=null, and the pane reopened empty. #10479 raised the cap, but nothing pinned the writer→reader round trip the cap had broken —history-reader-memory.test.tscovers the bounded reader at its limit, and its >16MiB case hand-writescheckpoint.json, so it never exercises the real writer.AssertionError: expected null not to be null), passes at the shipped cap.endedAtgate, pinned at a fixed checkpoint size so the size variable is held constant. Two separate investigations have now read this gate as a size regression.Screenshots
No visual change — test-only.
Testing
pnpm lint—oxlint+oxfmtcleanpnpm typecheck—tsc -p config/tsconfig.node.jsoncleanpnpm test— new suite 2 passed (~2.8s); fullsrc/main/daemon/suite 1036 passed / 5 skipped, no interferencepnpm build— not run; no production code changedRuns in ~2.8s (60s timeout). Temp dirs are created with
mkdtempSyncand removed inafterEach; verified zero leaked directories across 5 runs including 2 deliberate failures.Peak RSS of the full
src/main/daemon/suite: 241 MiB without this file, 793 MiB with it. The first cut of the fixture cost 1,244 MiB — see the memory finding below.AI Review Report
Reviewed with an AI agent whose primary mandate was to disprove the tests — because earlier in this same release cycle a retained oversized-checkpoint test was found to be vacuous (a 200MB NUL-byte fixture fails
JSON.parseeither way, so it stayed green with the byte cap disabled). A vacuous regression test is worse than none. Every claim below was executed, not read.Both tests were proven non-vacuous by mutation:
TERMINAL_HISTORY_CHECKPOINT_MAX_BYTES→16 * 1024 * 1024expected null not to be nullendedAtgate deleted (history-reader.ts:70)expected true to be falsewriteSyncforced unavailableIt fails for the right reason. An instrumented probe confirmed the throw at the reverted cap is
NodeFileReadTooLargeError: File too large: 46.1MB exceeds 16MB limit— the byte cap specifically, notJSON.parseand not the other bound in that same read path. The structural-token limit is not in play:assertJsonTextStructureWithinLimitsskips string contents, and the checkpoint is a flat 11-key object, so its token count stays in the dozens no matter how large the payload gets. That bound targets structural amplification, whichhistory-reader-memory.test.tscovers separately.The fixture is realistic, not filler. At the shipped cap the file parses into a real
TerminalCheckpointFilewith all 11 top-level keys (snapshotAnsi, scrollbackAnsi, oscLinks, rehydrateSequences, cwd, cols, rows, modes, scrollbackLines, generation, checkpointedAt), and all 300/300 distinct markers survive the round trip —checkpoint.json46.13 MiB, seed 26,381,501 chars.output.logis exactly 9 bytes =LOG_HEADER_BYTESandrehydrateSequencesis empty, so log replay contributes nothing and the checkpoint-only route is genuine.It measures what the user actually sees.
daemon-pty-adapter.ts:57-62getRecoveredHistorySeedcomposes exactlyrehydrateSequences + snapshotAnsion the non-alt-screen branch, which the test pins withexpect(modes.alternateScreen).toBe(false).Defects found and fixed
1. A silent-vacuity hole in the
endedAttest (also flagged by CodeRabbit, 45e178d).writeLargeScrollbackignoredwriteSync's boolean, which returnsfalseif xterm's private_core.writeSyncever goes away. The large-checkpoint test self-guards (failsexpected 416 to be greater than 16777216), but theendedAtgate is size-independent, so that test silently passed on a 416-byte checkpoint — pinning the gate over no scrollback at all. Fixed with an accumulator plus a single assertion in the helper; both tests now fail loudly.2. The fixture raised the whole daemon suite's peak memory 5.2x (33edb2b). Despite the comment claiming SGR-per-cell, the filler coloured per line, so the serialized seed only tracked plain-text size and needed 26k buffer rows to clear 16MiB. The xterm buffer costs rows × cols, so this one file took
src/main/daemon/from 241 MiB to 1,244 MiB peak RSS — a real CI OOM risk immediately after #10299 bounded shared readers for that same reason. Carrying the bytes in SGR runs instead of rows reaches a larger seed from 5.5k rows:checkpoint.jsonmarginSo it is cheaper and better-pinned, and the fixture now matches its own comment and is more representative of real coloured agent output. All three mutations were re-verified against the new fixture.
3. Review follow-ups (370d10d). Two independent reviewers returned LAND-READY; one reproduced the memory finding above with matching numbers. Their non-blocking points, applied: import
getHistorySessionDirNamerather than hand-rollingencodeURIComponent(equivalent today, but that helper exists to absorb encoding changes, so the copy would silently diverge from the writer it checks); indexFILLER_ROWSby its own length so growing the array cannot leave rows unused; and drop the timeout from300_000to60_000, since 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.One suggested change I applied only on its secondary rationale: taking the snapshot, disposing, then checkpointing is exception-safe, but it is not the memory saving it was proposed as. Peak RSS is set at
getSnapshot(), where the live buffer and the serialized strings coexist, so disposing beforecheckpoint()cannot move it — measured ~800 MiB either way across three runs each. I also declined renaming test 2, whose "at the same checkpoint size" refers to holding size constant across the two branches, which is accurate as written.Cross-platform (macOS/Linux/Windows):
getHistorySessionDirNameisencodeURIComponent(history-paths.ts), so the test's manual encoding matches production and the POSIX-lookingSESSION_IDis encoded to a valid Windows path segment — the reason that helper exists. All paths go throughpath.join; temp dirs come fromos.tmpdir(). No shell invocation, no shortcuts, no labels, no Electron-specific behavior touched.Considered and declined as gold-plating: asserting all 300/300 markers rather than first/last — the regression class is all-or-nothing, and
seed.length > 16MiBalready catches content loss loudly, including a future writer-side trim.Security Audit
Low risk: test-only, no production code, no new dependencies. No command execution, no IPC, no auth, no secrets, no network. Input handling is confined to a synthetic in-process ANSI fixture. Path handling writes only under a
mkdtempSyncdirectory removed inafterEach; thermSync(..., { recursive: true, force: true })targets are derived from that temp root, not from user input. No follow-up needed.Notes
ColdRestoreInfo. I did not drive two full Electron builds through a real UI, so a purely renderer-side blank is not excluded by this evidence. Given the main-side output is byte-identical across the refs, any such blank would not be a v1.4.156 regression in the restore read path.ColdRestorePayloadCache's new 16MiB bound makes an oversized payload self-evict during its ownset()(measured:oversizedSurvivesAlone=false, and it evicts prior entries too), where v1.4.155 used an unboundedMap. Both call sites appear to tolerate it — the spawn path returns the payload directly, and the sleep path guards withhas()and re-reads from disk on wake — so I believe it self-heals, but I have not end-to-end verified the wake path. Worth a follow-up.terminal-history-file-limits.tsnotes that trimming the snapshot writer-side is the real fix for oversized checkpoints; the 200MB cap bounds a corrupt/runaway file, not retention.