Bound retained checkpoints, and stop reading every one to open a window - #190
Merged
VeryComplexAndLongName merged 1 commit intoSep 3, 2026
Merged
Conversation
The extension had been taking many seconds to activate. On this repository .openspec-ui/checkpoints held 531.6 MB in 29 files, 12-24 MB each, against a 42 KB journal - and activate() awaits journal.load(), which read and parsed every one of those files, sequentially, before activation could finish. checkpoint-storage-split moved the bytes out of the journal and did not remove the read. Its goal was that the journal stays small, and it does; load() then re-read every payload it had just stopped embedding. The journal is 42 KB and the load was half a gigabyte. Nothing bounded the growth either. write() retained a checkpoint for any process inside maxProcesses, default 100, and pruneCheckpointFiles only deletes files nothing references - so with every session referenced it never had anything to delete. At roughly 20 MB per apply stage that is two gigabytes read at every window open. load() now returns references with a loadCheckpoint() reader, and checkpoint retention gets its own limit, maxCheckpointSessions, default ten. Ten is measured rather than picked: at 12-24 MB each that is about 150-200 MB and half a day of this repository's activity. Retention is by recency and never by process state. Dropping terminal runs was the first instinct and it is wrong - canRollback is ["completed", "failed", "interrupted"], so a finished run is precisely one the recovery view still offers to roll back. That mistake was made on the real directory before it was caught: pruning by state freed 502.8 MB and with it the rollback of 24 past runs, harmless because all were merged, instructive because the reasoning behind it was contradicted by the code. Half of the laziness is deliberately not done. WorkbenchRecoveryService's details() is synchronous and answers delta, coverage and canRollback out of the checkpoint, so deferring the read there means making it async and taking that through the transport protocol and both surfaces. Both consumers therefore resolve eagerly through loadCheckpoint(), bounded now by retention rather than unbounded. The remaining half - persist the small parts in the reference, read the large after snapshot only on rollback - is task 2.4, and is named in a comment at both call sites rather than left to be rediscovered. One regression I introduced and the tests caught: sorting the persisted session list newest-first broke rollbackChange, which reads sessions back in stored order to find the earliest state to restore to, so a reordered list turned a rollback into a conflict. Selection and ordering are now separate - chosen by recency, written in the caller's order. Also excludes .openspec-ui from the editor's file watcher, which had no watcherExclude entry at all, and from search. That is a second, smaller cost that happens to be free to remove; it is not what made activation slow. Typecheck clean, extension green, core 515/516 - the one failure is git.push.test.ts, already tracked as core-test-worker-contention and untouched by this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
VeryComplexAndLongName
deleted the
fix/checkpoint-retention-and-lazy-load
branch
September 3, 2026 11:15
VeryComplexAndLongName
added a commit
that referenced
this pull request
Sep 4, 2026
`checkpoint-retention-and-lazy-load` landed its implementation in #190 — lazy checkpoint reads and a `maxCheckpointSessions` bound separate from `maxProcesses` — and its test tasks 4.1, 4.2, 4.3 and 4.6 stayed open. The behaviour has been in `main` since, asserted by nothing. Each test asserts the property the task names, not merely that the code runs: - 4.1, no payload reads on load: two sessions are saved, both checkpoint files are then overwritten on disk with unparseable content, and `load()` still succeeds and lists both references. Only the later `loadCheckpoint()` touches the corrupt content, resolving to `undefined` rather than throwing. A test that merely counted reads would pass against an implementation that read them and discarded the result. - 4.2, the bound evicts: three sessions under `maxCheckpointSessions: 2`, asserting the oldest file is gone from disk while the two newest still resolve — and that all three *processes* are retained, which is what proves the two limits are independent rather than one limit applied twice. - 4.3, retention by recency and not by state: a `completed` process newer than a `failed` one, both within the bound, both keeping their checkpoint. Evicting by state would silently withdraw a rollback the product offers, since `canRollback` covers completed and failed runs. - 4.6, rollback after a lazy restore: a `completed` process restored through the same `loadCheckpoint()` indirection `restore()` uses in production, asserting the delta and rollback outcome match the eager path. Task 4.5 stays unchecked, deliberately. `restore()` still reads every referenced checkpoint, so a test asserting "no reads except an interrupted session with no delta" would fail against the real implementation, and a test that dropped the read-count claim would record the task as done while proving something weaker than it asks. It is outstanding until 2.3's remaining half lands. Tests only — no behaviour changes, so no changeset. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
The extension had been taking many seconds to activate.
What was happening
.openspec-ui/checkpoints.openspec-ui/workbench-runs.jsonactivate()awaitsjournal.load(), andload()read andJSON.parsed every one of those files, sequentially, before activation could finish.checkpoint-storage-splitmoved the bytes out of the journal without removing the read. Its goal was that the journal stays small, and it does —load()then re-read every payload it had just stopped embedding. The journal is 42 KB and the load was half a gigabyte.Nothing bounded the growth either.
write()retained a checkpoint for any process insidemaxProcesses(default 100), andpruneCheckpointFilesonly deletes files nothing references — so with every session referenced it never had anything to delete. At ~20 MB perapplystage that is two gigabytes read at every window open.What changed
load()returns references plus aloadCheckpoint()reader.maxCheckpointSessions, default 10, separate frommaxProcesses. Measured, not picked: at 12–24 MB each that is ~150–200 MB and about half a day of this repository's activity..openspec-uiexcluded from the editor's file watcher — which had nowatcherExcludeentry at all — and from search.Retention is by recency, never by state
Dropping terminal runs was the first instinct and it is wrong:
A finished run is precisely one the recovery view still offers to roll back. That mistake was made on the real directory before it was caught — pruning by state freed 502.8 MB and with it the rollback of 24 past runs. Harmless, because all of them were merged; instructive, because the reasoning behind it was contradicted by the code.
Half the laziness is deliberately not done
WorkbenchRecoveryService.details()is synchronous and answersdelta,coverageandcanRollbackout of the checkpoint. Deferring the read there means making it async and taking that through the transport protocol and both surfaces.So both consumers still resolve eagerly through
loadCheckpoint()— bounded now by retention rather than unbounded. The remaining half (persist the small parts in the reference; read the largeaftersnapshot only on rollback) is task 2.4, and is named in a comment at both call sites rather than left to be rediscovered.A regression I introduced, caught by the tests
Sorting the persisted session list newest-first broke
rollbackChange, which reads sessions back in stored order to find the earliest state to restore to — a reordered list turned a rollback into a conflict. Selection and ordering are now separate: chosen by recency, written in the caller's order.Test plan
npm run typecheck— cleanpackages/extension— greenpackages/core— 515/516; the one failure isgit.push.test.ts, already tracked ascore-test-worker-contentionand untouched hereopenspec change validate --strict— validnpm run lint:english— passed🤖 Generated with Claude Code