Skip to content

fix(storage): retry pending context-offload checkpoints - #6151

Merged
Astro-Han merged 1 commit into
apache:mainfrom
Dante-dan:fix/context-offload-checkpoint-retry
Oct 21, 2026
Merged

Astro-Han merged 1 commit into
apache:mainfrom
Dante-dan:fix/context-offload-checkpoint-retry

Conversation

@Dante-dan

@Dante-dan Dante-dan commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

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 hasMore into a busy loop. The runtime lane remains in #6083 and can reuse the helper.

Fixes #6149

Fixes #6149

Verification

  • Context-offload store suite: 26/26 passed. The new real-SQLite regression pins a reader across the final batch, checks idle cadence and a competing writer, then verifies the next available idle checkpoint shrinks the file and preserves quick_check. It fails against the original store implementation at the file-shrink assertion.
  • Passed: npm run lint, npm run format:check, npm run build, npm run typecheck, both required knip workspace checks, and commit hooks.
  • Full storage suite: 1,690 tests, 1,228 passed, 454 failed, 8 skipped. Most failures include sandbox EPERM on the OS account's Library/Application Support/Maka/state-root-owners or Library/Caches/Maka; the run also reported lock/watch timeouts and io_failure assertions. This run was not compared against the base and is not claimed green. Node 24.12.0.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Oct 18, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for retrying incomplete context-offload checkpoints and for the regression coverage. The reviewed head e6d722e7b6861b92558d924c5f125b569b1e1254 is unchanged, its test checks pass, and it merges cleanly with current main. I am proceeding with the merge at Astro-Han's explicit request.

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Astro-Han
Astro-Han merged commit 2bb4f25 into apache:main Oct 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(storage): context-offload page reclamation never retries an unfinished WAL checkpoint

2 participants