From 596bb5960b38d75fdebfad244fb661e995252854 Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Wed, 7 Oct 2026 13:02:49 +0530 Subject: [PATCH] fix(browser): an unreadable dirty page is an error, not dropped from the IndexedDB flush (#470) checkpoint(), importGraph() and every write collected dirty pages with read_page_raw(id).ok() in a filter_map, so a page that failed to read was silently left out of the flush to IndexedDB while the call reported success. A shared take_dirty_pages helper (the same one v3 has since #469) now returns the read error. No current path leaves a dirty id without a page (ids enter the dirty set only after the page is stored), so this is defensive rather than a fix for observed loss. The browser unit tests did not compile on wasm32 (a duplicate page_cache_capacity from #341/#342, and dir_sync tests using tempfile); both are fixed so the two new regression tests run. Claude-Session: https://claude.ai/code/session_015GgLEs8YozJjYhwPEYgTUn Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 4 ++ docs/TEST_COVERAGE.md | 2 + src/browser/buffer.rs | 6 +++ src/browser/mod.rs | 65 ++++++++++++++++++++------------- src/storage/dir_sync.rs | 3 +- src/storage/persistent_facts.rs | 11 ------ 6 files changed, 54 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63aab6159..2707f3e86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Fixed + +- **`BrowserDb` no longer drops a dirty page it cannot read from the IndexedDB flush.** `checkpoint()`, `importGraph()` and every write collected dirty pages with `read_page_raw(id).ok()`, so a page that failed to read was left out of the flush while the call still reported success. The read error is now returned. No current code path leaves a dirty id without a page, so this guards against a future bug rather than fixing observed data loss. The browser unit tests compile again on wasm32 (a duplicate `page_cache_capacity` and `tempfile`-based `dir_sync` tests). + ### Tests - **Golden-file compatibility corpus (#391).** `tests/golden/` holds v7 files written by minigraf 2.0.3, each with a JSON manifest of its CRC32, `tx_count` and expected query results. They cover one checkpoint, several checkpoints, indexes rebuilt on open, a WAL left by a crash, a WAL whose entries were already checkpointed, and same-transaction multi-values (#371). `tests/golden_corpus_test.rs` opens a copy of each file and checks its manifest after open, after a checkpoint and reopen, and after one more write. Golden files are never regenerated, only added; a new `golden-corpus` job in `policy.yml` fails a pull request that changes one. The generator is a standalone crate pinned to `minigraf = "=2.0.3"` (`tests/golden/gen/v7/`). diff --git a/docs/TEST_COVERAGE.md b/docs/TEST_COVERAGE.md index 450203b86..6c6bb0443 100644 --- a/docs/TEST_COVERAGE.md +++ b/docs/TEST_COVERAGE.md @@ -6,6 +6,8 @@ **Result**: 1,213 passing tests and 8 ignored tests (1,221 total), including the golden-file corpus added after v2.0.3 +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.3 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). ### Covered Areas diff --git a/src/browser/buffer.rs b/src/browser/buffer.rs index d170390a2..96dc9acca 100644 --- a/src/browser/buffer.rs +++ b/src/browser/buffer.rs @@ -43,6 +43,12 @@ impl BrowserBufferBackend { pub fn take_dirty(&mut self) -> HashSet { std::mem::take(&mut self.dirty) } + + /// Mark `page_id` dirty without writing it, for tests of the flush path. + #[cfg(test)] + pub(crate) fn mark_dirty_for_test(&mut self, page_id: u64) { + self.dirty.insert(page_id); + } } impl Default for BrowserBufferBackend { diff --git a/src/browser/mod.rs b/src/browser/mod.rs index 75daf39f0..66f27b32f 100644 --- a/src/browser/mod.rs +++ b/src/browser/mod.rs @@ -151,15 +151,7 @@ impl BrowserDb { .pfs .save() .map_err(|e| JsValue::from_str(&e.to_string()))?; - let dirty_ids = inner.pfs.with_backend_mut(|b| b.take_dirty()); - let pages: Vec<(u64, Vec)> = dirty_ids - .into_iter() - .filter_map(|id| { - inner - .pfs - .with_backend(|b| b.read_page_raw(id).ok().map(|d| (id, d))) - }) - .collect(); + let pages = take_dirty_pages(&mut inner.pfs)?; (pages, inner.idb.is_some()) }; @@ -225,13 +217,7 @@ impl BrowserDb { let new_fact_storage = new_pfs.storage().clone(); // Drain dirty set and collect owned page bytes before swapping inner. - let dirty_ids = new_pfs.with_backend_mut(|b| b.take_dirty()); - let dirty_pages: Vec<(u64, Vec)> = dirty_ids - .into_iter() - .filter_map(|id| { - new_pfs.with_backend(|b| b.read_page_raw(id).ok().map(|d| (id, d))) - }) - .collect(); + let dirty_pages = take_dirty_pages(&mut new_pfs)?; inner.pfs = new_pfs; inner.fact_storage = new_fact_storage; @@ -295,15 +281,7 @@ impl BrowserDb { .map_err(|e| JsValue::from_str(&e.to_string()))?; // Collect dirty pages as owned Vec<(u64, Vec)> — no borrows escape - let dirty_ids = inner.pfs.with_backend_mut(|b| b.take_dirty()); - let dirty_pages: Vec<(u64, Vec)> = dirty_ids - .into_iter() - .filter_map(|id| { - inner - .pfs - .with_backend(|b| b.read_page_raw(id).ok().map(|d| (id, d))) - }) - .collect(); + let dirty_pages = take_dirty_pages(&mut inner.pfs)?; let json = if is_retract { format!(r#"{{"retracted":{}}}"#, tx_id) @@ -330,6 +308,21 @@ impl BrowserDb { // ── JSON serialisation helpers (free functions, not exported to WASM) ──────── +/// The bytes of every page the last `save()` dirtied, clearing the dirty set. +/// A page that cannot be read is an error, never a page silently left out of +/// the flush to IndexedDB. +fn take_dirty_pages( + pfs: &mut PersistentFactStorage, +) -> Result)>, JsValue> { + let ids = pfs.with_backend_mut(|b| b.take_dirty()); + pfs.with_backend(|b| { + ids.into_iter() + .map(|id| b.read_page_raw(id).map(|d| (id, d))) + .collect::>>() + }) + .map_err(|e| JsValue::from_str(&crate::MinigrafError::from(e).to_string())) +} + fn query_result_to_json(result: QueryResult) -> String { use serde_json::{Value as JVal, json}; @@ -375,6 +368,28 @@ mod tests { wasm_bindgen_test_configure!(run_in_browser); + #[wasm_bindgen_test] + fn take_dirty_pages_returns_written_pages() { + use crate::storage::StorageBackend; + let mut pfs = PersistentFactStorage::new(BrowserBufferBackend::new(), 0).unwrap(); + let _ = pfs.with_backend_mut(|b| b.take_dirty()); + pfs.with_backend_mut(|b| b.write_page(3, &[7u8; crate::storage::PAGE_SIZE])) + .unwrap(); + let pages = take_dirty_pages(&mut pfs).expect("dirty pages"); + assert_eq!(pages.len(), 1); + assert_eq!(pages[0].0, 3); + assert!(take_dirty_pages(&mut pfs).expect("second call").is_empty()); + } + + #[wasm_bindgen_test] + fn unreadable_dirty_page_is_an_error_not_skipped() { + let mut pfs = PersistentFactStorage::new(BrowserBufferBackend::new(), 0).unwrap(); + // A dirty id with no page behind it: before the fix it was dropped + // from the flush and the caller still saw success. + pfs.with_backend_mut(|b| b.mark_dirty_for_test(99)); + assert!(take_dirty_pages(&mut pfs).is_err()); + } + #[wasm_bindgen_test] async fn in_memory_transact_and_query() { let db = BrowserDb::open_in_memory().expect("open_in_memory"); diff --git a/src/storage/dir_sync.rs b/src/storage/dir_sync.rs index 21f9d8d9c..65edc6fae 100644 --- a/src/storage/dir_sync.rs +++ b/src/storage/dir_sync.rs @@ -84,7 +84,8 @@ pub(crate) fn take_sync_log() -> Vec { SYNC_LOG.with(|log| std::mem::take(&mut *log.borrow_mut())) } -#[cfg(test)] +// The tests use `tempfile`, a native-only dev-dependency. +#[cfg(all(test, not(target_arch = "wasm32")))] mod tests { use super::*; diff --git a/src/storage/persistent_facts.rs b/src/storage/persistent_facts.rs index eccbf0ae8..4fdc4a333 100644 --- a/src/storage/persistent_facts.rs +++ b/src/storage/persistent_facts.rs @@ -1084,17 +1084,6 @@ impl PersistentFactStorage { self.dirty } - /// Capacity (in pages) of the internal LRU page cache passed to `new()`. - /// - /// Exposed only for the browser WASM layer's tests, which verify that - /// `BrowserDb` constructors pass `0` (cache disabled — see #275) since - /// `BrowserBufferBackend` is already an in-memory page store and the LRU - /// layer on top of it would be redundant. - #[cfg(all(target_arch = "wasm32", feature = "browser", test))] - pub(crate) fn page_cache_capacity(&self) -> usize { - self.page_cache.capacity() - } - /// Run a closure with read access to the underlying storage backend. /// /// Used by the browser WASM layer to read pages after `save()` without