8000 fix(db): usize::MAX threshold suppresses the close-time save too (#476) by adityamukho · Pull Request #526 · 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 @@ -45,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- **A v7 fact page with a damaged type byte fails the upgrade with `STG-014`** (#496). Opening a v7 file skipped, without an error, any page in its fact-page range whose type byte was not the fact-page type. The read-write open then upgraded the file to v8 and freed the old v7 pages, so that page's facts were lost for good; the read-only open returned the same answer with them missing. A v2.x writer fills that range with fact pages only, so any other type there is damage: open now fails with `STG-014`, read-write and read-only, and leaves the v7 file as it was. v7 fact pages have no checksum of their own, so damage elsewhere in a page can still decode to wrong facts or fail with another code; keep a copy of a v7 file until it has opened under v3.0.0.
- Reopening next to a WAL whose entries were all already checkpointed no longer rewinds the transaction counter, so later transactions no longer reuse `tx_count` values and `:as-of` history stays correct (#447; also fixed in v2.0.3).
- `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. Not backported to v2.x: it is not a data-integrity fix.
- 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. Not backported to v2.x: it is not a data-integrity fix.

### Internal

Expand Down
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ Auto-migrates v7 → v8 on open (spec §9, with a backup meta page). v1–v6 are

## Test Coverage

**1424 tests** (1414 passing, 10 ignored; unit + integration + doc).
**1426 tests** (1416 passing, 10 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 @@ -131,7 +131,7 @@ let r2 = pq.execute(&[("tx", BindValue::TxCount(2)), ("entity", BindValue::Entit

```bash
cargo run # interactive Datalog REPL
cargo test # run 1424 tests
cargo test # run 1426 tests
cargo run < demos/demo_recursive.txt # recursive rules demo
```

Expand Down
4 changes: 2 additions & 2 deletions docs/TEST_COVERAGE.md

Large diffs are not rendered by default.

31 changes: 29 additions & 2 deletions src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ pub struct OpenOptions {
/// size of the database (see [`Minigraf::checkpoint`]), and durability does not
/// depend on it: the WAL is crash-durable. Lower values keep the WAL and reopen
/// time small; higher values batch more facts per checkpoint.
///
/// `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).
///
Expand Down Expand Up @@ -361,8 +366,17 @@ 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).
if self.options.wal_checkpoint_threshold == usize::MAX || self.options.read_only {
// 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.read_only {
return;
}
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() {
Expand Down Expand Up @@ -2321,6 +2335,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();
Expand Down
4 changes: 3 additions & 1 deletion src/storage/persistent_facts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,9 @@ impl<B: StorageBackend + 'static> PersistentFactStorage<B> {
}

/// Forget unsaved changes, so dropping this storage commits nothing. Used
/// by an abandoned log writer, whose file is deleted next (#431).
/// by an abandoned log writer, whose file is deleted next (#431), and 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;
Expand Down
48 changes: 48 additions & 0 deletions tests/wal_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -907,3 +907,51 @@ fn test_v2_file_is_rejected() {
};
assert_eq!(err.code(), "STG-028", "v2 file must fail with STG-028");
}

/// #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");
}
Loading
0