Repository navigation
fix(query): read all of a query's lookups under one lock (#555) - #557
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
selective_fact_fetch called FactStorage::get_live_facts once per entity or attribute lookup, each under its own read lock, so a transaction that committed between two lookups was seen by some patterns of the query and not others. FactStorage::get_live_facts_of_scans reads every scan under one read guard, and selective_fact_fetch uses it. The #555 reproducer was passing only because of #554: execute added :pair/x before :pair/y, and scans run in sorted attribute order, so the query with :pair/x positive could only catch a partial commit. It now also runs the query with :pair/y positive, which fails without this fix (every one of 5 runs) and passes with it. Both are un-ignored. Refs #555 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 #555 (data-integrity: stays open until a release ships the fix).
Stacked on #556 (#554): includes its commits until it merges; the change here is the last commit, 38c0054.
Bug
selective_fact_fetchcalledFactStorage::get_live_factsonce per entity or attribute lookup, each under its own read lock. A transaction that committed between two lookups was seen by some patterns of the query and not others, so the query could return rows matching no state of the database.Fix
FactStorage::get_live_facts_of_scansreads every scan under one read guard (pending facts then committed facts per scan, in the order given, as before).get_live_factsdelegates to it;selective_fact_fetchbuilds its scan list and makes one call.The other query paths already read storage once per query: full scans,
:as-oftimestamps, and rules (one filtered snapshot).or,not,not-joinand prepared queries reuse that snapshot.Reproducer correction
The #555 reproducer was only passing because of #554.
executeadded:pair/xbefore:pair/y, and scans run in sorted attribute order. So the query with:pair/xpositive could only catch a partial commit, never a commit between two lookups. The test now also runs the query with:pair/ypositive. Without this fix it failed in all 5 runs, usually within milliseconds; with it, it passes. Both variants (in-memory, file) are un-ignored.Tests
live_facts_of_scans_concatenates_each_scan_in_order.Codex review: no findings.
🤖 Generated with Claude Code
https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h