adityamukho · Aug 10, 2026 · Aug 9, 2026 · Aug 9, 2026
@@ -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

@@ -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<Self> {
         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::<u32>() {
                     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();

@@ -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]])")

@@ -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<u8>)
         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<u8> = 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"));
+}