Repository navigation
feat(storage): checkpoint publishes under one lock (streaming engine 1/9, PR 1 of 3) - #578
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HyLscCUuJ1GQpdAeX79EdR
…e lock (#543) Pending facts, their duplicate-check keys and indexes move into a PendingStore behind its own lock. FactData holds the committed reader, its generation and the active pending store. A checkpoint calls FactStorage::publish, which sets all three under one write lock instead of set_committed_reader followed by post_checkpoint_clear, so no read sees a checkpointed fact twice (#473 spec §4.1). The old store is replaced, not cleared. Appends hold FactData's read lock while writing the store. Un-ignores checkpoint_never_shows_a_fact_twice. The race did not reproduce in 34 runs before the change; the test stays as a regression guard. Part of #473 (streaming engine 1/9). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HyLscCUuJ1GQpdAeX79EdR
…ree PRs Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HyLscCUuJ1GQpdAeX79EdR
…543) A checkpoint reads the pending facts, writes them, then publishes. publish now takes how many it wrote and carries any later facts into the new pending store. The database's write lock keeps that range empty today, but FactStorage is cloneable and does not enforce it (Codex review). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HyLscCUuJ1GQpdAeX79EdR
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Oct 13, 2026
Merged
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.
Part of #543 (streaming engine 1/9, #473), PR 1 of 3. Plan:
docs/superpowers/plans/2026-10-11-543-snapshots-and-quarantine.md(step 1). The plan now splits #543 into three PRs. This one is step 1; PR 2 adds pins and page quarantine and moves the fact log onto them; PR 3 adds query and cursor snapshots,BrowserDb, the crash test and the docs.What changes
PendingStorebehind its own lock.FactDataholds the committed reader, its generation and the active pending store. Lock order isFactData, thenpending. Appends holdFactData's read lock while they write the store, so none lands in a store being replaced.save,rebuild_indexes) callsFactStorage::publish(reader, generation, checkpointed). It sets the new committed reader, its generation and a new pending store under one write lock. Before, it calledset_committed_readerand thenpost_checkpoint_clearunder two separate locks, so a query in between could read the checkpointed facts twice (spec §4.1). The old store is replaced, not cleared, which PR 3's snapshots need.publishcarries any facts at positionscheckpointed..into the new store. The write lock keeps that range empty today, butFactStorageis cloneable and does not enforce it (Codex review finding).set_committed(reader, generation), which keeps the pending store.Tests
publish_replaces_pending_store_and_reader_together,publish_keeps_facts_appended_after_the_checkpoint_read,set_committed_keeps_pending_store. They replacetest_post_checkpoint_clear_clears_indexes.checkpoint_never_shows_a_fact_twice(tests/snapshot_atomicity_test.rs) is no longer ignored. It did not fail in 34 runs before the change (the window between the two locks is a few instructions), so it is a regression guard, not a demonstration.cargo test: 1436 passed, 10 ignored.cargo fmt,cargo clippyand the browser wasm clippy are clean.No CHANGELOG entry: the double read existed only in unreleased v3 code. No public API change and no new error code.
Review
Codex (
codex exec -s read-only) found one issue, which is fixed here: a fact appended between a checkpoint's read of the pending facts and its publish would be lost. No production path can do that today, because file-backed writes hold the write lock across the append and the checkpoint. It found nothing else in the lock ordering, the read paths, poisoned-lock handling, the browser build or the fact log. The fact log is unaffected because both checkpoint paths still refuse while a log is open (API-013), so its pending positions stay valid.🤖 Generated with Claude Code
https://claude.ai/code/session_01HyLscCUuJ1GQpdAeX79EdR