diff --git a/CHANGELOG.md b/CHANGELOG.md index 81e7b3e15..8680a38fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Bug fixes +- **Two handles on one file in the same process** (#304): `FileLock::acquire` treated a lock file whose holder PID equalled our own as stale, deleted it, and opened anyway — on the theory that it could only be a leaked handle. The far more likely cause is a handle that is open and in use right now elsewhere in the process, so the "self-heal" silently produced two live `FileBackend`s on one file. Each caches its own `header.page_count`, allocates new pages from that count, and bounds-checks `read_page` against it, so the two page tables diverge. That — not an allocation off-by-one — is the source of the intermittent `Page N out of bounds (total pages: M)`. It also corrupted the lock itself: whichever handle dropped first ran `FileLock::drop` and removed the lock file that by then belonged to the survivor, leaving a live handle unlocked and admitting other processes too. A lock held by our own PID is now refused with an error naming the same-process case; a lock held by a *different*, dead process is still reclaimed as before. **Behaviour change:** callers that relied on reopening a path they already hold open now get an error instead of silent corruption, and should reuse the existing handle (`Minigraf` is cheap to clone and all clones share one database). + - **String comparison in predicates**: `[(< ?a ?b)]`, `>`, `<=` and `>=` now order two strings lexicographically instead of failing. Previously every ordering predicate routed through `to_float_pair`, which errors on non-numeric operands, so a string comparison silently matched no rows — and because both directions returned nothing, a range filter dropped the entire relation rather than partitioning it. This most often bit ISO-8601 timestamps stored as strings. The predicate path now agrees with `value_lt` / `value_cmp`, which `min`, `max` and `:order-by` already use. Mixed-type comparison (string vs number) remains an error, and NaN comparisons remain false. ## v1.2.1 — 2026-06-28 diff --git a/src/storage/backend/file.rs b/src/storage/backend/file.rs index f03944b3c..08a088257 100644 --- a/src/storage/backend/file.rs +++ b/src/storage/backend/file.rs @@ -17,9 +17,23 @@ impl FileLock { /// Attempt to acquire an exclusive lock for the given database path. /// Returns `Err` if another process already holds the lock. /// - /// If a stale lock file exists (the holder PID is no longer running), - /// it is automatically removed and a new lock is acquired. This handles - /// the case where the previous process crashed without cleaning up. + /// If a stale lock file exists (the holder is a *different* process that is + /// no longer running), it is automatically removed and a new lock is + /// acquired. This handles the case where the previous process crashed + /// without cleaning up. + /// + /// A lock held by our OWN pid is never treated as stale. It used to be, on + /// the theory that it could only be a leaked handle — but the overwhelmingly + /// more likely explanation is a handle that is open and in use right now, + /// somewhere else in this process. Stealing it produced two live + /// `FileBackend`s on one file, each caching its own `header.page_count`, + /// each allocating new pages from that stale count and bounds-checking + /// `read_page` against it. That is the source of the intermittent + /// "Page N out of bounds (total pages: M)" in #304: not an allocation + /// off-by-one, but two page tables diverging. It also corrupted the lock + /// itself — whichever handle dropped first ran `FileLock::drop` and deleted + /// the lock file belonging to the survivor, leaving a live handle unlocked + /// and letting other processes in too. fn acquire(db_path: &Path) -> Result { let lock_path = db_path.with_extension("graph.lock"); // create_new fails with AlreadyExists if the lock file is present @@ -38,9 +52,23 @@ impl FileLock { let holder = std::fs::read_to_string(&lock_path).unwrap_or_default(); if let Ok(pid) = holder.trim().parse::() { let our_pid = std::process::id(); - if pid == our_pid || !Self::is_process_alive(pid) { - // Stale lock — either our own leaked handle or previous - // process crashed. Remove and retry. + if pid == our_pid { + // A handle in THIS process holds the lock. Say so + // explicitly: the generic "another process" wording sent + // #304 looking for a second process for a long time. + anyhow::bail!( + "Database is already open in this process (lock file: {}, \ + holder PID: {} == our own). A second handle on one file \ + would give each its own page table and corrupt both — \ + reuse the existing handle instead. `Minigraf` is cheap to \ + clone and all clones share the same database.", + lock_path.display(), + pid + ); + } + if !Self::is_process_alive(pid) { + // Stale lock — the holding process died without cleaning + // up. Remove and retry. let _ = std::fs::remove_file(&lock_path); return Self::acquire(db_path); } @@ -278,6 +306,90 @@ impl StorageBackend for FileBackend { mod tests { use super::*; + /// #304: a second handle on a file this process already has open must be + /// refused, not granted by deleting our own lock file. + /// + /// Before the fix `acquire` treated `pid == our_pid` as a stale lock, so + /// this second open succeeded and the two backends diverged: each caches + /// its own `header.page_count`, allocates new pages from it, and + /// bounds-checks `read_page` against it. + #[test] + fn test_second_open_in_same_process_is_refused() { + let dir = tempfile::tempdir().unwrap(); + let temp_path = dir.path().join("test_minigraf_self_steal.graph"); + + let first = FileBackend::open(&temp_path).unwrap(); + + // FileBackend is not Debug, so match on the Result rather than + // expect_err. + let msg = match FileBackend::open(&temp_path) { + Ok(_) => panic!("a second handle in the same process must be refused"), + Err(e) => e.to_string(), + }; + assert!( + msg.contains("already open in this process"), + "error should name the same-process case, got: {msg}" + ); + + // The refusal must not have removed the lock the first handle holds. + let lock_path = temp_path.with_extension("graph.lock"); + assert!( + lock_path.exists(), + "a refused acquire must leave the existing lock in place" + ); + drop(first); + assert!( + !lock_path.exists(), + "dropping the only handle should release the lock" + ); + } + + /// The refusal must not regress the sequential open/drop/reopen cycle that + /// `temporal_reasoning` relies on between commits. + #[test] + fn test_reopen_after_drop_still_works() { + let dir = tempfile::tempdir().unwrap(); + let temp_path = dir.path().join("test_minigraf_reopen.graph"); + + for _ in 0..3 { + let backend = FileBackend::open(&temp_path).unwrap(); + drop(backend); + } + assert!(FileBackend::open(&temp_path).is_ok()); + } + + /// A lock left behind by a *different*, dead process is still stale and + /// must still be self-healed — that behaviour is what the `pid == our_pid` + /// branch was overreaching from, and it must survive the fix. + /// + /// procfs-only: without `/proc`, `is_process_alive` cannot distinguish a + /// dead PID from a live one and deliberately errs towards "alive" (see its + /// own comment), so reclamation is a Linux/Android guarantee only. On other + /// platforms a stale lock must still be removed by hand. + #[cfg(any(target_os = "linux", target_os = "android"))] + #[test] + fn test_dead_other_process_lock_is_still_reclaimed() { + let dir = tempfile::tempdir().unwrap(); + let temp_path = dir.path().join("test_minigraf_stale.graph"); + let lock_path = temp_path.with_extension("graph.lock"); + + // PID 1 is always alive; instead spawn a real child and let it exit so + // we have a genuinely dead PID that is not us. + let child = std::process::Command::new("true").spawn().unwrap(); + let dead_pid = child.id(); + let mut child = child; + child.wait().unwrap(); + + std::fs::write(&lock_path, dead_pid.to_string()).unwrap(); + + let backend = FileBackend::open(&temp_path); + assert!( + backend.is_ok(), + "a dead other process's lock must still be reclaimed: {:?}", + backend.err() + ); + } + #[test] fn test_file_backend_create() { let dir = tempfile::tempdir().unwrap(); diff --git a/tests/migration_matrix_test.rs b/tests/migration_matrix_test.rs index 55b0cf371..7ecda2c29 100644 --- a/tests/migration_matrix_test.rs +++ b/tests/migration_matrix_test.rs @@ -96,8 +96,17 @@ fn wal_replay_after_migration_is_idempotent() { { let db = Minigraf::open(&path).unwrap(); db.execute(r#"(transact [[:e2 :color "blue"]])"#).unwrap(); + // Simulate a crash: no Drop, so no checkpoint, and the WAL is left for + // the next open to replay. std::mem::forget(db); } + // mem::forget also skips FileLock::drop, leaving the sidecar lock behind + // holding OUR pid. A lock held by a live pid means a handle that is open + // right now, so reopening is correctly refused (#304) -- but that is not + // what a crash leaves behind. Clear it, which models the state the next + // open finds once a crashed holder is cleaned up. The WAL-replay assertion + // below is unchanged. + std::fs::remove_file(path.with_extension("graph.lock")).unwrap(); let db3 = Minigraf::open(&path).unwrap(); let n = count_results( db3.execute("(query [:find ?c :where [?e :color ?c]])") diff --git a/tests/wal_test.rs b/tests/wal_test.rs index 65e83a64e..18eb77549 100644 --- a/tests/wal_test.rs +++ b/tests/wal_test.rs @@ -87,6 +87,7 @@ fn test_wal_recovery_after_simulated_crash() { // is the only handle, forgetting it leaks the Arc permanently and // the Drop impl never runs. std::mem::forget(db); + simulate_crashed_holder(&db_path); } // WAL must still exist (no checkpoint happened) @@ -129,6 +130,7 @@ fn test_no_duplicate_facts_after_post_checkpoint_crash() { db.execute(r#"(transact [[:alice :name "Alice"]])"#) .unwrap(); std::mem::forget(db); + simulate_crashed_holder(&db_path); } // Back up the WAL before the next open checkpoints it away @@ -188,6 +190,7 @@ fn test_partial_wal_entry_discarded_earlier_entries_intact() { db.execute(r#"(transact [[:alice :name "Alice"]])"#) .unwrap(); std::mem::forget(db); + simulate_crashed_holder(&db_path); } // Append garbage bytes after the valid WAL entry (simulate partial second write) @@ -275,6 +278,7 @@ fn test_manual_checkpoint_deletes_wal() { // Simulate crash: skip Drop (checkpoint already happened, no WAL to write) std::mem::forget(db); + simulate_crashed_holder(&db_path); // Reopen: must recover the fact from the main file alone (no WAL needed) let db2 = Minigraf::open(&db_path).unwrap(); @@ -319,6 +323,7 @@ fn test_auto_checkpoint_fires_at_threshold() { ); // Crash: skip Drop checkpoint (but checkpoint already happened) std::mem::forget(db); + simulate_crashed_holder(&db_path); } // No WAL after crash @@ -367,6 +372,7 @@ fn test_explicit_tx_all_or_nothing_commit() { // Crash before Drop checkpoint std::mem::forget(db); + simulate_crashed_holder(&db_path); } // Recovery session @@ -516,6 +522,7 @@ fn test_implicit_tx_execute_survives_replay() { // Simulate crash: skip Drop (and its checkpoint). std::mem::forget(db); + simulate_crashed_holder(&db_path); } // WAL must exist — no checkpoint fired. @@ -550,6 +557,7 @@ fn setup_db_with_one_fact() -> (tempfile::TempDir, std::path::PathBuf, Vec) let db = minigraf::db::Minigraf::open(&db_path).unwrap(); db.execute(r#"(transact [[:e1 :name "Alice"]])"#).unwrap(); std::mem::forget(db); + simulate_crashed_holder(&db_path); } let wal_bytes = read_wal_bytes(&db_path); (dir, db_path, wal_bytes) @@ -604,6 +612,7 @@ fn wal_recover_bad_checksum_second_entry() { db.execute(r#"(transact [[:e1 :name "Alice"]])"#).unwrap(); db.execute(r#"(transact [[:e2 :name "Bob"]])"#).unwrap(); std::mem::forget(db); + simulate_crashed_holder(&db_path); } let mut wal_bytes = read_wal_bytes(&db_path); assert!(wal_bytes.len() > 36, "WAL too short to corrupt"); @@ -630,6 +639,7 @@ fn wal_recover_committed_tx_crash_before_checkpoint() { tx.execute(r#"(transact [[:e1 :name "Charlie"]])"#).unwrap(); tx.commit().unwrap(); std::mem::forget(db); + simulate_crashed_holder(&db_path); } let names = query_names(&db_path); assert_eq!( @@ -653,6 +663,7 @@ fn wal_recover_rollback_crash() { tx.execute(r#"(transact [[:e1 :name "Dave"]])"#).unwrap(); tx.rollback(); std::mem::forget(db); + simulate_crashed_holder(&db_path); } let names = query_names(&db_path); assert_eq!( @@ -671,6 +682,7 @@ fn wal_recover_multiple_committed_corrupt_tail() { db.execute(r#"(transact [[:e1 :name "Eve"]])"#).unwrap(); db.execute(r#"(transact [[:e2 :name "Frank"]])"#).unwrap(); std::mem::forget(db); + simulate_crashed_holder(&db_path); } let mut wal_bytes = read_wal_bytes(&db_path); wal_bytes.extend_from_slice(&[0xDE, 0xAD, 0xBE, 0xEF, 0x00, 0x00]); @@ -691,6 +703,7 @@ fn wal_corrupt_tail_never_applied() { let db = minigraf::db::Minigraf::open(&db_path).unwrap(); db.execute(r#"(transact [[:e1 :name "Grace"]])"#).unwrap(); std::mem::forget(db); + simulate_crashed_holder(&db_path); } let mut wal_bytes = read_wal_bytes(&db_path); let mut fake_entry: Vec = Vec::new(); @@ -807,3 +820,23 @@ fn test_v2_file_opens_and_upgrades_to_v3_on_checkpoint() { "last_checkpointed_tx_count must be set after checkpoint on v2→v6 upgrade" ); } + +/// Complete a `mem::forget`-based crash simulation. +/// +/// `mem::forget` skips `FileLock::drop`, so the sidecar `.graph.lock` is left +/// behind holding OUR pid. A lock held by a live pid means a handle that is +/// open right now, so reopening is correctly refused (#304) -- but that is not +/// what a crash leaves behind. A crashed process leaves the lock holding its +/// own, now-dead pid. +/// +/// Removing the lock models the state the next open actually finds after a +/// crashed holder is cleaned up, and does so on every platform. Writing a dead +/// pid instead would only work on procfs targets: `is_process_alive` cannot +/// tell dead from live without `/proc` and deliberately errs towards "alive", +/// so a dead pid still blocks on macOS and Windows. Reclamation of a dead +/// holder's lock is covered directly by +/// `storage::backend::file::tests::test_dead_other_process_lock_is_still_reclaimed`; +/// these tests are about WAL replay. +fn simulate_crashed_holder(db_path: &std::path::Path) { + let _ = std::fs::remove_file(db_path.with_extension("graph.lock")); +}