Repository navigation
fix(storage): add a committed transaction's facts under one lock (#554) - #556
Merged
Merged
Conversation
Ignored: they fail on v3 and main today. A concurrent query sees part of a WriteTransaction commit, and two patterns of one query can read different states. The checkpoint double-read case did not reproduce. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h
A commit wrote its WAL entry, then added the 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. execute's transact and retract path (since v0.6.0, 21b555a) did this as well as WriteTransaction::commit (since v0.5.0). FactStorage::load_facts appends a batch under one write lock. Commits, WAL replay, BrowserDb's commit and the v7 in-memory load use it. The two #554 reproducers are un-ignored, with execute variants added. Refs #554 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h
This was referenced Oct 11, 2026
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
q-qp-p
pushed a commit
to q-qp-p/minigraf
that referenced
this pull request
Oct 19, 2026
…minigraf#554, project-minigraf#555) Backport of the v3 fixes (PRs project-minigraf#556, project-minigraf#557) to v2.x. project-minigraf#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. project-minigraf#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 project-minigraf#555 reproducer disables checkpoints: on v2.x a query that runs during a checkpoint can lose committed facts, a separate bug. Refs project-minigraf#554, project-minigraf#555 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h
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 #554 (data-integrity: stays open until a release ships the fix).
Bug
A commit wrote its WAL entry, then added the transaction's facts to
FactStorageoneload_factcall at a time, each under its own write lock. A query on another thread could run between two calls and see part of the transaction.The issue said
execute("(transact ...)")was atomic. It is not:Minigraf::execute's transact/retract path has added facts one at a time since v0.6.0 (21b555a, WAL-first ordering);WriteTransaction::commitsince v0.5.0.transact_batchis only used below the facade.Fix
FactStorage::load_factsappends a batch under one write lock, with the same duplicate skip asload_fact. Used by:Minigraf::executetransact/retractWriteTransaction::commitBrowserDbcommitLogWriterstill loads one fact at a time into its own private store, which no query reads.Tests
tests/snapshot_atomicity_test.rs(reproducers fromtest/snapshot-atomicity-repro): the twoWriteTransactionreproducers are un-ignored, andexecutevariants (in-memory, file) are added. All four failed within milliseconds before the fix and pass after.RUN_FORis now 3 s.load_facts_appends_a_batch_and_skips_duplicates.Codex review: no findings.
A v2.x backport to
mainfollows.🤖 Generated with Claude Code
https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h