From 170d6fae82a404f2a80da999ff2b4fd0e11d3318 Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Tue, 6 Oct 2026 08:30:35 +0530 Subject: [PATCH] fix(storage): never rewind the tx counter on WAL replay (#447) Open sets the transaction counter from the file's checkpointed count, then WAL replay skips entries at or below it and calls restore_tx_counter, which stored the highest tx_count among the facts it had loaded. When every WAL entry was already checkpointed, nothing was loaded and the counter was reset to 0, so later transactions reused tx_counts already in the file and :as-of history was wrong, permanently after the next checkpoint. restore_tx_counter now only raises the counter (fetch_max). Refs #447 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015GgLEs8YozJjYhwPEYgTUn --- CHANGELOG.md | 1 + CLAUDE.md | 2 +- README.md | 2 +- docs/TEST_COVERAGE.md | 4 +-- src/graph/storage.rs | 27 +++++++++++++++++--- tests/wal_test.rs | 57 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 86 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 208e39804..c39aadd29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Queries returned wrong results with more than 65,535 uncheckpointed facts (#445).** Each fact not yet checkpointed was indexed by its position in memory, stored as a 16-bit number that stopped counting at 65,535. Every later fact was indexed as that same position, so entity-bound queries (`[:e :attr ?v]`) and attribute scans (`[?e :attr ?v]`) silently lost or duplicated facts. This affected in-memory databases with more than 65,535 facts and file databases with more than 65,535 facts written since the last checkpoint. The stored data was always intact, and a checkpoint made file databases correct again. Positions are now 32-bit and checked. Affected releases: v0.7.0 through v2.0.2. The file format is unchanged. +- **Reopening next to an already-checkpointed WAL rewound the transaction counter (#447).** The WAL normally disappears after a checkpoint. It survives one only if the process crashes between the checkpoint's commit and the WAL delete, if the delete fails (WAL-006), or with `wal_checkpoint_threshold(usize::MAX)`. When such a WAL held only entries already in the file, replay skipped them all and then reset the transaction counter from the facts it had loaded, which were none, so it reset to 0. The next transactions reused `tx_count` values already in the file, `:as-of` answers mixed old and new transactions, and the next checkpoint made that history permanent. The counter now never moves backwards on replay. Affected releases: v0.5.0 through v2.0.2. ## v2.0.2 — 2026-09-27 diff --git a/CLAUDE.md b/CLAUDE.md index 2aedaa28d..0d31311ec 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 -**1217 tests** (1209 passing, 8 ignored; unit + integration + doc). +**1219 tests** (1211 passing, 8 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 318191365..082451215 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 1217 tests +cargo test # run 1219 tests cargo run < demos/demo_recursive.txt # recursive rules demo ``` diff --git a/docs/TEST_COVERAGE.md b/docs/TEST_COVERAGE.md index 3298a56fe..540d5ba8e 100644 --- a/docs/TEST_COVERAGE.md +++ b/docs/TEST_COVERAGE.md @@ -4,9 +4,9 @@ **Verified**: 2026-09-27 with `cargo test` -**Result**: 1,209 passing tests and 8 ignored tests (1,217 total, including the #445 tests added after v2.0.2) +**Result**: 1,211 passing tests and 8 ignored tests (1,219 total, including the #445 and #447 tests added after v2.0.2) -The v2.0.2 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). After v2.0.2, 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). +The v2.0.2 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). After v2.0.2, 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). ### Covered Areas diff --git a/src/graph/storage.rs b/src/graph/storage.rs index 771151e50..1edaa6354 100644 --- a/src/graph/storage.rs +++ b/src/graph/storage.rs @@ -294,17 +294,19 @@ impl FactStorage { Ok(true) } - /// Set tx_counter to max(tx_count) across all loaded facts. + /// Raise tx_counter to max(tx_count) across all loaded facts. /// /// Must be called after all `load_fact()` calls complete so that the next - /// `transact()` call picks up from the right sequence number. + /// `transact()` call picks up from the right sequence number. The counter + /// never moves backwards: it may already hold the file's checkpointed count, + /// and when WAL replay loads no facts that count must survive (#447). pub(crate) fn restore_tx_counter(&self) -> Result<()> { let d = self .data .read() .map_err(|_| err_coded!(ErrorCode::Int050, "data"))?; let max = d.facts.iter().map(|f| f.tx_count).max().unwrap_or(0); - self.tx_counter.store(max, Ordering::SeqCst); + self.tx_counter.fetch_max(max, Ordering::SeqCst); Ok(()) } @@ -2503,4 +2505,23 @@ mod tests { assert!(pending_refs(base, 2).is_err(), "past u32::MAX is an error"); } } + + /// #447: restoring the counter after loading facts never lowers it. + #[test] + fn restore_tx_counter_never_moves_backwards() { + let storage = FactStorage::new(); + storage.restore_tx_counter_from(5); + storage.restore_tx_counter().unwrap(); + assert_eq!(storage.current_tx_count(), 5, "no loaded facts: keep 5"); + let mut f = Fact::new( + uuid::Uuid::from_u128(1), + ":a".to_string(), + Value::Integer(1), + 1, + ); + f.tx_count = 9; + storage.load_fact(f).unwrap(); + storage.restore_tx_counter().unwrap(); + assert_eq!(storage.current_tx_count(), 9, "a higher loaded count wins"); + } } diff --git a/tests/wal_test.rs b/tests/wal_test.rs index 02cdc4a20..68e76fdf3 100644 --- a/tests/wal_test.rs +++ b/tests/wal_test.rs @@ -148,6 +148,63 @@ fn test_no_duplicate_facts_after_post_checkpoint_crash() { ); } +/// #447: reopening next to a WAL whose entries are all checkpointed must not +/// rewind the transaction counter. Before the fix, the next transactions +/// reused tx 1 and 2, so `:as-of` saw them as part of history they never were. +#[test] +fn test_reopen_with_checkpointed_wal_does_not_reuse_tx_counts() { + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("rewind.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(); // tx 1 + db.execute("(transact [[:alice :age 31]])").unwrap(); // tx 2 + 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 ages = |db: &Minigraf, q: &str| -> Vec { + let QueryResult::QueryResults { results, .. } = db.execute(q).unwrap() else { + panic!("expected query results"); + }; + let mut v: Vec = results + .iter() + .flatten() + .map(|v| match v { + minigraf::Value::Integer(n) => *n, + _ => panic!("expected an integer"), + }) + .collect(); + v.sort_unstable(); + v + }; + for reopen in 0..2 { + let db = Minigraf::open(&db_path).unwrap(); + if reopen == 0 { + db.execute("(retract [[:alice :age 30]])").unwrap(); // tx 3 + db.execute("(transact [[:alice :age 99]])").unwrap(); // tx 4 + } + let when = if reopen == 0 { + "before reopen" + } else { + "after reopen" + }; + let q = |n| format!("(query [:find ?a :as-of {n} :where [:alice :age ?a]])"); + assert_eq!(ages(&db, &q(1)), vec![30], "as-of 1 {when}"); + assert_eq!(ages(&db, &q(2)), vec![30, 31], "as-of 2 {when}"); + assert_eq!(ages(&db, &q(3)), vec![31], "as-of 3 {when}"); + let now = ages(&db, "(query [:find ?a :where [:alice :age ?a]])"); + assert_eq!(now, vec![31, 99], "current {when}"); + } +} + // ── 4. Partial WAL entry is discarded; earlier entries intact ───────────────── /// Write 1 fact, crash (no checkpoint), then append garbage bytes to the WAL