From 710fb56db74940eccb2fcd4cd7050a0802dc7bfd Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Sun, 11 Oct 2026 11:04:27 +0530 Subject: [PATCH 1/7] fix(query): count only derived facts against max_results (#528) Backport of the v3 fix (#534) to v2.x. RecursiveEvaluator seeded seen_facts with every base fact and compared its size with max_results, so on a database with max_results (default 1,000,000) live facts every rule query failed with INT-020 at its first derived fact, whatever the size of its answer. The limit now counts only the facts the rules derive. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h --- CHANGELOG.md | 1 + CLAUDE.md | 2 +- README.md | 2 +- docs/ERROR_REFERENCE.md | 3 ++- docs/TEST_COVERAGE.md | 4 ++-- src/db.rs | 8 +++++--- src/query/datalog/evaluator.rs | 10 ++++++---- tests/recursive_rules_test.rs | 35 ++++++++++++++++++++++++++++++++++ 8 files changed, 53 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4938b401a..8194d88db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **A file shorter than one page is refused at open with `STG-045` unless it is a torn first write (#506).** Open treated any file under 4 KiB as a new database and wrote a new header over it, so a database cut below one page by an interrupted copy, or an unrelated file at the path, was silently replaced. Such a file now opens as a new database only when its bytes are zeros or a prefix of the header a new database writes (what a crash during creation can leave); anything else is `STG-045` and the file is not modified. A zero-length file is still a new database. New code `STG-045`, numbered as in v3.0.0, which also fixes this. - **A commit made after a torn WAL entry is no longer lost on recovery (#513).** Recovery replays the WAL up to its first partial or damaged entry, but the writer appended after the end of the file. After a crash during an append, the next session's commits went in after the partial entry, so a second crash before any checkpoint lost them, although each had been fsynced and reported as committed. A failed append (short write, full disk, I/O error) did the same to the next commit through that handle. Opening a database for writing now cuts the WAL back to the end of its last valid entry, and syncs it, before anything is appended. After a failed append, every later write through that handle fails with the new `WAL-007` until the database is reopened or a `checkpoint()` succeeds. The transaction whose append failed is not applied; after a reopen it is there in full or not at all. Affected every release since v0.5.0. Also fixed in v3.0.0. +- **Rule queries no longer fail with `INT-020` on databases with 1,000,000 or more live facts (#528).** The recursive evaluator counted the database's own facts against `max_results`, so once a database held that many live facts, every rule query failed at its first derived fact, whatever the size of its answer. `max_results` now counts only the facts the rules derive, so raising it with `OpenOptions::max_results` or `:max-results` is no longer needed. Affected every release since v0.18.0. Also fixed in v3.0.0. ### Tests diff --git a/CLAUDE.md b/CLAUDE.md index 22d8885b1..63d02547b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -174,7 +174,7 @@ Auto-migrates v1/v2/v3/v4/v5/v6 → v7 on open/checkpoint. ## Test Coverage -**1237 tests** (1228 passing, 9 ignored; unit + integration + doc). +**1238 tests** (1229 passing, 9 ignored; unit + integration + doc). See `docs/TEST_COVERAGE.md` for the full per-file breakdown. **Testing conventions** — see the Testing Conventions section below before writing any tests. diff --git a/README.md b/README.md index 352ad4bd1..5a5222f6d 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,7 @@ let r2 = pq.execute(&[("tx", BindValue::TxCount(2)), ("entity", BindValue::Entit ```bash cargo run # interactive Datalog REPL -cargo test # run 1237 tests +cargo test # run 1238 tests cargo run < demos/demo_recursive.txt # recursive rules demo ``` diff --git a/docs/ERROR_REFERENCE.md b/docs/ERROR_REFERENCE.md index c94de6a9e..aaa774af9 100644 --- a/docs/ERROR_REFERENCE.md +++ b/docs/ERROR_REFERENCE.md @@ -2540,7 +2540,8 @@ similar). The `{}` carries the specific underlying message. **Cause**: The recursive rule evaluator (semi-naive evaluation) exceeded its maximum iteration count, maximum derived facts per iteration, or maximum -query result count — guards against infinite recursion/cycles or +number of facts derived in total (`max_results`; the database's own facts do +not count) — guards against infinite recursion/cycles or runaway-generating rules. **Resolution**: diff --git a/docs/TEST_COVERAGE.md b/docs/TEST_COVERAGE.md index fb0d91981..399602210 100644 --- a/docs/TEST_COVERAGE.md +++ b/docs/TEST_COVERAGE.md @@ -4,7 +4,7 @@ **Verified**: 2026-10-10 with `cargo test` -**Result**: 1,228 passing tests and 9 ignored tests (1,237 total), including the golden-file corpus (#391) +**Result**: 1,229 passing tests and 9 ignored tests (1,238 total), including the golden-file corpus (#391) Since v2.0.4, `tests/edge_cases_test.rs` checks that a file of 1 to 4,095 bytes is `STG-045`, unchanged and with no WAL, when it is a database cut below one page or foreign bytes, and opens as a new database that takes a write and reads it back after reopen when it is a prefix of the initial header or all zeros (#506). @@ -16,7 +16,7 @@ Since v2.0.4, torn-WAL-tail tests (#513) check that opening a WAL writer cuts of Not counted above: the `BrowserDb` tests run only on wasm32 with `wasm-bindgen-test` in a browser. Two of them check that the IndexedDB flush returns every page a write dirtied, and that a dirty page that cannot be read is an error rather than left out. -The v2.0.4 suite covers the public database API, the Datalog engine, storage and recovery paths, and the v2.0.0 locking and structured-error-code changes, plus v2.0.1's regression test for rebuilding indexes on open after several checkpoints (#370). Since v2.0.1, `tests/point_query_history_test.rs` and new storage unit tests check that attribute-narrowed bound-entity lookups return exactly what a full scan returns, and that `net_asserted_facts` matches its previous implementation on randomized histories (#323). Attribute-scan tests check that the AEVT range covers exactly the queried attribute, including non-ASCII names such as `:丿` and prefix siblings such as `:ab` for `:a` (#381). Incremental B+tree rebuild tests check that checkpoints which copy untouched index leaves produce exactly the entries and file checksum a full rebuild would, including over repeated checkpoints, that single-fact checkpoints do not fragment the index, and that a cyclic leaf chain or damaged slot directory is rejected rather than copied (#315). Directory-sync unit tests check that creating the `.graph` file or the WAL, and deleting the WAL, fsync the parent directory after the file operation, and that reopening an existing file does not (#389). Disjunction tests check that `or-join` returns an empty result, not INT-031, when no rows reach it: on an empty database, after a pattern that matches nothing, after a `not` that removes every row, and in a rule body (#405). A stratification unit test builds the same two-rule negative cycle 64 times and checks that the INT-054 message names the same predicate pair every time (#410). Magic-sets tests check that a recursive rule with a literal start that recurses through another rule returns the same results as without the literal (#297). API tests check that `execute()` and `WriteTransaction::execute()` reject `$slot` bind tokens in every position with API-010 (#407). The REPL `run()` test now runs in a child process with null stdin, so it no longer hangs when the test runner's stdin is an open non-TTY stream (#415). A fault-injection test crashes `save()` at each of its 47 page writes and syncs and checks that every reopen finds exactly the old or the new checkpointed facts (#374). For v2.0.3, storage unit tests and two `edge_cases_test.rs` tests check that more than 65,535 uncheckpointed facts each resolve to their own fact through entity lookups and attribute scans, in memory and file-backed, before and after a checkpoint, and that the widened `FactRef` encodes to the same bytes as before (#445). A WAL test reopens a database next to a WAL whose entries are all checkpointed, writes again, and checks that `:as-of` history is unchanged before and after another reopen; a unit test checks that restoring the transaction counter never lowers it (#447). Since v2.0.3, `tests/golden_corpus_test.rs` opens a copy of each committed golden file in `tests/golden/` (v7 files written by minigraf 2.0.3: one checkpoint, several checkpoints, indexes rebuilt on open, a crash-left WAL, an already-checkpointed WAL, same-transaction multi-values) and checks its manifest's CRC32, `tx_count` and query results after open, after a checkpoint and reopen, and after one more write; a second test checks that every golden file has a manifest (#391). +The v2.0.4 suite covers the public database API, the Datalog engine, storage and recovery paths, and the v2.0.0 locking and structured-error-code changes, plus v2.0.1's regression test for rebuilding indexes on open after several checkpoints (#370). Since v2.0.1, `tests/point_query_history_test.rs` and new storage unit tests check that attribute-narrowed bound-entity lookups return exactly what a full scan returns, and that `net_asserted_facts` matches its previous implementation on randomized histories (#323). Attribute-scan tests check that the AEVT range covers exactly the queried attribute, including non-ASCII names such as `:丿` and prefix siblings such as `:ab` for `:a` (#381). Incremental B+tree rebuild tests check that checkpoints which copy untouched index leaves produce exactly the entries and file checksum a full rebuild would, including over repeated checkpoints, that single-fact checkpoints do not fragment the index, and that a cyclic leaf chain or damaged slot directory is rejected rather than copied (#315). Directory-sync unit tests check that creating the `.graph` file or the WAL, and deleting the WAL, fsync the parent directory after the file operation, and that reopening an existing file does not (#389). Disjunction tests check that `or-join` returns an empty result, not INT-031, when no rows reach it: on an empty database, after a pattern that matches nothing, after a `not` that removes every row, and in a rule body (#405). A stratification unit test builds the same two-rule negative cycle 64 times and checks that the INT-054 message names the same predicate pair every time (#410). Magic-sets tests check that a recursive rule with a literal start that recurses through another rule returns the same results as without the literal (#297). API tests check that `execute()` and `WriteTransaction::execute()` reject `$slot` bind tokens in every position with API-010 (#407). The REPL `run()` test now runs in a child process with null stdin, so it no longer hangs when the test runner's stdin is an open non-TTY stream (#415). A fault-injection test crashes `save()` at each of its 47 page writes and syncs and checks that every reopen finds exactly the old or the new checkpointed facts (#374). For v2.0.3, storage unit tests and two `edge_cases_test.rs` tests check that more than 65,535 uncheckpointed facts each resolve to their own fact through entity lookups and attribute scans, in memory and file-backed, before and after a checkpoint, and that the widened `FactRef` encodes to the same bytes as before (#445). A WAL test reopens a database next to a WAL whose entries are all checkpointed, writes again, and checks that `:as-of` history is unchanged before and after another reopen; a unit test checks that restoring the transaction counter never lowers it (#447). Since v2.0.3, `tests/golden_corpus_test.rs` opens a copy of each committed golden file in `tests/golden/` (v7 files written by minigraf 2.0.3: one checkpoint, several checkpoints, indexes rebuilt on open, a crash-left WAL, an already-checkpointed WAL, same-transaction multi-values) and checks its manifest's CRC32, `tx_count` and query results after open, after a checkpoint and reopen, and after one more write; a second test checks that every golden file has a manifest (#391). A recursive-rules test runs rule queries on a database with more live facts than `max_results` and checks that they answer, and that the limit still stops a rule that derives more facts than it allows (#528). ### Covered Areas diff --git a/src/db.rs b/src/db.rs index d0128a085..541dd8ba5 100644 --- a/src/db.rs +++ b/src/db.rs @@ -128,7 +128,8 @@ pub struct OpenOptions { /// Maximum facts that can be derived per recursive rule iteration. /// Defaults to 1_000_000. Use to prevent runaway recursive rules. pub max_derived_facts: usize, - /// Maximum total query results. Defaults to 1_000_000. + /// Maximum facts a rule query may derive. Defaults to 1_000_000. + /// The database's own facts do not count against it. pub max_results: usize, /// Open even when the filesystem cannot lock files at all. /// @@ -199,9 +200,10 @@ impl OpenOptions { self } - /// Set the maximum total query results. + /// Set the maximum number of facts a rule query may derive. /// - /// Defaults to 1_000_000. Use lower values to limit result set size. + /// Defaults to 1_000_000. The database's own facts do not count against it, + /// and queries without rules are not limited by it. pub fn max_results(mut self, n: usize) -> Self { self.max_results = n; self diff --git a/src/query/datalog/evaluator.rs b/src/query/datalog/evaluator.rs index 7e58d225c..3dfd10ade 100644 --- a/src/query/datalog/evaluator.rs +++ b/src/query/datalog/evaluator.rs @@ -68,7 +68,7 @@ pub struct RecursiveEvaluator { max_iterations: usize, /// Maximum facts that can be derived per iteration max_derived_facts: usize, - /// Maximum total query results + /// Maximum facts the rules may derive in one evaluation; base facts do not count max_results: usize, } @@ -81,7 +81,7 @@ impl RecursiveEvaluator { /// * `functions` - Function registry for UDF predicates/aggregates /// * `max_iterations` - Safety limit (e.g., 1000) /// * `max_derived_facts` - Maximum facts per iteration - /// * `max_results` - Maximum total results + /// * `max_results` - Maximum facts the rules may derive (base facts do not count) pub fn new( storage: FactStorage, rules: Arc>, @@ -133,6 +133,8 @@ impl RecursiveEvaluator { (f.entity, f.attribute.clone(), encoded) }) .collect(); + // `max_results` limits the facts the rules derive, not the database (#528). + let base_count = seen_facts.len(); let mut iteration = 0; @@ -170,8 +172,8 @@ impl RecursiveEvaluator { let encoded = encode_value(&fact.value); let key = (fact.entity, fact.attribute.clone(), encoded); if !seen_facts.contains(&key) { - // Check total result limit - if seen_facts.len() >= self.max_results { + // Check total derived-fact limit + if seen_facts.len() - base_count >= self.max_results { return Err(err_coded!( ErrorCode::Int020, format!("Max query results ({}) exceeded.", self.max_results) diff --git a/tests/recursive_rules_test.rs b/tests/recursive_rules_test.rs index 7428e48f5..967b36243 100644 --- a/tests/recursive_rules_test.rs +++ b/tests/recursive_rules_test.rs @@ -468,3 +468,38 @@ fn test_convergence_simple_graph() { _ => panic!("Expected QueryResults"), } } + +/// #528: `max_results` limits what the rules derive, not the size of the database. +/// A rule query must answer on a database with more live facts than `max_results`. +#[test] +fn test_max_results_counts_derived_facts_not_base_facts() { + let db = minigraf::OpenOptions::new() + .max_results(10) + .open_memory() + .unwrap(); + // A chain a -> b -> c -> d, plus 50 unrelated facts: 53 live facts in all. + exec( + &db, + "(transact [[:a :manager :b] [:b :manager :c] [:c :manager :d]])", + ); + let filler: String = (0..50).map(|i| format!("[:p{i} :age {i}] ")).collect(); + exec(&db, &format!("(transact [{filler}])")); + exec(&db, "(rule [(boss ?x ?y) [?x :manager ?y]])"); + exec(&db, "(rule [(boss ?x ?y) [?x :manager ?m] (boss ?m ?y)])"); + + // Bound first argument (magic-sets path): b, c, d. + match exec(&db, "(query [:find ?y :where (boss :a ?y)])") { + QueryResult::QueryResults { results, .. } => assert_eq!(results.len(), 3), + _ => panic!("Expected QueryResults"), + } + // All free: 6 pairs. + match exec(&db, "(query [:find ?x ?y :where (boss ?x ?y)])") { + QueryResult::QueryResults { results, .. } => assert_eq!(results.len(), 6), + _ => panic!("Expected QueryResults"), + } + // The limit still stops a rule that derives more than it allows. + let err = db + .execute("(query [:find ?x ?y :where (boss ?x ?y) :max-results 5])") + .unwrap_err(); + assert_eq!(err.code(), "INT-020"); +} From 34f52e60854cf0df4e1e2da49e0cb1342109dd8f Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Sun, 11 Oct 2026 13:24:11 +0530 Subject: [PATCH 2/7] fix(db): remove an all-checkpointed WAL; usize::MAX threshold skips close-time save (#457, #476) Backport of #525 and #526 (v3) to v2.x. - #457: do_checkpoint's early return (nothing to flush) now deletes a WAL whose entries are all already in the file, and syncs the parent directory. The golden-corpus test requires v7_stale_wal to have no WAL after a checkpoint. - #476: Inner::drop under wal_checkpoint_threshold(usize::MAX) discards the storage's dirty flag, so PersistentFactStorage::drop saves nothing; the WAL keeps the pending facts for the next open. OpenOptions field docs say what usize::MAX does. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h --- CHANGELOG.md | 2 + CLAUDE.md | 2 +- README.md | 2 +- docs/TEST_COVERAGE.md | 4 +- src/db.rs | 37 ++++++++++++- src/storage/persistent_facts.rs | 8 +++ tests/golden_corpus_test.rs | 6 +-- tests/wal_test.rs | 95 +++++++++++++++++++++++++++++++++ 8 files changed, 148 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8194d88db..21fdc3ac8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **A file shorter than one page is refused at open with `STG-045` unless it is a torn first write (#506).** Open treated any file under 4 KiB as a new database and wrote a new header over it, so a database cut below one page by an interrupted copy, or an unrelated file at the path, was silently replaced. Such a file now opens as a new database only when its bytes are zeros or a prefix of the header a new database writes (what a crash during creation can leave); anything else is `STG-045` and the file is not modified. A zero-length file is still a new database. New code `STG-045`, numbered as in v3.0.0, which also fixes this. - **A commit made after a torn WAL entry is no longer lost on recovery (#513).** Recovery replays the WAL up to its first partial or damaged entry, but the writer appended after the end of the file. After a crash during an append, the next session's commits went in after the partial entry, so a second crash before any checkpoint lost them, although each had been fsynced and reported as committed. A failed append (short write, full disk, I/O error) did the same to the next commit through that handle. Opening a database for writing now cuts the WAL back to the end of its last valid entry, and syncs it, before anything is appended. After a failed append, every later write through that handle fails with the new `WAL-007` until the database is reopened or a `checkpoint()` succeeds. The transaction whose append failed is not applied; after a reopen it is there in full or not at all. Affected every release since v0.5.0. Also fixed in v3.0.0. - **Rule queries no longer fail with `INT-020` on databases with 1,000,000 or more live facts (#528).** The recursive evaluator counted the database's own facts against `max_results`, so once a database held that many live facts, every rule query failed at its first derived fact, whatever the size of its answer. `max_results` now counts only the facts the rules derive, so raising it with `OpenOptions::max_results` or `:max-results` is no longer needed. Affected every release since v0.18.0. Also fixed in v3.0.0. +- `checkpoint()` removes a WAL whose entries are all already in the file (#457). When it had no facts to flush, it returned early and left the WAL until a later write was checkpointed. That WAL is left by a crash between a checkpoint's commit and its WAL delete, or by a failed delete (`WAL-006`). Its contents were always skipped on replay, so no data was affected; the sidecar file just stayed. Also fixed in v3.0.0. +- Closing a handle opened with `wal_checkpoint_threshold(usize::MAX)` no longer writes its pending facts to the `.graph` file (#476). The sentinel was documented to suppress the close-time checkpoint, and `Minigraf`'s close did skip it, but the storage layer still saved on drop, leaving a WAL whose entries were all checkpointed. The file now stays as the last explicit `checkpoint()` left it, and the WAL keeps every pending fact for the next open to replay, so bulk loaders and benchmarks see the pending WAL they ask for. No data is lost either way. Also fixed in v3.0.0. ### Tests diff --git a/CLAUDE.md b/CLAUDE.md index 63d02547b..a00a92265 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -174,7 +174,7 @@ Auto-migrates v1/v2/v3/v4/v5/v6 → v7 on open/checkpoint. ## Test Coverage -**1238 tests** (1229 passing, 9 ignored; unit + integration + doc). +**1241 tests** (1232 passing, 9 ignored; unit + integration + doc). See `docs/TEST_COVERAGE.md` for the full per-file breakdown. **Testing conventions** — see the Testing Conventions section below before writing any tests. diff --git a/README.md b/README.md index 5a5222f6d..97b88b98f 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,7 @@ let r2 = pq.execute(&[("tx", BindValue::TxCount(2)), ("entity", BindValue::Entit ```bash cargo run # interactive Datalog REPL -cargo test # run 1238 tests +cargo test # run 1241 tests cargo run < demos/demo_recursive.txt # recursive rules demo ``` diff --git a/docs/TEST_COVERAGE.md b/docs/TEST_COVERAGE.md index 399602210..dc7d87da8 100644 --- a/docs/TEST_COVERAGE.md +++ b/docs/TEST_COVERAGE.md @@ -4,7 +4,7 @@ **Verified**: 2026-10-10 with `cargo test` -**Result**: 1,229 passing tests and 9 ignored tests (1,238 total), including the golden-file corpus (#391) +**Result**: 1,232 passing tests and 9 ignored tests (1,241 total), including the golden-file corpus (#391) Since v2.0.4, `tests/edge_cases_test.rs` checks that a file of 1 to 4,095 bytes is `STG-045`, unchanged and with no WAL, when it is a database cut below one page or foreign bytes, and opens as a new database that takes a write and reads it back after reopen when it is a prefix of the initial header or all zeros (#506). @@ -16,7 +16,7 @@ Since v2.0.4, torn-WAL-tail tests (#513) check that opening a WAL writer cuts of Not counted above: the `BrowserDb` tests run only on wasm32 with `wasm-bindgen-test` in a browser. Two of them check that the IndexedDB flush returns every page a write dirtied, and that a dirty page that cannot be read is an error rather than left out. -The v2.0.4 suite covers the public database API, the Datalog engine, storage and recovery paths, and the v2.0.0 locking and structured-error-code changes, plus v2.0.1's regression test for rebuilding indexes on open after several checkpoints (#370). Since v2.0.1, `tests/point_query_history_test.rs` and new storage unit tests check that attribute-narrowed bound-entity lookups return exactly what a full scan returns, and that `net_asserted_facts` matches its previous implementation on randomized histories (#323). Attribute-scan tests check that the AEVT range covers exactly the queried attribute, including non-ASCII names such as `:丿` and prefix siblings such as `:ab` for `:a` (#381). Incremental B+tree rebuild tests check that checkpoints which copy untouched index leaves produce exactly the entries and file checksum a full rebuild would, including over repeated checkpoints, that single-fact checkpoints do not fragment the index, and that a cyclic leaf chain or damaged slot directory is rejected rather than copied (#315). Directory-sync unit tests check that creating the `.graph` file or the WAL, and deleting the WAL, fsync the parent directory after the file operation, and that reopening an existing file does not (#389). Disjunction tests check that `or-join` returns an empty result, not INT-031, when no rows reach it: on an empty database, after a pattern that matches nothing, after a `not` that removes every row, and in a rule body (#405). A stratification unit test builds the same two-rule negative cycle 64 times and checks that the INT-054 message names the same predicate pair every time (#410). Magic-sets tests check that a recursive rule with a literal start that recurses through another rule returns the same results as without the literal (#297). API tests check that `execute()` and `WriteTransaction::execute()` reject `$slot` bind tokens in every position with API-010 (#407). The REPL `run()` test now runs in a child process with null stdin, so it no longer hangs when the test runner's stdin is an open non-TTY stream (#415). A fault-injection test crashes `save()` at each of its 47 page writes and syncs and checks that every reopen finds exactly the old or the new checkpointed facts (#374). For v2.0.3, storage unit tests and two `edge_cases_test.rs` tests check that more than 65,535 uncheckpointed facts each resolve to their own fact through entity lookups and attribute scans, in memory and file-backed, before and after a checkpoint, and that the widened `FactRef` encodes to the same bytes as before (#445). A WAL test reopens a database next to a WAL whose entries are all checkpointed, writes again, and checks that `:as-of` history is unchanged before and after another reopen; a unit test checks that restoring the transaction counter never lowers it (#447). Since v2.0.3, `tests/golden_corpus_test.rs` opens a copy of each committed golden file in `tests/golden/` (v7 files written by minigraf 2.0.3: one checkpoint, several checkpoints, indexes rebuilt on open, a crash-left WAL, an already-checkpointed WAL, same-transaction multi-values) and checks its manifest's CRC32, `tx_count` and query results after open, after a checkpoint and reopen, and after one more write; a second test checks that every golden file has a manifest (#391). A recursive-rules test runs rule queries on a database with more live facts than `max_results` and checks that they answer, and that the limit still stops a rule that derives more facts than it allows (#528). +The v2.0.4 suite covers the public database API, the Datalog engine, storage and recovery paths, and the v2.0.0 locking and structured-error-code changes, plus v2.0.1's regression test for rebuilding indexes on open after several checkpoints (#370). Since v2.0.1, `tests/point_query_history_test.rs` and new storage unit tests check that attribute-narrowed bound-entity lookups return exactly what a full scan returns, and that `net_asserted_facts` matches its previous implementation on randomized histories (#323). Attribute-scan tests check that the AEVT range covers exactly the queried attribute, including non-ASCII names such as `:丿` and prefix siblings such as `:ab` for `:a` (#381). Incremental B+tree rebuild tests check that checkpoints which copy untouched index leaves produce exactly the entries and file checksum a full rebuild would, including over repeated checkpoints, that single-fact checkpoints do not fragment the index, and that a cyclic leaf chain or damaged slot directory is rejected rather than copied (#315). Directory-sync unit tests check that creating the `.graph` file or the WAL, and deleting the WAL, fsync the parent directory after the file operation, and that reopening an existing file does not (#389). Disjunction tests check that `or-join` returns an empty result, not INT-031, when no rows reach it: on an empty database, after a pattern that matches nothing, after a `not` that removes every row, and in a rule body (#405). A stratification unit test builds the same two-rule negative cycle 64 times and checks that the INT-054 message names the same predicate pair every time (#410). Magic-sets tests check that a recursive rule with a literal start that recurses through another rule returns the same results as without the literal (#297). API tests check that `execute()` and `WriteTransaction::execute()` reject `$slot` bind tokens in every position with API-010 (#407). The REPL `run()` test now runs in a child process with null stdin, so it no longer hangs when the test runner's stdin is an open non-TTY stream (#415). A fault-injection test crashes `save()` at each of its 47 page writes and syncs and checks that every reopen finds exactly the old or the new checkpointed facts (#374). For v2.0.3, storage unit tests and two `edge_cases_test.rs` tests check that more than 65,535 uncheckpointed facts each resolve to their own fact through entity lookups and attribute scans, in memory and file-backed, before and after a checkpoint, and that the widened `FactRef` encodes to the same bytes as before (#445). A WAL test reopens a database next to a WAL whose entries are all checkpointed, writes again, and checks that `:as-of` history is unchanged before and after another reopen; a unit test checks that restoring the transaction counter never lowers it (#447). Since v2.0.3, `tests/golden_corpus_test.rs` opens a copy of each committed golden file in `tests/golden/` (v7 files written by minigraf 2.0.3: one checkpoint, several checkpoints, indexes rebuilt on open, a crash-left WAL, an already-checkpointed WAL, same-transaction multi-values) and checks its manifest's CRC32, `tx_count` and query results after open, after a checkpoint and reopen, and after one more write; a second test checks that every golden file has a manifest (#391). A recursive-rules test runs rule queries on a database with more live facts than `max_results` and checks that they answer, and that the limit still stops a rule that derives more facts than it allows (#528). A WAL test restores a WAL whose entries are all checkpointed and checks that an explicit `checkpoint()` removes it, keeps the fact, and that the next write does not reuse tx 1; the golden-corpus test now requires `v7_stale_wal` to have no WAL after a checkpoint too (#457). A WAL test checks that dropping a handle opened with `wal_checkpoint_threshold(usize::MAX)` leaves the `.graph` file byte for byte unchanged under `Full` and `Normal` sync, keeps the WAL, and that a reopen replays it (#476). ### Covered Areas diff --git a/src/db.rs b/src/db.rs index 541dd8ba5..b2d43d2cf 100644 --- a/src/db.rs +++ b/src/db.rs @@ -118,6 +118,11 @@ pub struct OpenOptions { /// A checkpoint's cost grows with the total size of the database (see /// [`Minigraf::checkpoint`]), while durability does not depend on it: the WAL /// is crash-durable. Raise this for write-heavy workloads on large graphs. + /// + /// `usize::MAX` turns off every implicit checkpoint: no auto-checkpoint and + /// nothing written to the `.graph` file on close. Only an explicit + /// [`Minigraf::checkpoint`] writes it; until then the WAL holds the pending + /// facts, and the next open replays them. pub wal_checkpoint_threshold: usize, /// Number of pages to hold in the LRU page cache. Default: 256 (= 1MB at 4KB pages). /// @@ -305,8 +310,14 @@ impl Drop for Inner { // On clean close, perform a best-effort checkpoint to reduce WAL size. // Errors are silently ignored (can't propagate from Drop). // Skip if wal_checkpoint_threshold is usize::MAX — that sentinel suppresses - // all checkpointing (used by benchmarks to keep WAL entries pending). + // all checkpointing (used by benchmarks to keep WAL entries pending), + // including the storage's own save on drop (#476). The WAL holds every + // pending fact, and the next open replays it. if self.options.wal_checkpoint_threshold == usize::MAX { + #[cfg(not(target_arch = "wasm32"))] + if let Ok(WriteContext::File { pfs, .. }) = self.write_lock.get_mut() { + pfs.discard(); + } return; } if let Ok(mut ctx) = self.write_lock.lock() { @@ -759,7 +770,18 @@ impl Minigraf { // second handle in this one — #304/#314). The guard still matters where // the filesystem cannot lock (e.g. NFSv3 without lockd, some FUSE // mounts) and the caller set `allow_unlocked`. + // + // A WAL can still exist here with every entry already in the file + // (a crash between a checkpoint's commit and its WAL delete, or a + // failed delete): replay counts only the entries it applies. Each + // of those entries is at or below the committed tx count, so + // removing the file loses nothing (#457). if *wal_entry_count == 0 && !pfs.is_dirty() { + let wal_path = Self::wal_path_for(db_path); + if wal_path.exists() { + *wal = None; + WalWriter::delete_file(&wal_path)?; + } return Ok(()); } // `force_dirty` is needed for the WAL-replay case: facts were loaded @@ -1879,6 +1901,19 @@ mod tests { // ── file-backed: sync mode is plumbed through to both WalWriter call sites ── + /// #476: an in-memory handle under the `usize::MAX` sentinel closes + /// without a checkpoint and without touching any file storage. + #[test] + fn test_max_threshold_in_memory_drop() { + let db = Minigraf::in_memory_with_options( + OpenOptions::new().wal_checkpoint_threshold(usize::MAX), + ) + .unwrap(); + db.execute("(transact [[:e0 :a0 1]])").unwrap(); + assert_eq!(db.current_tx_count(), 1, "write applied"); + drop(db); + } + #[test] fn test_normal_sync_mode_reaches_both_wal_writer_call_sites() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/storage/persistent_facts.rs b/src/storage/persistent_facts.rs index 4fdc4a333..47693af24 100644 --- a/src/storage/persistent_facts.rs +++ b/src/storage/persistent_facts.rs @@ -1084,6 +1084,14 @@ impl PersistentFactStorage { self.dirty } + /// Forget unsaved changes, so dropping this storage commits nothing. Used + /// on close under `wal_checkpoint_threshold(usize::MAX)`, whose WAL keeps + /// the pending facts (#476). + #[cfg(not(target_arch = "wasm32"))] + pub(crate) fn discard(&mut self) { + self.dirty = false; + } + /// Run a closure with read access to the underlying storage backend. /// /// Used by the browser WASM layer to read pages after `save()` without diff --git a/tests/golden_corpus_test.rs b/tests/golden_corpus_test.rs index b3991f2ed..069ead2bb 100644 --- a/tests/golden_corpus_test.rs +++ b/tests/golden_corpus_test.rs @@ -171,9 +171,9 @@ fn check_manifest(m: &Manifest, failures: &mut Vec) { db.checkpoint().unwrap(); } - // 4. Persist: reopen after the checkpoint. v2.x leaves a WAL whose entries - // were all checkpointed already (`v7_stale_wal`) in place; v3 removes it. - if READER >= 3 && wal_path(&path).exists() { + // 4. Persist: reopen after the checkpoint, which removes the WAL, including + // one whose entries are all checkpointed already (`v7_stale_wal`, #457). + if wal_path(&path).exists() { failures.push(format!("{}: WAL still present after checkpoint", m.name)); } { diff --git a/tests/wal_test.rs b/tests/wal_test.rs index eaef5167a..937ef2885 100644 --- a/tests/wal_test.rs +++ b/tests/wal_test.rs @@ -205,6 +205,53 @@ fn test_reopen_with_checkpointed_wal_does_not_reuse_tx_counts() { } } +/// #457: a checkpoint with nothing to flush still removes a WAL whose entries +/// are all checkpointed already. Before the fix it stayed until a later write +/// was checkpointed. +#[test] +fn test_checkpoint_removes_wal_whose_entries_are_all_checkpointed() { + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("stale.graph"); + let wal_path = wal_path_for(&db_path); + { + // usize::MAX: no close-time checkpoint, so the restored WAL survives drop. + let db = OpenOptions::default() + .wal_checkpoint_threshold(usize::MAX) + .path(db_path.to_str().unwrap()) + .open() + .unwrap(); + db.execute("(transact [[:alice :age 30]])").unwrap(); + let stale = std::fs::read(&wal_path).unwrap(); + db.checkpoint().unwrap(); + // Crash between the checkpoint's commit and the WAL delete. + std::fs::write(&wal_path, &stale).unwrap(); + } + { + let db = OpenOptions::default() + .wal_checkpoint_threshold(usize::MAX) + .path(db_path.to_str().unwrap()) + .open() + .unwrap(); + assert!(wal_path.exists(), "stale WAL present after open"); + db.checkpoint().unwrap(); + assert!(!wal_path.exists(), "stale WAL removed by checkpoint"); + // A second checkpoint with no WAL at all is still a no-op. + db.checkpoint().unwrap(); + } + let db = Minigraf::open(&db_path).unwrap(); + let n = count_results( + db.execute("(query [:find ?a :where [:alice :age ?a]])") + .unwrap(), + ); + assert_eq!(n, 1, "fact kept after the stale WAL is removed"); + db.execute("(transact [[:alice :age 31]])").unwrap(); + let n = count_results( + db.execute("(query [:find ?a :as-of 1 :where [:alice :age ?a]])") + .unwrap(), + ); + assert_eq!(n, 1, "the next write does not reuse tx 1"); +} + // ── 4. Partial WAL entry is discarded; earlier entries intact ───────────────── /// Write 1 fact, crash (no checkpoint), then append garbage bytes to the WAL @@ -892,3 +939,51 @@ fn test_v2_file_opens_and_upgrades_to_v3_on_checkpoint() { "last_checkpointed_tx_count must be set after checkpoint on v2→v6 upgrade" ); } + +/// #476: `wal_checkpoint_threshold(usize::MAX)` suppresses the close-time save +/// as well as the checkpoint. Before the fix, dropping the handle still wrote +/// every pending fact to the `.graph` file and left a fully checkpointed WAL. +#[test] +fn test_max_threshold_drop_leaves_graph_file_untouched() { + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("nosave.graph"); + let wal_path = wal_path_for(&db_path); + { + let db = Minigraf::open(&db_path).unwrap(); + db.execute("(transact [[:e0 :a0 0]])").unwrap(); + } + assert!(!wal_path.exists(), "default close checkpoints"); + let before = std::fs::read(&db_path).unwrap(); + for sync in [minigraf::SyncMode::Full, minigraf::SyncMode::Normal] { + let db = OpenOptions::new() + .synchronous(sync) + .wal_checkpoint_threshold(usize::MAX) + .path(&db_path) + .open() + .unwrap(); + db.execute("(transact [[:e0 :a0 1]])").unwrap(); + db.execute("(transact [[:e0 :a0 2]])").unwrap(); + drop(db); + assert!( + std::fs::read(&db_path).unwrap() == before, + ".graph file unchanged by drop" + ); + assert!(wal_path.exists(), "WAL kept by drop"); + std::fs::remove_file(&wal_path).unwrap(); + } + // The WAL alone carries the writes: keep one, reopen, and they replay. + { + let db = OpenOptions::new() + .wal_checkpoint_threshold(usize::MAX) + .path(&db_path) + .open() + .unwrap(); + db.execute("(transact [[:e0 :a0 1]])").unwrap(); + } + let db = Minigraf::open(&db_path).unwrap(); + assert_eq!(db.current_tx_count(), 2, "WAL entry replayed on reopen"); + let r = db + .execute("(query [:find ?v :where [:e0 :a0 ?v]])") + .unwrap(); + assert_eq!(count_results(r), 2, "both values visible after replay"); +} From a861e551162623c8f4752f94daf5b637543f8358 Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Sun, 11 Oct 2026 13:42:56 +0530 Subject: [PATCH 3/7] release: v2.0.5 Doc sync (CHANGELOG, CLAUDE.md, ROADMAP, README, TEST_COVERAGE) and version bump to 2.0.5. v2.x patch: a sub-page file is refused with STG-045 unless it is a torn first write (#506); a commit after a torn WAL entry is no longer lost, failed appends poison the handle with WAL-007 (#513); backports of #528, #457 and #476 from v3; weekly soak test (#392) and working coverage gates (#396). File format v7 unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h --- CHANGELOG.md | 8 +++++++- CLAUDE.md | 2 +- Cargo.toml | 2 +- README.md | 2 +- ROADMAP.md | 15 ++++++++++++++- docs/TEST_COVERAGE.md | 10 +++++----- 6 files changed, 29 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21fdc3ac8..ac7d40c71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,9 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## Unreleased +## v2.0.5 — 2026-10-11 + +Patch release on the v2.x line under the [support policy](PHILOSOPHY.md#support-policy): two data-integrity fixes (#506, #513) and three bug fixes backported from v3.0.0 (#528, #457, #476), plus a weekly soak test and working coverage gates. File format is unchanged (v7). Two new error codes, `STG-045` and `WAL-007`, numbered as in v3.0.0; no other API changes. Upgrading is recommended for every v2.x user. ### Fixed @@ -20,6 +22,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Weekly soak test at scale (#392).** `tests/soak_test.rs` grows a file-backed database to 10M live facts in batches of 1–2,000 facts, then churns it for the rest of a 5-hour budget at 50 transactions a second (retracts, re-asserts, long histories on 32 hot entities), with a checkpoint every 20–200 transactions and a reopen every 2–10 minutes. Samples are checked against a reference through EAVT, entity and attribute, AVET, AEVT, `:as-of` and the transaction counter, with a full scan in a child process at 2M facts. RSS after a reopen is bounded relative to the fact versions held. Metrics go to a JSON-lines file that `.github/workflows/soak.yml` uploads weekly, alternating `main` (Saturday) and `v3` (Sunday). On v2.x no transaction writes two values of one entity and attribute, because of the known issue #371; the v3 workload fails here. It also checkpoints every 100–1,000 transactions and samples `:as-of` less, since both cost time in the database size on v2.x. `soak_short` runs the driver at 20K facts in the normal suite. - **Coverage gates that gate (#396).** The per-module gate in `coverage-gates.yml` had never checked anything: the instrumented test run failed (the lock tests could not find `pid_ns_helper`), the report it grepped was empty, and every module was skipped with a warning, so the job passed. It also read `cargo llvm-cov --text`, which has no per-file percentages. The job now runs the whole suite with branch coverage on a pinned nightly, fails if any test fails, and checks each file's line and branch percentage against `coverage-thresholds.toml` with `scripts/coverage_gates.py`; a file missing from the report, or without branch data, fails. The floors are the measured values less one point, for the WAL, `db.rs`, `graph/storage.rs`, the storage files (except the v5 B+tree kept for migration) and the evaluator, executor and stratification. New tests check that WAL replay stops at an entry whose first fact is over 10 MB (the old test changed bytes inside the fact and never reached the limit) or that claims more than 1M facts, that a WAL writer cannot be opened in a missing directory, and that two threads missing the same page in the page cache get the one cached copy (before, only a lucky race reached that path). +### Known issues + +- Same-transaction multi-valued facts can read back as one value (#371, #287), a fact retracted and re-asserted in one `WriteTransaction` is lost at commit (#477), a later assertion does not replace an earlier valid-time window (#435), `save()` is not crash-atomic (#374), and a `.wal` from a later checkpoint is replayed onto an older `.graph` (#535); all are fixed in v3.0.0 + ## v2.0.4 — 2026-10-08 Patch release on the v2.x line under the [support policy](PHILOSOPHY.md#support-policy): one `BrowserDb` durability hardening fix, plus new compatibility and crash tests. File format is unchanged (v7). No API changes. Native and WASI builds behave exactly as in v2.0.3. diff --git a/CLAUDE.md b/CLAUDE.md index a00a92265..66de66d99 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -181,7 +181,7 @@ See `docs/TEST_COVERAGE.md` for the full per-file breakdown. ## Current Maintainer Context -**v2.0.4 released** (2026-10-08) — `BrowserDb` IndexedDB flush returns an error for an unreadable dirty page (#470); golden-file v7 corpus (#391) and SIGKILL committed-data crash test (#384). **v2.0.3** (2026-10-06) — data-integrity patch under the support policy: indexed queries with >65,535 uncheckpointed facts (#445), tx counter rewound on reopen next to an already-checkpointed WAL (#447). **v2.0.2** (2026-09-27) — last planned v2.x feature/fix release: query fixes (#297, #405, #407, #410), directory fsync (#389), point-query/attribute-scan/checkpoint perf mitigations (#323, #381, #315), CI policy checks (#395), support policy and tiers (#397, #399, #400). **v2.0.1** (2026-09-25) fixed index rebuild on open (#370). **v2.0.0** introduced kernel file locking (#317, #304), structured runtime error codes (#277), `OpenOptions` `#[non_exhaustive]`, and an MSRV of Rust 1.89. See `CHANGELOG.md` for the full rationale and release history. +**v2.0.5 released** (2026-10-11) — short-file open refused with STG-045 (#506), commit after a torn WAL entry no longer lost, WAL-007 (#513); backported rule-query `max_results` (#528), all-checkpointed WAL removal (#457) and `usize::MAX` close-time save (#476); weekly soak test (#392), real coverage gates (#396). **v2.0.4** (2026-10-08) — `BrowserDb` IndexedDB flush returns an error for an unreadable dirty page (#470); golden-file v7 corpus (#391) and SIGKILL committed-data crash test (#384). **v2.0.3** (2026-10-06) — data-integrity patch under the support policy: indexed queries with >65,535 uncheckpointed facts (#445), tx counter rewound on reopen next to an already-checkpointed WAL (#447). **v2.0.2** (2026-09-27) — last planned v2.x feature/fix release: query fixes (#297, #405, #407, #410), directory fsync (#389), point-query/attribute-scan/checkpoint perf mitigations (#323, #381, #315), CI policy checks (#395), support policy and tiers (#397, #399, #400). **v2.0.1** (2026-09-25) fixed index rebuild on open (#370). **v2.0.0** introduced kernel file locking (#317, #304), structured runtime error codes (#277), `OpenOptions` `#[non_exhaustive]`, and an MSRV of Rust 1.89. See `CHANGELOG.md` for the full rationale and release history. Current plan (see `ROADMAP.md` and the milestones): - **v2.x** (`main`, format v7) now gets only data-integrity and security fixes as patch releases (v2.0.3 is the first); v2.0.2 was the last planned v2.x release. diff --git a/Cargo.toml b/Cargo.toml index af46a4baa..115ebe30f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,7 +5,7 @@ resolver = "2" [package] name = "minigraf" -version = "2.0.4" +version = "2.0.5" edition = "2024" rust-version = "1.89" description = "Zero-config, single-file, embedded graph database with bi-temporal Datalog queries" diff --git a/README.md b/README.md index 97b88b98f..a952b4310 100644 --- a/README.md +++ b/README.md @@ -44,7 +44,7 @@ Minigraf is a **single-file embedded graph database** that lets you: ```toml [dependencies] -minigraf = "2.0.4" +minigraf = "2.0.5" ``` Or via cargo: diff --git a/ROADMAP.md b/ROADMAP.md index 8c214b10a..6bb22f1be 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -8,6 +8,19 @@ Completed releases and their implementation details live in the [CHANGELOG](CHAN --- +## v2.0.5 — Data-Integrity Patch (released 2026-10-11) + +File format stays v7. Two new error codes (`STG-045`, `WAL-007`); no other API changes. Details are in the [CHANGELOG](CHANGELOG.md). + +Shipped: + +- A file shorter than one page is refused with `STG-045` unless it is a torn first write (#506) +- A commit made after a torn WAL entry is no longer lost on recovery; a failed append makes later writes `WAL-007` until reopen or checkpoint (#513) +- Backported from v3.0.0: rule queries count only derived facts against `max_results` (#528), `checkpoint()` removes an all-checkpointed WAL (#457), `usize::MAX` checkpoint threshold skips the close-time save (#476) +- Weekly 10M-fact soak test (#392) and per-file coverage gates that actually gate (#396) + +--- + ## v2.0.4 — Browser Durability Patch (released 2026-10-08) File format stays v7 and the public API does not change. Details are in the [CHANGELOG](CHANGELOG.md). @@ -192,4 +205,4 @@ See [GitHub Issues](https://github.com/project-minigraf/minigraf/issues) for spe --- -**Last updated**: October 2026 — v2.0.4, a browser durability patch on v2.x, is the current release; v3.0.0 work, including the file-format v8 fix for #287/#371, is developed on the long-lived `v3` branch and merges into `main` at the v3.0.0 cut. +**Last updated**: October 2026 — v2.0.5, a data-integrity patch on v2.x, is the current release; v3.0.0 work, including the file-format v8 fix for #287/#371, is developed on the long-lived `v3` branch and merges into `main` at the v3.0.0 cut. diff --git a/docs/TEST_COVERAGE.md b/docs/TEST_COVERAGE.md index dc7d87da8..89ebe1621 100644 --- a/docs/TEST_COVERAGE.md +++ b/docs/TEST_COVERAGE.md @@ -1,14 +1,14 @@ # Minigraf Test Coverage -## v2.0.4 Coverage Summary +## v2.0.5 Coverage Summary -**Verified**: 2026-10-10 with `cargo test` +**Verified**: 2026-10-11 with `cargo test` **Result**: 1,232 passing tests and 9 ignored tests (1,241 total), including the golden-file corpus (#391) -Since v2.0.4, `tests/edge_cases_test.rs` checks that a file of 1 to 4,095 bytes is `STG-045`, unchanged and with no WAL, when it is a database cut below one page or foreign bytes, and opens as a new database that takes a write and reads it back after reopen when it is a prefix of the initial header or all zeros (#506). +For v2.0.5, `tests/edge_cases_test.rs` checks that a file of 1 to 4,095 bytes is `STG-045`, unchanged and with no WAL, when it is a database cut below one page or foreign bytes, and opens as a new database that takes a write and reads it back after reopen when it is a prefix of the initial header or all zeros (#506). -Since v2.0.4, torn-WAL-tail tests (#513) check that opening a WAL writer cuts off a partial last entry (cut in the checksum, the header or the payload) or an entry with a bad checksum and everything after it, then appends where replay reaches, and leaves an intact WAL byte for byte; that an append that fails part-way makes every later append `WAL-007` while a rejected oversize fact (`WAL-003`, nothing written) does not; through `Minigraf`, that the failed transaction is not applied, later `transact` and `commit` are `WAL-007`, and a `checkpoint()` makes writes work again; and, in `tests/wal_test.rs`, that a commit made after reopening a crash-left WAL with each kind of torn tail survives a second crash. +For v2.0.5, torn-WAL-tail tests (#513) check that opening a WAL writer cuts off a partial last entry (cut in the checksum, the header or the payload) or an entry with a bad checksum and everything after it, then appends where replay reaches, and leaves an intact WAL byte for byte; that an append that fails part-way makes every later append `WAL-007` while a rejected oversize fact (`WAL-003`, nothing written) does not; through `Minigraf`, that the failed transaction is not applied, later `transact` and `commit` are `WAL-007`, and a `checkpoint()` makes writes work again; and, in `tests/wal_test.rs`, that a commit made after reopening a crash-left WAL with each kind of torn tail survives a second crash. `tests/property_test.rs` writes random histories (batched transacts, retracts, write transactions, checkpoints) in memory and to a file, and compares random queries (one to three joined patterns, `not` or `not-join`, a comparison predicate, `:as-of`, `:valid-at` or `:any-valid-time`) with a reference evaluator, on the open file and after a reopen; a query error fails the test (#386). The histories leave out the v2.x known issues #371, #435 and #477. @@ -16,7 +16,7 @@ Since v2.0.4, torn-WAL-tail tests (#513) check that opening a WAL writer cuts of Not counted above: the `BrowserDb` tests run only on wasm32 with `wasm-bindgen-test` in a browser. Two of them check that the IndexedDB flush returns every page a write dirtied, and that a dirty page that cannot be read is an error rather than left out. -The v2.0.4 suite covers the public database API, the Datalog engine, storage and recovery paths, and the v2.0.0 locking and structured-error-code changes, plus v2.0.1's regression test for rebuilding indexes on open after several checkpoints (#370). Since v2.0.1, `tests/point_query_history_test.rs` and new storage unit tests check that attribute-narrowed bound-entity lookups return exactly what a full scan returns, and that `net_asserted_facts` matches its previous implementation on randomized histories (#323). Attribute-scan tests check that the AEVT range covers exactly the queried attribute, including non-ASCII names such as `:丿` and prefix siblings such as `:ab` for `:a` (#381). Incremental B+tree rebuild tests check that checkpoints which copy untouched index leaves produce exactly the entries and file checksum a full rebuild would, including over repeated checkpoints, that single-fact checkpoints do not fragment the index, and that a cyclic leaf chain or damaged slot directory is rejected rather than copied (#315). Directory-sync unit tests check that creating the `.graph` file or the WAL, and deleting the WAL, fsync the parent directory after the file operation, and that reopening an existing file does not (#389). Disjunction tests check that `or-join` returns an empty result, not INT-031, when no rows reach it: on an empty database, after a pattern that matches nothing, after a `not` that removes every row, and in a rule body (#405). A stratification unit test builds the same two-rule negative cycle 64 times and checks that the INT-054 message names the same predicate pair every time (#410). Magic-sets tests check that a recursive rule with a literal start that recurses through another rule returns the same results as without the literal (#297). API tests check that `execute()` and `WriteTransaction::execute()` reject `$slot` bind tokens in every position with API-010 (#407). The REPL `run()` test now runs in a child process with null stdin, so it no longer hangs when the test runner's stdin is an open non-TTY stream (#415). A fault-injection test crashes `save()` at each of its 47 page writes and syncs and checks that every reopen finds exactly the old or the new checkpointed facts (#374). For v2.0.3, storage unit tests and two `edge_cases_test.rs` tests check that more than 65,535 uncheckpointed facts each resolve to their own fact through entity lookups and attribute scans, in memory and file-backed, before and after a checkpoint, and that the widened `FactRef` encodes to the same bytes as before (#445). A WAL test reopens a database next to a WAL whose entries are all checkpointed, writes again, and checks that `:as-of` history is unchanged before and after another reopen; a unit test checks that restoring the transaction counter never lowers it (#447). Since v2.0.3, `tests/golden_corpus_test.rs` opens a copy of each committed golden file in `tests/golden/` (v7 files written by minigraf 2.0.3: one checkpoint, several checkpoints, indexes rebuilt on open, a crash-left WAL, an already-checkpointed WAL, same-transaction multi-values) and checks its manifest's CRC32, `tx_count` and query results after open, after a checkpoint and reopen, and after one more write; a second test checks that every golden file has a manifest (#391). A recursive-rules test runs rule queries on a database with more live facts than `max_results` and checks that they answer, and that the limit still stops a rule that derives more facts than it allows (#528). A WAL test restores a WAL whose entries are all checkpointed and checks that an explicit `checkpoint()` removes it, keeps the fact, and that the next write does not reuse tx 1; the golden-corpus test now requires `v7_stale_wal` to have no WAL after a checkpoint too (#457). A WAL test checks that dropping a handle opened with `wal_checkpoint_threshold(usize::MAX)` leaves the `.graph` file byte for byte unchanged under `Full` and `Normal` sync, keeps the WAL, and that a reopen replays it (#476). +The v2.0.5 suite covers the public database API, the Datalog engine, storage and recovery paths, and the v2.0.0 locking and structured-error-code changes, plus v2.0.1's regression test for rebuilding indexes on open after several checkpoints (#370). Since v2.0.1, `tests/point_query_history_test.rs` and new storage unit tests check that attribute-narrowed bound-entity lookups return exactly what a full scan returns, and that `net_asserted_facts` matches its previous implementation on randomized histories (#323). Attribute-scan tests check that the AEVT range covers exactly the queried attribute, including non-ASCII names such as `:丿` and prefix siblings such as `:ab` for `:a` (#381). Incremental B+tree rebuild tests check that checkpoints which copy untouched index leaves produce exactly the entries and file checksum a full rebuild would, including over repeated checkpoints, that single-fact checkpoints do not fragment the index, and that a cyclic leaf chain or damaged slot directory is rejected rather than copied (#315). Directory-sync unit tests check that creating the `.graph` file or the WAL, and deleting the WAL, fsync the parent directory after the file operation, and that reopening an existing file does not (#389). Disjunction tests check that `or-join` returns an empty result, not INT-031, when no rows reach it: on an empty database, after a pattern that matches nothing, after a `not` that removes every row, and in a rule body (#405). A stratification unit test builds the same two-rule negative cycle 64 times and checks that the INT-054 message names the same predicate pair every time (#410). Magic-sets tests check that a recursive rule with a literal start that recurses through another rule returns the same results as without the literal (#297). API tests check that `execute()` and `WriteTransaction::execute()` reject `$slot` bind tokens in every position with API-010 (#407). The REPL `run()` test now runs in a child process with null stdin, so it no longer hangs when the test runner's stdin is an open non-TTY stream (#415). A fault-injection test crashes `save()` at each of its 47 page writes and syncs and checks that every reopen finds exactly the old or the new checkpointed facts (#374). For v2.0.3, storage unit tests and two `edge_cases_test.rs` tests check that more than 65,535 uncheckpointed facts each resolve to their own fact through entity lookups and attribute scans, in memory and file-backed, before and after a checkpoint, and that the widened `FactRef` encodes to the same bytes as before (#445). A WAL test reopens a database next to a WAL whose entries are all checkpointed, writes again, and checks that `:as-of` history is unchanged before and after another reopen; a unit test checks that restoring the transaction counter never lowers it (#447). Since v2.0.3, `tests/golden_corpus_test.rs` opens a copy of each committed golden file in `tests/golden/` (v7 files written by minigraf 2.0.3: one checkpoint, several checkpoints, indexes rebuilt on open, a crash-left WAL, an already-checkpointed WAL, same-transaction multi-values) and checks its manifest's CRC32, `tx_count` and query results after open, after a checkpoint and reopen, and after one more write; a second test checks that every golden file has a manifest (#391). A recursive-rules test runs rule queries on a database with more live facts than `max_results` and checks that they answer, and that the limit still stops a rule that derives more facts than it allows (#528). A WAL test restores a WAL whose entries are all checkpointed and checks that an explicit `checkpoint()` removes it, keeps the fact, and that the next write does not reuse tx 1; the golden-corpus test now requires `v7_stale_wal` to have no WAL after a checkpoint too (#457). A WAL test checks that dropping a handle opened with `wal_checkpoint_threshold(usize::MAX)` leaves the `.graph` file byte for byte unchanged under `Full` and `Normal` sync, keeps the WAL, and that a reopen replays it (#476). ### Covered Areas From 591a45f67d55e2aab03187ad480cb5b516024c0b Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Sun, 11 Oct 2026 23:25:46 +0530 Subject: [PATCH 4/7] fix: queries see whole transactions and one state per query (#554, #555) 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 Claude-Session: https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h --- CHANGELOG.md | 11 + CLAUDE.md | 2 +- README.md | 2 +- docs/TEST_COVERAGE.md | 4 +- src/browser/mod.rs | 11 +- src/db.rs | 21 +- src/graph/storage.rs | 449 ++++++++++++++++++++----------- src/query/datalog/executor.rs | 22 +- tests/snapshot_atomicity_test.rs | 168 ++++++++++++ 9 files changed, 495 insertions(+), 195 deletions(-) create mode 100644 tests/snapshot_atomicity_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ac7d40c71..adbc6c53f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed + +- **A query on another thread sees each transaction whole (#554).** A commit wrote its WAL entry, then added the transaction's facts to the in-memory store one fact at a time, each under its own lock, so a query running on another thread could return part of the transaction. `execute("(transact ...)")`, `execute("(retract ...)")` and `WriteTransaction::commit` all did this. They now add the whole transaction under one lock, as does WAL replay. Stored data was never affected. Affected `WriteTransaction` since v0.5.0 and `execute` since v0.6.0. Also fixed in v3.0.0. +- **All patterns of one query read the same state of the database (#555).** A query whose patterns name entities or attributes read each of them with its own lookup, each under its own lock, so a transaction that committed between two lookups was seen by some patterns and not others, and the query could return rows that match no state of the database. The lookups now run under one lock. Stored data was never affected. Affected every release since v1.1.0. Also fixed in v3.0.0. + +### Tests + +- `tests/snapshot_atomicity_test.rs` races a writer against a reader: a query must see each transaction whole, whether written with `execute` or a `WriteTransaction` (#554), and must read one state for all its patterns (#555), in memory and on file. + ## v2.0.5 — 2026-10-11 Patch release on the v2.x line under the [support policy](PHILOSOPHY.md#support-policy): two data-integrity fixes (#506, #513) and three bug fixes backported from v3.0.0 (#528, #457, #476), plus a weekly soak test and working coverage gates. File format is unchanged (v7). Two new error codes, `STG-045` and `WAL-007`, numbered as in v3.0.0; no other API changes. Upgrading is recommended for every v2.x user. diff --git a/CLAUDE.md b/CLAUDE.md index 66de66d99..8abf03d7e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -174,7 +174,7 @@ Auto-migrates v1/v2/v3/v4/v5/v6 → v7 on open/checkpoint. ## Test Coverage -**1241 tests** (1232 passing, 9 ignored; unit + integration + doc). +**1249 tests** (1240 passing, 9 ignored; unit + integration + doc). See `docs/TEST_COVERAGE.md` for the full per-file breakdown. **Testing conventions** — see the Testing Conventions section below before writing any tests. diff --git a/README.md b/README.md index a952b4310..45c1e7d79 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,7 @@ let r2 = pq.execute(&[("tx", BindValue::TxCount(2)), ("entity", BindValue::Entit ```bash cargo run # interactive Datalog REPL -cargo test # run 1241 tests +cargo test # run 1249 tests cargo run < demos/demo_recursive.txt # recursive rules demo ``` diff --git a/docs/TEST_COVERAGE.md b/docs/TEST_COVERAGE.md index 89ebe1621..28383b99f 100644 --- a/docs/TEST_COVERAGE.md +++ b/docs/TEST_COVERAGE.md @@ -4,7 +4,7 @@ **Verified**: 2026-10-11 with `cargo test` -**Result**: 1,232 passing tests and 9 ignored tests (1,241 total), including the golden-file corpus (#391) +**Result**: 1,240 passing tests and 9 ignored tests (1,249 total), including the golden-file corpus (#391) For v2.0.5, `tests/edge_cases_test.rs` checks that a file of 1 to 4,095 bytes is `STG-045`, unchanged and with no WAL, when it is a database cut below one page or foreign bytes, and opens as a new database that takes a write and reads it back after reopen when it is a prefix of the initial header or all zeros (#506). @@ -22,7 +22,7 @@ The v2.0.5 suite covers the public database API, the Datalog engine, storage and - **Core database behavior**: in-memory and file-backed operation, transactions, checkpoints, retractions, bi-temporal queries, recursive rules, prepared statements, aggregation, expressions, disjunction, window functions, UDFs, and magic-sets evaluation. - **Storage correctness**: packed pages, B+tree indexes and range scans, cache behavior, file-header validation, migration, WAL replay, checksums, corruption handling, and fault injection. -- **Reliability and concurrency**: concurrent reads and writes, rollback behavior, same-process handle exclusion, cross-process locking, PID namespaces, NFS and container lock behavior, and SIGKILL recovery. +- **Reliability and concurrency**: concurrent reads and writes, rollback behavior, same-process handle exclusion, cross-process locking, PID namespaces, NFS and container lock behavior, and SIGKILL recovery, and queries seeing each concurrent transaction whole and reading one state for all their patterns (`tests/snapshot_atomicity_test.rs`, #554, #555). - **Public diagnostics**: structured parser, query, storage, WAL, API, and internal error codes; the error-code registry is checked against [`ERROR_REFERENCE.md`](ERROR_REFERENCE.md). - **Compatibility and quality**: golden-file corpus of released file formats, cross-platform format compatibility, property-based tests, long-haul smoke coverage, a weekly soak at 10M facts with a 5-hour churn (#392), XTDB/Datomic semantic compatibility, grammar conformance, and rustdoc examples. diff --git a/src/browser/mod.rs b/src/browser/mod.rs index 66f27b32f..a7d80ca3b 100644 --- a/src/browser/mod.rs +++ b/src/browser/mod.rs @@ -267,12 +267,11 @@ impl BrowserDb { }) .collect(); - for fact in &stamped { - inner - .fact_storage - .load_fact(fact.clone()) - .map_err(|e| JsValue::from_str(&e.to_string()))?; - } + // One batch, so no query sees part of the transaction (#554). + inner + .fact_storage + .load_facts(stamped.iter().cloned()) + .map_err(|e| JsValue::from_str(&e.to_string()))?; inner.pfs.mark_dirty(); inner diff --git a/src/db.rs b/src/db.rs index b2d43d2cf..66059e09e 100644 --- a/src/db.rs +++ b/src/db.rs @@ -519,9 +519,7 @@ impl Minigraf { // Already present in the main file; skip. continue; } - for fact in &entry.facts { - let _ = fact_storage.load_fact(fact.clone())?; - } + let _ = fact_storage.load_facts(entry.facts.iter().cloned())?; #[allow(clippy::arithmetic_side_effects)] { replayed += 1; @@ -637,10 +635,12 @@ impl Minigraf { &stamped, )?; - // WAL succeeded — now apply facts to shared FactStorage. - for fact in &stamped { - let _ = self.inner.fact_storage.load_fact(fact.clone())?; - } + // WAL succeeded — now apply facts to shared FactStorage, in one + // batch so no query sees part of the transaction (#554). + let _ = self + .inner + .fact_storage + .load_facts(stamped.iter().cloned())?; // Trigger auto-checkpoint AFTER facts are in FactStorage so the // checkpoint captures the newly written facts. @@ -1249,10 +1249,9 @@ impl<'a> WriteTransaction<'a> { &stamped, )?; - // WAL succeeded — now apply facts to shared FactStorage. - for fact in stamped { - let _ = self.inner.fact_storage.load_fact(fact)?; - } + // WAL succeeded — now apply facts to shared FactStorage, in one + // batch so no query sees part of the transaction (#554). + let _ = self.inner.fact_storage.load_facts(stamped)?; // Trigger auto-checkpoint AFTER facts are in FactStorage so the // checkpoint captures the newly written facts. diff --git a/src/graph/storage.rs b/src/graph/storage.rs index 1edaa6354..21727c943 100644 --- a/src/graph/storage.rs +++ b/src/graph/storage.rs @@ -294,6 +294,34 @@ impl FactStorage { Ok(true) } + /// Insert facts with their tx_id and tx_count preserved, all under one + /// write lock, so a query sees either none of them or all of them (#554). + /// + /// Commits and WAL replay use this for a whole transaction. Duplicates are + /// skipped as in [`Self::load_fact`]. Returns how many facts were loaded. + pub(crate) fn load_facts(&self, facts: impl IntoIterator) -> Result { + let mut d = self + .data + .write() + .map_err(|_| err_coded!(ErrorCode::Int050, "data"))?; + // Pick the new facts and their slots before changing anything, so a + // slot past `u32::MAX` fails the whole batch, never part of it. + let mut keys = HashSet::new(); + let new: Vec<(PendingKey, Fact)> = facts + .into_iter() + .map(|f| (pending_key(&f), f)) + .filter(|(k, _)| !d.pending_keys.contains(k) && keys.insert(k.clone())) + .collect(); + let refs = pending_refs(d.facts.len(), new.len())?; + let loaded = new.len(); + for ((key, fact), fact_ref) in new.into_iter().zip(refs) { + d.pending_keys.insert(key); + d.pending_indexes.insert(&fact, fact_ref); + d.facts.push(fact); + } + Ok(loaded) + } + /// Raise tx_counter to max(tx_count) across all loaded facts. /// /// Must be called after all `load_fact()` calls complete so that the next @@ -337,14 +365,7 @@ impl FactStorage { .data .read() .map_err(|_| err_coded!(ErrorCode::Int050, "data"))?; - let mut all = Vec::new(); - // Committed facts first (on disk, via CommittedFactReader) - if let Some(loader) = &d.committed { - all.extend(loader.stream_all()?); - } - // Then pending facts (post-checkpoint, in memory) - all.extend(d.facts.iter().cloned()); - Ok(all) + all_facts(&d) } /// Return all facts visible as of the given transaction point. @@ -596,74 +617,13 @@ fn resolve_fact_ref(d: &FactData, fr: FactRef) -> Result { /// Production helpers on FactStorage: index-driven entity/attribute lookups used by the query executor. impl FactStorage { + #[cfg(test)] /// Get all facts for a specific entity (index-driven). pub(crate) fn get_facts_by_entity(&self, entity_id: &EntityId) -> Result> { - use crate::storage::index::EavtKey; - let d = self.data.read().unwrap_or_else(|e| e.into_inner()); - - let start = EavtKey { - entity: *entity_id, - attribute: String::new(), - valid_from: i64::MIN, - valid_to: i64::MIN, - tx_count: 0, - }; - let next_entity = uuid::Uuid::from_u128(entity_id.as_u128().wrapping_add(1)); - let end = EavtKey { - entity: next_entity, - attribute: String::new(), - valid_from: i64::MIN, - valid_to: i64::MIN, - tx_count: 0, - }; - - // Fallback: no indexes built yet - if d.pending_indexes.eavt.is_empty() && d.committed_index_reader.is_none() { - if d.committed.is_none() { - return Ok(d - .facts - .iter() - .filter(|f| &f.entity == entity_id) - .cloned() - .collect()); - } - let mut result: Vec = d - .facts - .iter() - .filter(|f| &f.entity == entity_id) - .cloned() - .collect(); - if let Some(loader) = &d.committed { - for fact in loader.stream_all()? { - if &fact.entity == entity_id { - result.push(fact); - } - } - } - return Ok(result); - } - - let mut facts = Vec::new(); - - // Pending: in-memory BTreeMap bounded range. - for (key, &fr) in d.pending_indexes.eavt.range(start.clone()..end.clone()) { - if key.entity != *entity_id { - break; - } - facts.push(resolve_fact_ref(&d, fr)?); - } - - // Committed: on-disk B+tree range scan - if let Some(reader) = &d.committed_index_reader { - let committed_refs = reader.range_scan_eavt(&start, Some(&end))?; - for fr in committed_refs { - facts.push(resolve_fact_ref(&d, fr)?); - } - } - - Ok(facts) + self.get_facts_for_lookups(&[Lookup::Entity(entity_id)]) } + #[cfg(test)] /// Get every stored record for one `(entity, attribute)` pair (index-driven, #323). /// /// Range-scans EAVT over exactly `[(e, a, …), (e, a + "\0", …))`, so other @@ -674,116 +634,237 @@ impl FactStorage { entity_id: &EntityId, attribute: &Attribute, ) -> Result> { - use crate::storage::index::EavtKey; - let d = self.data.read().unwrap_or_else(|e| e.into_inner()); - let matches = |f: &Fact| &f.entity == entity_id && &f.attribute == attribute; + self.get_facts_for_lookups(&[Lookup::EntityAttribute(entity_id, attribute)]) + } - // Fallback: no indexes built yet - if d.pending_indexes.eavt.is_empty() && d.committed_index_reader.is_none() { - let mut result: Vec = d.facts.iter().filter(|f| matches(f)).cloned().collect(); - if let Some(loader) = &d.committed { - result.extend(loader.stream_all()?.into_iter().filter(|f| matches(f))); - } - return Ok(result); + #[cfg(test)] + /// Get all facts for a specific attribute (index-driven). + pub(crate) fn get_facts_by_attribute(&self, attribute: &Attribute) -> Result> { + self.get_facts_for_lookups(&[Lookup::Attribute(attribute)]) + } + + /// The facts of several lookups, all read from one state of the store: no + /// commit lands between two of them (#555). Results are concatenated in + /// `lookups` order and not deduplicated. + pub(crate) fn get_facts_for_lookups(&self, lookups: &[Lookup<'_>]) -> Result> { + let d = self.data.read().unwrap_or_else(|e| e.into_inner()); + let mut facts = Vec::new(); + for lookup in lookups { + facts.extend(match *lookup { + Lookup::Entity(e) => entity_facts(&d, e)?, + Lookup::EntityAttribute(e, a) => entity_attribute_facts(&d, e, a)?, + Lookup::Attribute(a) => attribute_facts(&d, a)?, + }); } + Ok(facts) + } +} - let start = EavtKey { - entity: *entity_id, - attribute: attribute.clone(), - valid_from: i64::MIN, - valid_to: i64::MIN, - tx_count: 0, - }; - // Exact successor of `attribute`: the smallest string that sorts after it is - // `attribute + "\0"`, so `[start, end)` holds exactly this (e, a) pair. Always - // valid UTF-8 — unlike incrementing the last byte, which fails for attributes - // ending in 0x7F or a character whose last UTF-8 byte is 0xBF (e.g. `:丿`). - let end = EavtKey { - entity: *entity_id, - attribute: format!("{attribute}\0"), - valid_from: i64::MIN, - valid_to: i64::MIN, - tx_count: 0, - }; +/// One index lookup of [`FactStorage::get_facts_for_lookups`]. +#[derive(Clone, Copy)] +pub(crate) enum Lookup<'a> { + /// Every record of one entity. + Entity(&'a EntityId), + /// Every record of one `(entity, attribute)` pair. + EntityAttribute(&'a EntityId, &'a Attribute), + /// Every record with one attribute. + Attribute(&'a Attribute), +} - let mut facts = Vec::new(); +/// Committed facts, then pending facts. The caller holds `d`'s lock. +fn all_facts(d: &FactData) -> Result> { + let mut all = Vec::new(); + // Committed facts first (on disk, via CommittedFactReader) + if let Some(loader) = &d.committed { + all.extend(loader.stream_all()?); + } + // Then pending facts (post-checkpoint, in memory) + all.extend(d.facts.iter().cloned()); + Ok(all) +} - // Pending: EAVT keys for the exact (e, a) are contiguous from `start`; - // the first key with a different entity or attribute ends the run. - for (key, &fr) in d.pending_indexes.eavt.range(start.clone()..) { - if key.entity != *entity_id || key.attribute != *attribute { - break; - } - facts.push(resolve_fact_ref(&d, fr)?); +/// Get all facts for a specific entity (index-driven). +fn entity_facts(d: &FactData, entity_id: &EntityId) -> Result> { + use crate::storage::index::EavtKey; + + let start = EavtKey { + entity: *entity_id, + attribute: String::new(), + valid_from: i64::MIN, + valid_to: i64::MIN, + tx_count: 0, + }; + let next_entity = uuid::Uuid::from_u128(entity_id.as_u128().wrapping_add(1)); + let end = EavtKey { + entity: next_entity, + attribute: String::new(), + valid_from: i64::MIN, + valid_to: i64::MIN, + tx_count: 0, + }; + + // Fallback: no indexes built yet + if d.pending_indexes.eavt.is_empty() && d.committed_index_reader.is_none() { + if d.committed.is_none() { + return Ok(d + .facts + .iter() + .filter(|f| &f.entity == entity_id) + .cloned() + .collect()); } - - // Committed: on-disk B+tree range scan over exactly this (e, a) pair; the - // post-filter is a cheap guard, not something the range relies on. - if let Some(reader) = &d.committed_index_reader { - for fr in reader.range_scan_eavt(&start, Some(&end))? { - let fact = resolve_fact_ref(&d, fr)?; - if matches(&fact) { - facts.push(fact); + let mut result: Vec = d + .facts + .iter() + .filter(|f| &f.entity == entity_id) + .cloned() + .collect(); + if let Some(loader) = &d.committed { + for fact in loader.stream_all()? { + if &fact.entity == entity_id { + result.push(fact); } } } - - Ok(facts) + return Ok(result); } - /// Get all facts for a specific attribute (index-driven). - pub(crate) fn get_facts_by_attribute(&self, attribute: &Attribute) -> Result> { - use crate::storage::index::AevtKey; - let d = self.data.read().unwrap_or_else(|e| e.into_inner()); + let mut facts = Vec::new(); - // Fallback: no index - if d.pending_indexes.aevt.is_empty() && d.committed_index_reader.is_none() { - drop(d); - return Ok(self - .get_all_facts()? - .into_iter() - .filter(|f| &f.attribute == attribute) - .collect()); + // Pending: in-memory BTreeMap bounded range. + for (key, &fr) in d.pending_indexes.eavt.range(start.clone()..end.clone()) { + if key.entity != *entity_id { + break; } + facts.push(resolve_fact_ref(d, fr)?); + } - let start = AevtKey { - attribute: attribute.clone(), - entity: uuid::Uuid::nil(), - valid_from: i64::MIN, - valid_to: i64::MIN, - tx_count: 0, - }; - // Exact successor of `attribute` (see `get_facts_by_entity_attribute_indexed`): - // `[start, end)` holds exactly this attribute, never prefix siblings such as - // `:ab` for `:a`, and is valid UTF-8 for any attribute (#381). - let end = AevtKey { - attribute: format!("{attribute}\0"), - entity: uuid::Uuid::nil(), - valid_from: i64::MIN, - valid_to: i64::MIN, - tx_count: 0, - }; + // Committed: on-disk B+tree range scan + if let Some(reader) = &d.committed_index_reader { + let committed_refs = reader.range_scan_eavt(&start, Some(&end))?; + for fr in committed_refs { + facts.push(resolve_fact_ref(d, fr)?); + } + } - let mut facts = Vec::new(); + Ok(facts) +} - // Pending: AEVT keys for the exact attribute are contiguous from `start`. - for (_, &fr) in d.pending_indexes.aevt.range(start.clone()..end.clone()) { - facts.push(resolve_fact_ref(&d, fr)?); +/// Get every stored record for one `(entity, attribute)` pair (index-driven, #323). +/// +/// Range-scans EAVT over exactly `[(e, a, …), (e, a + "\0", …))`, so other +/// attributes of the same entity — including prefix siblings such as `:ab` for +/// `:a` — and their version history are never resolved. +fn entity_attribute_facts( + d: &FactData, + entity_id: &EntityId, + attribute: &Attribute, +) -> Result> { + use crate::storage::index::EavtKey; + let matches = |f: &Fact| &f.entity == entity_id && &f.attribute == attribute; + + // Fallback: no indexes built yet + if d.pending_indexes.eavt.is_empty() && d.committed_index_reader.is_none() { + let mut result: Vec = d.facts.iter().filter(|f| matches(f)).cloned().collect(); + if let Some(loader) = &d.committed { + result.extend(loader.stream_all()?.into_iter().filter(|f| matches(f))); + } + return Ok(result); + } + + let start = EavtKey { + entity: *entity_id, + attribute: attribute.clone(), + valid_from: i64::MIN, + valid_to: i64::MIN, + tx_count: 0, + }; + // Exact successor of `attribute`: the smallest string that sorts after it is + // `attribute + "\0"`, so `[start, end)` holds exactly this (e, a) pair. Always + // valid UTF-8 — unlike incrementing the last byte, which fails for attributes + // ending in 0x7F or a character whose last UTF-8 byte is 0xBF (e.g. `:丿`). + let end = EavtKey { + entity: *entity_id, + attribute: format!("{attribute}\0"), + valid_from: i64::MIN, + valid_to: i64::MIN, + tx_count: 0, + }; + + let mut facts = Vec::new(); + + // Pending: EAVT keys for the exact (e, a) are contiguous from `start`; + // the first key with a different entity or attribute ends the run. + for (key, &fr) in d.pending_indexes.eavt.range(start.clone()..) { + if key.entity != *entity_id || key.attribute != *attribute { + break; } + facts.push(resolve_fact_ref(d, fr)?); + } - // Committed: on-disk B+tree range scan over exactly this attribute; the - // post-filter is a cheap guard, not something the range relies on. - if let Some(reader) = &d.committed_index_reader { - for fr in reader.range_scan_aevt(&start, Some(&end))? { - let fact = resolve_fact_ref(&d, fr)?; - if &fact.attribute == attribute { - facts.push(fact); - } + // Committed: on-disk B+tree range scan over exactly this (e, a) pair; the + // post-filter is a cheap guard, not something the range relies on. + if let Some(reader) = &d.committed_index_reader { + for fr in reader.range_scan_eavt(&start, Some(&end))? { + let fact = resolve_fact_ref(d, fr)?; + if matches(&fact) { + facts.push(fact); } } + } - Ok(facts) + Ok(facts) +} + +/// Get all facts for a specific attribute (index-driven). +fn attribute_facts(d: &FactData, attribute: &Attribute) -> Result> { + use crate::storage::index::AevtKey; + + // Fallback: no index + if d.pending_indexes.aevt.is_empty() && d.committed_index_reader.is_none() { + return Ok(all_facts(d)? + .into_iter() + .filter(|f| &f.attribute == attribute) + .collect()); + } + + let start = AevtKey { + attribute: attribute.clone(), + entity: uuid::Uuid::nil(), + valid_from: i64::MIN, + valid_to: i64::MIN, + tx_count: 0, + }; + // Exact successor of `attribute` (see `get_facts_by_entity_attribute_indexed`): + // `[start, end)` holds exactly this attribute, never prefix siblings such as + // `:ab` for `:a`, and is valid UTF-8 for any attribute (#381). + let end = AevtKey { + attribute: format!("{attribute}\0"), + entity: uuid::Uuid::nil(), + valid_from: i64::MIN, + valid_to: i64::MIN, + tx_count: 0, + }; + + let mut facts = Vec::new(); + + // Pending: AEVT keys for the exact attribute are contiguous from `start`. + for (_, &fr) in d.pending_indexes.aevt.range(start.clone()..end.clone()) { + facts.push(resolve_fact_ref(d, fr)?); + } + + // Committed: on-disk B+tree range scan over exactly this attribute; the + // post-filter is a cheap guard, not something the range relies on. + if let Some(reader) = &d.committed_index_reader { + for fr in reader.range_scan_aevt(&start, Some(&end))? { + let fact = resolve_fact_ref(d, fr)?; + if &fact.attribute == attribute { + facts.push(fact); + } + } } + + Ok(facts) } /// Test-only helpers on FactStorage: for use in tests, not the production query path. @@ -1793,6 +1874,54 @@ mod tests { assert_eq!(storage.fact_count(), 2); } + #[test] + fn load_facts_appends_a_batch_and_skips_duplicates() { + let storage = FactStorage::new(); + let a = Fact::new( + uuid::Uuid::new_v4(), + ":t/a".to_string(), + Value::Integer(1), + 1, + ); + let b = Fact::new( + uuid::Uuid::new_v4(), + ":t/a".to_string(), + Value::Integer(2), + 1, + ); + assert_eq!( + storage + .load_facts(vec![a.clone(), b.clone(), a.clone()]) + .unwrap(), + 2 + ); + assert_eq!(storage.fact_count(), 2); + assert_eq!(storage.load_facts(vec![b]).unwrap(), 0); + assert_eq!(storage.fact_count(), 2); + } + + #[test] + fn facts_for_lookups_concatenates_each_lookup_in_order() { + let storage = FactStorage::new(); + let e = uuid::Uuid::new_v4(); + let (x, y) = (":t/x".to_string(), ":t/y".to_string()); + let fx = Fact::new(e, x.clone(), Value::Integer(1), 1); + let fy = Fact::new(e, y.clone(), Value::Integer(2), 1); + storage.load_facts(vec![fx, fy]).unwrap(); + let lookups = [ + Lookup::Attribute(&y), + Lookup::EntityAttribute(&e, &x), + Lookup::Entity(&e), + ]; + let attrs: Vec = storage + .get_facts_for_lookups(&lookups) + .unwrap() + .into_iter() + .map(|f| f.attribute) + .collect(); + assert_eq!(attrs, [":t/y", ":t/x", ":t/x", ":t/y"]); + } + #[test] fn test_load_fact_duplicate_detection_includes_asserted() { let storage = FactStorage::new(); diff --git a/src/query/datalog/executor.rs b/src/query/datalog/executor.rs index 896629031..ead5d24c2 100644 --- a/src/query/datalog/executor.rs +++ b/src/query/datalog/executor.rs @@ -382,6 +382,7 @@ impl DatalogExecutor { /// Results are not deduplicated: the sole caller feeds them to `net_asserted_facts`, /// which is idempotent under duplicated records. fn selective_fact_fetch(&self, patterns: &[Pattern], threshold: usize) -> Option> { + use crate::graph::storage::Lookup; use std::collections::{BTreeMap, BTreeSet}; // Per bound entity: Some(attrs) = only these attributes are referenced; @@ -433,26 +434,19 @@ impl DatalogExecutor { return None; } - let mut all_facts: Vec = Vec::new(); - + // Every lookup reads one state of the store, so a commit cannot land + // between two patterns of this query (#555). + let mut lookups: Vec> = Vec::with_capacity(total); for (uid, attrs) in &entity_attrs { match attrs { Some(attrs) if narrow => { - for attr in attrs { - all_facts.extend( - self.storage - .get_facts_by_entity_attribute_indexed(uid, attr) - .ok()?, - ); - } + lookups.extend(attrs.iter().map(|a| Lookup::EntityAttribute(uid, a))); } - _ => all_facts.extend(self.storage.get_facts_by_entity(uid).ok()?), + _ => lookups.push(Lookup::Entity(uid)), } } - - for attr in &attributes { - all_facts.extend(self.storage.get_facts_by_attribute(attr).ok()?); - } + lookups.extend(attributes.iter().map(Lookup::Attribute)); + let all_facts = self.storage.get_facts_for_lookups(&lookups).ok()?; Some(all_facts) } diff --git a/tests/snapshot_atomicity_test.rs b/tests/snapshot_atomicity_test.rs new file mode 100644 index 000000000..f3c08fcca --- /dev/null +++ b/tests/snapshot_atomicity_test.rs @@ -0,0 +1,168 @@ +//! A query must see each transaction whole (#554) and read one state of the +//! database for all its patterns (#555) while other threads commit. +//! +//! Each test runs a writer thread against a reader thread for a fixed time and +//! fails on the first answer that breaks the invariant. +#![cfg(not(target_arch = "wasm32"))] + +use minigraf::{Minigraf, OpenOptions, QueryResult}; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::thread; +use std::time::{Duration, Instant}; + +const RUN_FOR: Duration = Duration::from_secs(3); + +fn rows(db: &Minigraf, query: &str) -> Vec> { + match db.execute(query).unwrap() { + QueryResult::QueryResults { results, .. } => results + .iter() + .map(|r| { + r.iter() + .map(|v| match v { + minigraf::Value::Integer(n) => n.to_string(), + minigraf::Value::Keyword(k) => k.clone(), + minigraf::Value::String(s) => s.clone(), + _ => "other".to_string(), + }) + .collect() + }) + .collect(), + _ => panic!("expected query results"), + } +} + +/// Runs `write` in a loop on one thread and `check` in a loop on this one +/// until `RUN_FOR` passes. `check` returns an error message on a violation. +fn race( + db: Arc, + mut write: impl FnMut(&Minigraf, u64) + Send + 'static, + mut check: impl FnMut(&Minigraf) -> Option, +) -> Option { + let stop = Arc::new(AtomicBool::new(false)); + let writer = { + let (db, stop) = (Arc::clone(&db), Arc::clone(&stop)); + thread::spawn(move || { + let mut i = 0; + while !stop.load(Ordering::Relaxed) { + write(&db, i); + i += 1; + } + }) + }; + let start = Instant::now(); + let mut found = None; + while start.elapsed() < RUN_FOR { + if let Some(msg) = check(&db) { + found = Some(msg); + break; + } + } + stop.store(true, Ordering::Relaxed); + writer.join().unwrap(); + found +} + +const N: u64 = 500; + +fn batch(i: u64) -> String { + let facts: String = (0..N) + .map(|j| format!("[:b{i}x{j} :batch/id {i}]")) + .collect(); + format!("(transact [{facts}])") +} + +/// Every write commits `N` facts in one transaction; a query must never see +/// a count that is not a multiple of `N` (#554). +fn commit_is_seen_whole(db: Minigraf, write: impl FnMut(&Minigraf, u64) + Send + 'static) { + let found = race(Arc::new(db), write, |db| { + let n = rows(db, "(query [:find ?e :where [?e :batch/id ?b]])").len() as u64; + (n % N != 0).then(|| format!("saw {n} facts, not a multiple of {N}")) + }); + if let Some(msg) = found { + panic!("partial transaction visible: {msg}"); + } +} + +fn write_transaction(db: &Minigraf, i: u64) { + let mut tx = db.begin_write().unwrap(); + tx.execute(&batch(i)).unwrap(); + tx.commit().unwrap(); +} + +fn execute_transact(db: &Minigraf, i: u64) { + db.execute(&batch(i)).unwrap(); +} + +fn file_db(dir: &tempfile::TempDir) -> Minigraf { + let opts = OpenOptions::default().wal_checkpoint_threshold(usize::MAX); + Minigraf::open_with_options(dir.path().join("w.graph"), opts).unwrap() +} + +#[test] +fn write_transaction_commit_is_seen_whole_in_memory() { + commit_is_seen_whole(Minigraf::in_memory().unwrap(), write_transaction); +} + +#[test] +fn write_transaction_commit_is_seen_whole_on_file() { + let dir = tempfile::tempdir().unwrap(); + commit_is_seen_whole(file_db(&dir), write_transaction); +} + +#[test] +fn execute_transact_is_seen_whole_in_memory() { + commit_is_seen_whole(Minigraf::in_memory().unwrap(), execute_transact); +} + +#[test] +fn execute_transact_is_seen_whole_on_file() { + let dir = tempfile::tempdir().unwrap(); + commit_is_seen_whole(file_db(&dir), execute_transact); +} + +/// A query fetched each attribute with its own `get_live_facts` call, each +/// under its own read lock, so a transaction that committed between the two +/// calls was seen by one pattern and not the other (#555). Every transaction +/// writes `:pair/x` and `:pair/y` for one entity together, atomically (#554). +/// Attributes are fetched in sorted order, so only the query whose positive +/// pattern is `:pair/y` can catch a commit between the fetches; both are run. +fn patterns_see_one_state(db: Minigraf) { + let found = race( + Arc::new(db), + |db, i| { + db.execute(&format!( + "(transact [[:p{i} :pair/x {i}] [:p{i} :pair/y {i}]])" + )) + .unwrap(); + }, + |db| { + for q in [ + "(query [:find ?e :where [?e :pair/x ?v] (not [?e :pair/y ?v])])", + "(query [:find ?e :where [?e :pair/y ?v] (not [?e :pair/x ?v])])", + ] { + let n = rows(db, q).len(); + if n != 0 { + return Some(format!("{n} entities with one attribute of the pair: {q}")); + } + } + None + }, + ); + if let Some(msg) = found { + panic!("one query saw two database states: {msg}"); + } +} + +#[test] +fn patterns_of_one_query_see_one_state_in_memory() { + patterns_see_one_state(Minigraf::in_memory().unwrap()); +} + +#[test] +fn patterns_of_one_query_see_one_state_on_file() { + let dir = tempfile::tempdir().unwrap(); + // No checkpoints: on v2.x a query during a checkpoint can lose committed + // facts, a separate bug this test does not cover. + patterns_see_one_state(file_db(&dir)); +} From 5d0304c76ff48717f39dfc93c8780dc96bcb76f0 Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Sun, 11 Oct 2026 23:42:56 +0530 Subject: [PATCH 5/7] fix(storage): queries wait for a checkpoint instead of reading it half-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 Claude-Session: https://claude.ai/code/session_012wXxZguv7PgKQjMMH4yJ5h --- CHANGELOG.md | 3 +- CLAUDE.md | 2 +- README.md | 2 +- docs/TEST_COVERAGE.md | 4 +-- src/graph/storage.rs | 36 ++++++++++++++++++++++++ src/storage/persistent_facts.rs | 15 ++++++---- tests/snapshot_atomicity_test.rs | 48 ++++++++++++++++++++++++++++++-- 7 files changed, 96 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index adbc6c53f..e6458fb63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,10 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **A query on another thread sees each transaction whole (#554).** A commit wrote its WAL entry, then added the transaction's facts to the in-memory store one fact at a time, each under its own lock, so a query running on another thread could return part of the transaction. `execute("(transact ...)")`, `execute("(retract ...)")` and `WriteTransaction::commit` all did this. They now add the whole transaction under one lock, as does WAL replay. Stored data was never affected. Affected `WriteTransaction` since v0.5.0 and `execute` since v0.6.0. Also fixed in v3.0.0. - **All patterns of one query read the same state of the database (#555).** A query whose patterns name entities or attributes read each of them with its own lookup, each under its own lock, so a transaction that committed between two lookups was seen by some patterns and not others, and the query could return rows that match no state of the database. The lookups now run under one lock. Stored data was never affected. Affected every release since v1.1.0. Also fixed in v3.0.0. +- **A query during a checkpoint no longer misses committed facts (#558).** A checkpoint wrote its new fact pages over the index pages that queries on other threads were still reading, then installed its new readers and cleared the pending facts in three separately locked steps. A query during a checkpoint, including the automatic one every `wal_checkpoint_threshold` WAL entries, could miss facts committed before it started or see part of a transaction. Queries now wait for a checkpoint to finish: it holds the store's write lock from its first page write until the new state is published. Stored data was never affected. Affected every release since v0.9.0; v3.0.0's copy-on-write checkpoints are not affected. ### Tests -- `tests/snapshot_atomicity_test.rs` races a writer against a reader: a query must see each transaction whole, whether written with `execute` or a `WriteTransaction` (#554), and must read one state for all its patterns (#555), in memory and on file. +- `tests/snapshot_atomicity_test.rs` races a writer against a reader: a query must see each transaction whole, whether written with `execute` or a `WriteTransaction` (#554), and must read one state for all its patterns (#555), in memory and on file, and must not miss committed facts while a checkpoint runs (#558). ## v2.0.5 — 2026-10-11 diff --git a/CLAUDE.md b/CLAUDE.md index 8abf03d7e..d14476e07 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -174,7 +174,7 @@ Auto-migrates v1/v2/v3/v4/v5/v6 → v7 on open/checkpoint. ## Test Coverage -**1249 tests** (1240 passing, 9 ignored; unit + integration + doc). +**1250 tests** (1241 passing, 9 ignored; unit + integration + doc). See `docs/TEST_COVERAGE.md` for the full per-file breakdown. **Testing conventions** — see the Testing Conventions section below before writing any tests. diff --git a/README.md b/README.md index 45c1e7d79..9232f34a5 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,7 @@ let r2 = pq.execute(&[("tx", BindValue::TxCount(2)), ("entity", BindValue::Entit ```bash cargo run # interactive Datalog REPL -cargo test # run 1249 tests +cargo test # run 1250 tests cargo run < demos/demo_recursive.txt # recursive rules demo ``` diff --git a/docs/TEST_COVERAGE.md b/docs/TEST_COVERAGE.md index 28383b99f..8ef6a1a4c 100644 --- a/docs/TEST_COVERAGE.md +++ b/docs/TEST_COVERAGE.md @@ -4,7 +4,7 @@ **Verified**: 2026-10-11 with `cargo test` -**Result**: 1,240 passing tests and 9 ignored tests (1,249 total), including the golden-file corpus (#391) +**Result**: 1,241 passing tests and 9 ignored tests (1,250 total), including the golden-file corpus (#391) For v2.0.5, `tests/edge_cases_test.rs` checks that a file of 1 to 4,095 bytes is `STG-045`, unchanged and with no WAL, when it is a database cut below one page or foreign bytes, and opens as a new database that takes a write and reads it back after reopen when it is a prefix of the initial header or all zeros (#506). @@ -22,7 +22,7 @@ The v2.0.5 suite covers the public database API, the Datalog engine, storage and - **Core database behavior**: in-memory and file-backed operation, transactions, checkpoints, retractions, bi-temporal queries, recursive rules, prepared statements, aggregation, expressions, disjunction, window functions, UDFs, and magic-sets evaluation. - **Storage correctness**: packed pages, B+tree indexes and range scans, cache behavior, file-header validation, migration, WAL replay, checksums, corruption handling, and fault injection. -- **Reliability and concurrency**: concurrent reads and writes, rollback behavior, same-process handle exclusion, cross-process locking, PID namespaces, NFS and container lock behavior, and SIGKILL recovery, and queries seeing each concurrent transaction whole and reading one state for all their patterns (`tests/snapshot_atomicity_test.rs`, #554, #555). +- **Reliability and concurrency**: concurrent reads and writes, rollback behavior, same-process handle exclusion, cross-process locking, PID namespaces, NFS and container lock behavior, and SIGKILL recovery, and queries seeing each concurrent transaction whole and reading one state for all their patterns, also during a checkpoint (`tests/snapshot_atomicity_test.rs`, #554, #555, #558). - **Public diagnostics**: structured parser, query, storage, WAL, API, and internal error codes; the error-code registry is checked against [`ERROR_REFERENCE.md`](ERROR_REFERENCE.md). - **Compatibility and quality**: golden-file corpus of released file formats, cross-platform format compatibility, property-based tests, long-haul smoke coverage, a weekly soak at 10M facts with a 5-hour churn (#392), XTDB/Datomic semantic compatibility, grammar conformance, and rustdoc examples. diff --git a/src/graph/storage.rs b/src/graph/storage.rs index 21727c943..90aa0aba9 100644 --- a/src/graph/storage.rs +++ b/src/graph/storage.rs @@ -411,13 +411,22 @@ impl FactStorage { d.pending_indexes = indexes; } + /// Exclusive access for a checkpoint (#558): no query reads the store + /// until the guard drops, so none sees the pages the checkpoint overwrites + /// or a state between its publishing steps. + pub(crate) fn lock_for_checkpoint(&self) -> CheckpointGuard<'_> { + CheckpointGuard(self.data.write().unwrap_or_else(|e| e.into_inner())) + } + /// Return the pending (uncommitted) facts held in memory. + #[cfg(test)] pub(crate) fn get_pending_facts(&self) -> Vec { let d = self.data.read().unwrap_or_else(|e| e.into_inner()); d.facts.clone() } /// Clear pending facts and pending indexes after a successful checkpoint. + #[cfg(test)] pub(crate) fn post_checkpoint_clear(&self) { let mut d = self.data.write().unwrap_or_else(|e| e.into_inner()); d.facts.clear(); @@ -660,6 +669,33 @@ impl FactStorage { } } +/// [`FactStorage`] held exclusively by a checkpoint; see +/// [`FactStorage::lock_for_checkpoint`]. Take it before the backend lock: a +/// query holds the store's read lock while it reads pages. +pub(crate) struct CheckpointGuard<'a>(std::sync::RwLockWriteGuard<'a, FactData>); + +impl CheckpointGuard<'_> { + /// The pending (uncommitted) facts held in memory. + pub(crate) fn pending_facts(&self) -> Vec { + self.0.facts.clone() + } + + /// Install the checkpoint's readers and drop the pending facts it wrote, + /// in one step. + pub(crate) fn publish( + &mut self, + reader: Arc, + index_reader: Arc, + ) { + let d = &mut *self.0; + d.committed = Some(reader); + d.committed_index_reader = Some(index_reader); + d.facts.clear(); + d.pending_keys.clear(); + d.pending_indexes = Indexes::new(); + } +} + /// One index lookup of [`FactStorage::get_facts_for_lookups`]. #[derive(Clone, Copy)] pub(crate) enum Lookup<'a> { diff --git a/src/storage/persistent_facts.rs b/src/storage/persistent_facts.rs index 47693af24..9eac695d7 100644 --- a/src/storage/persistent_facts.rs +++ b/src/storage/persistent_facts.rs @@ -870,8 +870,13 @@ impl PersistentFactStorage { return Ok(()); } + // Queries wait until the new state is published: the new fact pages + // overwrite the pages of the index they read (#558). Taken before the + // backend lock, the order queries use. + let mut store = self.storage.lock_for_checkpoint(); + // ── Step A: read current header + stream old B+tree entries BEFORE overwriting ── - let pending_facts = self.storage.get_pending_facts(); + let pending_facts = store.pending_facts(); let mut backend = self .backend .lock() @@ -1035,7 +1040,6 @@ impl PersistentFactStorage { committed_fact_pages: self.committed_fact_pages.clone(), first_fact_page: 1, }); - self.storage.set_committed_reader(loader); let index_reader: Arc = Arc::new(OnDiskIndexReader::new( @@ -1046,10 +1050,9 @@ impl PersistentFactStorage { avet_root, vaet_root, )); - self.storage.set_committed_index_reader(index_reader); - - // Clear pending — all data now on disk - self.storage.post_checkpoint_clear(); + // Readers in place and pending cleared — all data now on disk — in + // one step under the checkpoint lock. + store.publish(loader, index_reader); Ok(()) } diff --git a/tests/snapshot_atomicity_test.rs b/tests/snapshot_atomicity_test.rs index f3c08fcca..3f992f624 100644 --- a/tests/snapshot_atomicity_test.rs +++ b/tests/snapshot_atomicity_test.rs @@ -7,7 +7,7 @@ use minigraf::{Minigraf, OpenOptions, QueryResult}; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::thread; use std::time::{Duration, Instant}; @@ -162,7 +162,49 @@ fn patterns_of_one_query_see_one_state_in_memory() { #[test] fn patterns_of_one_query_see_one_state_on_file() { let dir = tempfile::tempdir().unwrap(); - // No checkpoints: on v2.x a query during a checkpoint can lose committed - // facts, a separate bug this test does not cover. + // No checkpoints: `checkpoint_never_hides_committed_facts` covers those. patterns_see_one_state(file_db(&dir)); } + +/// A checkpoint wrote its new fact pages over the index pages queries were +/// reading, and published its new readers in three locked steps, so a query +/// during a checkpoint could miss committed facts or see half a transaction +/// (#558). Every write here is followed by a checkpoint. +#[test] +fn checkpoint_never_hides_committed_facts() { + let dir = tempfile::tempdir().unwrap(); + let committed = Arc::new(AtomicU64::new(0)); + let found = { + let written = Arc::clone(&committed); + race( + Arc::new(file_db(&dir)), + move |db, i| { + db.execute(&format!( + "(transact [[:c{i} :pair/x {i}] [:c{i} :pair/y {i}]])" + )) + .unwrap(); + written.store(i + 1, Ordering::SeqCst); + db.checkpoint().unwrap(); + }, + |db| { + let before = committed.load(Ordering::SeqCst) as usize; + let half = rows( + db, + "(query [:find ?e :where [?e :pair/y ?v] (not [?e :pair/x ?v])])", + ) + .len(); + let seen = rows(db, "(query [:find ?e :where [?e :pair/x ?v]])").len(); + if half != 0 { + Some(format!("{half} entities with y and no x")) + } else if seen < before { + Some(format!("{seen} entities seen, {before} committed before")) + } else { + None + } + }, + ) + }; + if let Some(msg) = found { + panic!("a query during a checkpoint saw the wrong state: {msg}"); + } +} From e194cb90df750819e17a698e1e5e6f31c8889c7f Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Tue, 13 Oct 2026 10:25:57 +0530 Subject: [PATCH 6/7] ci(bench): name the branch in every regression issue Issues for main were titled "Benchmark regression: " with no branch, while v3 issues carried "(v3)". Title every issue "Benchmark regression: ()", like the nightly fuzz issue, and add a Branch line to the body. An open issue with the old main title is still matched, renamed and commented on. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HyLscCUuJ1GQpdAeX79EdR --- .github/workflows/bench.yml | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 05af0de9f..6a46e9120 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -266,10 +266,11 @@ jobs: # Shared CI runners are noisy, so one alert on its own is usually a slow # runner, not a regression. Report only benchmarks that also alerted in - # this suite on the previous run. Each suite has its own issue: if one is - # open, add a comment to it instead of skipping. Every outcome, including - # an alert that is not reported, goes in the job summary, because the - # Bencher step is continue-on-error and the run stays green (#393). + # this suite on the previous run. Each suite and branch has its own issue, + # titled with the branch like the nightly fuzz issue: if one is open, add + # a comment to it instead of skipping. Every outcome, including an alert + # that is not reported, goes in the job summary, because the Bencher step + # is continue-on-error and the run stays green (#393). - name: Report regression if: steps.bencher.outcome == 'failure' uses: actions/github-script@v9 @@ -282,8 +283,7 @@ jobs: const suite = process.env.SUITE; const ref = process.env.BENCH_REF; const jobName = `bench / ${suite}`; - // main keeps its existing issue titles; other branches get their own. - const label = ref === 'main' ? suite : `${suite}, ${ref}`; + const label = `${suite}, ${ref}`; // Bencher lists each alert as "- (Latency): <.../alerts/>". // The "View results" lines use /perf? URLs, so they do not match. @@ -300,6 +300,7 @@ jobs: body = [ `Bencher failed in the nightly benchmark run (${label}) but listed no alerts. This is likely an API or upload error.`, '', + `**Branch:** \`${ref}\``, `**Run:** ${runUrl}`, ].join('\n'); } else { @@ -358,6 +359,7 @@ jobs: '', ...repeated.map(b => `- \`${b}\``), '', + `**Branch:** \`${ref}\``, `**Run:** ${runUrl}`, `**Previous run:** ${previousUrl}`, '', @@ -371,12 +373,20 @@ jobs: state: 'open', per_page: 100, }); - const title = ref === 'main' - ? `Benchmark regression: ${suite}` - : `Benchmark regression: ${suite} (${ref})`; - const existing = openIssues.find(i => !i.pull_request && i.title === title); + const title = `Benchmark regression: ${suite} (${ref})`; + // Issues for main used to be titled without the branch. Keep + // commenting on such an open issue, and give it the new title. + const legacyTitle = ref === 'main' ? `Benchmark regression: ${suite}` : null; + const existing = + openIssues.find(i => !i.pull_request && i.title === title) || + openIssues.find(i => !i.pull_request && legacyTitle && i.title === legacyTitle); let reported; if (existing) { + if (existing.title !== title) { + await github.rest.issues.update({ + owner, repo, issue_number: existing.number, title, + }); + } await github.rest.issues.createComment({ owner, repo, issue_number: existing.number, body, }); From 75ffc9b11f58cee3494430677465d553e582847b Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Tue, 13 Oct 2026 10:53:44 +0530 Subject: [PATCH 7/7] ci(bench): compare each run with the last 30 runs, not all history (#568) Bencher's t-test used every sample since May, so a correct step change kept alerting for weeks: query/predicate_pushdown/100k since #445 (#512, #495), and checkpoint/* and open/wal_replay/100k since #476's backport stopped the drop-save that had made those fixtures no-ops (#561, #562). Replaying main's history, a 30-run window still alerts on two runs in a row for a lasting change, then takes it as the baseline after about five runs. Claude-Session: https://claude.ai/code/session_016QAt9R1aSPQG7rV5ouFDMU Co-authored-by: Claude Opus 5.5 --- .github/workflows/bench.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 6a46e9120..d3482f22e 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -245,6 +245,12 @@ jobs: continue-on-error: true run: | set -o pipefail + # The t-test compares a run with the last 30 runs of its branch, not + # all history. Without a window, a correct step change (#445's + # predicate_pushdown, #476's checkpoint and wal_replay fixtures) kept + # alerting for weeks; with 30, a lasting change alerts on about five + # runs (enough for the two-in-a-row issue below), then becomes the + # baseline. # A branch other than main starts from main's history and thresholds. start_point=() if [ "$BENCH_REF" != main ]; then @@ -258,6 +264,7 @@ jobs: --testbed ubuntu-latest \ --threshold-measure latency \ --threshold-test t_test \ + --threshold-max-sample-size 30 \ --threshold-upper-boundary ${{ matrix.threshold }} \ --err \ --adapter rust_criterion \