Repository navigation
fix(storage): queries wait for a checkpoint instead of reading it half-written (#558) - #560
Merged
Merged
Conversation
Backport of the v3 fixes (PRs #556, #557) to v2.x. #554: commits added a transaction's facts to FactStorage one load_fact call at a time, each under its own write lock, so a query on another thread could see part of the transaction. FactStorage::load_facts appends a batch under one lock; execute, WriteTransaction::commit, WAL replay and BrowserDb's commit use it. #555: selective_fact_fetch read each entity/attribute lookup under its own read lock, so a commit between two lookups was seen by some patterns and not others. The three lookup bodies become functions over &FactData and FactStorage::get_facts_for_lookups runs them all under one read guard. The old methods remain for tests only. The file-backed #555 reproducer disables checkpoints: on v2.x a query that runs during a checkpoint can lose committed facts, a separate bug. Refs #554, #555 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h
…f-written (#558) save() appends the new fact pages at 1 + old_fact_page_count, over the pages of the index that queries on other threads read through the published CommittedIndexReader, and then published the new readers and cleared the pending facts with three separately locked calls. A query during a checkpoint could miss committed facts or see part of a transaction. FactStorage::lock_for_checkpoint returns a guard on the store's write lock. save() takes it before the backend lock (the order queries use: store read lock, then backend), reads the pending facts through it, and publishes both readers and clears pending in one step (CheckpointGuard::publish). get_pending_facts and post_checkpoint_clear are now test-only. v3 is not affected (copy-on-write checkpoints). Refs #558 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Oct 11, 2026
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.
Refs #558 (data-integrity: stays open until a release ships the fix). v2.x only; v3's copy-on-write checkpoints are not affected.
Stacked on #559 (the #554/#555 backport, which adds
tests/snapshot_atomicity_test.rs). The change here is the last commit, 5d0304c.Bug
save()writes the new fact pages at1 + old_fact_page_count, over the pages of the index that queries on other threads read through the publishedCommittedIndexReader. It then published the new readers and cleared the pending facts with three separately locked calls. A query during a checkpoint could miss committed facts (1 seen of 43) or see part of a transaction. This includes the automatic checkpoint everywal_checkpoint_thresholdWAL entries and the one at close.Fix
FactStorage::lock_for_checkpointreturns aCheckpointGuardon the store's write lock.save():CheckpointGuard::publish).Queries now wait for the whole checkpoint, including its fsyncs; commits were already serialized with it by the write lock.
get_pending_factsandpost_checkpoint_clearare now test-only.Tests
checkpoint_never_hides_committed_facts: a writer commits a pair of facts and checkpoints, in a loop. A reader checks that no entity has half a pair and that it never sees fewer entities than were committed before the query began.Codex review
No deadlock and no misplaced guard found. One point is out of scope: if
save()fails after its first page write, the old readers stay published over overwritten pages, so later queries on that handle can be wrong. That happens without any concurrency. It is the in-process form of the known v2.x issue #374 (in #421), and v3 handles it through #390's refuse-after-failed-checkpoint, which has no v2.x backport.🤖 Generated with Claude Code
https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h