Repository navigation
fix(storage): retry pending context-offload checkpoints - #6151
Conversation
Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
First review at e6d722e (+93/-1, 3 files).
The change keeps a reclamation WAL checkpoint pending until it actually completes. Before, reclaimFreePages ran PRAGMA wal_checkpoint(PASSIVE) once, right after a shrink. If a reader blocked part of it, nothing retried, and the freed pages stayed in the WAL until some unrelated checkpoint. Now SqliteReclamationCheckpoint marks a checkpoint pending when pages were reclaimed. Later idle ticks (freelist already 0) retry it until busy === 0 && checkpointed >= log.
Checked:
- PASSIVE never waits on readers or writers, and the call stays outside the read/write transactions, so an idle tick still never takes the write lock.
- When the database is not in WAL mode, SQLite returns
log = checkpointed = -1, so the pending flag clears and nothing loops. - The result row is validated, and an unexpected shape throws out of the maintenance call instead of being treated as complete.
- The new test holds a reader, checks that the first checkpoint stays pending and that a later idle tick completes it.
Findings: none at P0–P3.
CI: test and label pass on this head. Mergeable. No protocol files are touched.
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I reviewed all three changed files at e6d722e. No P0–P2 defects found.
sqlite-context-offload-store.ts:520–536 now retries an unfinished reclamation checkpoint even after the freelist reaches zero. sqlite-reclamation-checkpoint.ts:26–39 retains pending work from SQLite's returned checkpoint status. The PASSIVE call runs after the store transaction ends. Pending checkpoint work does not set hasMore, so the Host maintenance scheduler keeps its normal idle cadence instead of spinning. Actual page reclamation remains bounded by maxPages. No schema or migration changes are introduced.
Validation: fresh dependency installation and normal root build:test passed. All 26 real-SQLite context-offload store tests passed, plus 24 adjacent context, retention and Host maintenance tests. An independent two-cycle disk probe reclaimed 244 pages each time, kept the main file unchanged while a reader pinned it, and then shrank it from 1,073,152 to 73,728 bytes through idle retry calls. Calls made while another connection held BEGIN IMMEDIATE returned in approximately 3.6 ms; both cycles ended with quick_check = ok. Restoring only the parent store implementation makes both the new regression and the independent probe fail at the expected file-shrink assertion. Biome passed for all three changed files; the epoch guard passed at 222.
I completed the independent analysis and checks before reading the existing review, and agree with its conclusion. No additional findings. At the final check, exact-head test and label succeeded and GitHub reported MERGEABLE.
I did not rerun the complete storage suite or exercise this on Windows/macOS. This change addresses the context-offload store; it does not add retry behavior to other database reclamation paths.
|
Thanks for retrying incomplete context-offload checkpoints and for the regression coverage. The reviewed head
|
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: merge the small-fix group in the October 21 09:00 report. The reviewed head is e6d722e, its test checks pass, and it has no merge conflict. This approval is submitted by an automated agent at that explicit request.
Summary
Retain incomplete PASSIVE checkpoint state after context-offload page reclamation and retry it on later idle calls, including after the freelist drains. The small internal helper reads SQLite's checkpoint result and keeps the retry outside write transactions; pending readers do not turn
hasMoreinto a busy loop. The runtime lane remains in #6083 and can reuse the helper.Fixes #6149
Fixes #6149
Verification
quick_check. It fails against the original store implementation at the file-shrink assertion.npm run lint,npm run format:check,npm run build,npm run typecheck, both requiredknipworkspace checks, and commit hooks.EPERMon the OS account'sLibrary/Application Support/Maka/state-root-ownersorLibrary/Caches/Maka; the run also reported lock/watch timeouts andio_failureassertions. This run was not compared against the base and is not claimed green. Node 24.12.0.AI use
Tool(s) and scope: OpenAI Codex prepared this automated contribution's analysis, implementation, regression test, and PR text. The commit includes
Generated-by: OpenAI Codex.Checklist
Does this PR entail a change in behavior?