8000 fix(storage): never rewind the tx counter on WAL replay (#447) by adityamukho · Pull Request #448 · project-minigraf/minigraf · GitHub
[go: up one dir, main page]

Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
```

Expand Down
4 changes: 2 additions & 2 deletions docs/TEST_COVERAGE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
27 changes: 24 additions & 3 deletions src/graph/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}

Expand Down Expand Up @@ -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");
}
}
57 changes: 57 additions & 0 deletions tests/wal_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<i64> {
let QueryResult::QueryResults { results, .. } = db.execute(q).unwrap() else {
panic!("expected query results");
};
let mut v: Vec<i64> = 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
Expand Down
Loading
0