From 11bb7b3de6feb1cc5fb81f147d9fdfe1bb405468 Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Sat, 24 Oct 2026 13:12:47 +0530 Subject: [PATCH 1/2] fix(db): calls inside a WriteTransaction return INT-001 instead of deadlocking (#734) checkpoint, verify and rebuild_indexes locked the write lock their own thread's WriteTransaction held, and hung. They now check, as execute, query, fact_log and begin_write do; prepare and PreparedQuery::execute/ query get the same check for consistency. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015NBT2Yx9JUQKYvv2CvC9py --- CHANGELOG.md | 1 + CLAUDE.md | 2 +- docs/ERROR_REFERENCE.md | 11 ++-- docs/TEST_COVERAGE.md | 5 +- src/db.rs | 15 +++++- src/query/datalog/prepared.rs | 8 ++- tests/write_tx_guard_test.rs | 94 +++++++++++++++++++++++++++++++++++ 7 files changed, 127 insertions(+), 9 deletions(-) create mode 100644 tests/write_tx_guard_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ca142f69..01a09b523 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`checkpoint`, `verify` and `rebuild_indexes` inside a `WriteTransaction` return `INT-001` instead of deadlocking** (#734). Called on a thread that holds a `WriteTransaction`, they waited on the write lock that the transaction already held, so the thread hung. They now return `INT-001`, as `execute`, `query`, `fact_log` and `begin_write` already did, and the transaction stays usable. `prepare` and `PreparedQuery::execute`/`query` make the same check; they could not deadlock, but they ran inside the transaction where `execute`/`query` refused. `checkpoint` deadlocked in every release since `WriteTransaction` was added; the fix for it and for the prepared calls ships in v2.x too. - **A float constant in a pattern matches equal floats only** (#706). A float written in a pattern's value position, or bound to a `$slot` of a prepared query, matched every stored float within an absolute `f64::EPSILON` (about `2.2e-16`), so `[?e :m/x 0.0]` returned an entity whose `:m/x` is `1e-17`, while `[?e :m/x ?v] [(= ?v 0.0)]` did not. Above 1.0 the epsilon is smaller than the gap between adjacent floats, so only floats smaller than about `2.2e-16` in magnitude were affected: every one of them matched `0.0`, and `1e-20` matched `5e-20`. Float constants now compare by IEEE `==`, as joins, predicates and the indexes do: `0.0` matches `-0.0` and nothing else changes. The fix covers patterns in queries, rule bodies and `not` / `or` bodies, on the executor and the streaming engine. Nothing stored was wrong. The same fix ships in v2.0.10. - **WAL entries of more than 1,000,000 facts replay** (#686). A WAL entry of more than 1,000,000 facts was taken as damage: replay stopped there, and the next open cut the WAL off at that entry, so after a crash, kill or close without a checkpoint that transaction and every later one in the WAL were lost, though each had committed. The writer had no such limit. Since #550 no v3 transaction writes such an entry, but a WAL left by v2.x can hold one. Replay now takes an entry's fact count as damage only when the bytes after it cannot hold that many facts. Affected every release since v0.5.0. Also fixed in v2.0.10. - **A recursive rule query no longer joins a rule's new facts with every magic-set fact** (#694). On the engine (#548), each round after the first runs a recursive rule from the facts the last round derived, then joins the rest of its body. The rest kept the order the planner chose for the whole body, so the next clause could share no variable with the new facts: after the magic-set rewrite of `(reach :n0 ?to)`, the body `(magic ?from) [?from :next ?mid] (reach ?mid ?to)` joined each new `reach` fact with every `magic` fact, then probed `:next` once per pair. The rest of the body is now planned with the new facts' variables bound, each next pattern sharing a variable with those bound before it when one can. Answers are unchanged. Nightly `recursion` benchmarks (bench profile): `fanout/w10_d3` 4.94 s → 22.6 ms (it was about 94 ms on the executor before #548), `chain/depth_100` 699 ms → 41.5 ms, `chain/depth_10` 1.15 ms → 0.68 ms. diff --git a/CLAUDE.md b/CLAUDE.md index a60373b3e..d2effebaa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -212,7 +212,7 @@ Auto-migrates v7 → v8 on open (spec §9, with a backup meta page). v1–v6 are ## Test Coverage -**1716 tests** (1703 passing, 13 ignored; unit + integration + doc). Engine answers once checked by the executor are frozen in `tests/answers/` (#551). +**1723 tests** (1710 passing, 13 ignored; unit + integration + doc). Engine answers once checked by the executor are frozen in `tests/answers/` (#551). See `docs/TEST_COVERAGE.md` for the full per-file breakdown. **Testing conventions** — see the Testing Conventions section below before writing any tests. diff --git a/docs/ERROR_REFERENCE.md b/docs/ERROR_REFERENCE.md index 8bd05bbad..fdb5b79a7 100644 --- a/docs/ERROR_REFERENCE.md +++ b/docs/ERROR_REFERENCE.md @@ -3414,10 +3414,13 @@ specific code, so this should now be rare. **Error text**: `a WriteTransaction is already in progress on this thread; use tx.execute() instead` -**Cause**: `Minigraf::execute()` or `Minigraf::begin_write()` was called on -the same thread that already holds an active `WriteTransaction`. Acquiring -the write lock again on that thread would deadlock, so this is detected and -rejected instead. +**Cause**: a `Minigraf` method was called on the same thread that already +holds an active `WriteTransaction`: `execute()`, `query()`, `fact_log()`, +`begin_write()`, `checkpoint()`, `verify()`, `rebuild_indexes()`, +`prepare()`, or `PreparedQuery::execute()`/`query()`. Acquiring the write +lock again on that thread would deadlock, so this is detected and rejected +instead. (Before v3.0.0, `checkpoint()`, `verify()` and `rebuild_indexes()` +did not check and deadlocked, #734.) **Resolution**: - Use the existing `WriteTransaction`'s `tx.execute()` for further writes diff --git a/docs/TEST_COVERAGE.md b/docs/TEST_COVERAGE.md index 8de85e9b2..e0de1795a 100644 --- a/docs/TEST_COVERAGE.md +++ b/docs/TEST_COVERAGE.md @@ -4,9 +4,9 @@ | | Default features | With `--features test-hooks` | |---|---|---| -| **Passing** | 1,703 | 1,719 | +| **Passing** | 1,710 | 1,726 | | **Ignored** | 13 | 14 | -| **Total** | 1,716 | 1,733 | +| **Total** | 1,723 | 1,740 | **Verified**: 2026-10-24 with `cargo test --quiet`. @@ -281,6 +281,7 @@ These tests compare the database against an independent answer rather than a sin - The iterator, and aggregates, rules, `:as-of` and `:any-valid-time`, match `execute()`. - A cursor opened before a `transact`, a `WriteTransaction` commit, a retract or a checkpoint returns the pre-write answer to the end, in memory and file-backed. - `transact`/`retract`/`rule` are `API-012`; bind slots are `API-010`; a `WriteTransaction` thread gets `INT-001`; rule limits give `INT-020` as from `execute()`. + - `tests/write_tx_guard_test.rs` (#734): `checkpoint`, `verify`, `rebuild_indexes`, `prepare` and `PreparedQuery::execute`/`query` on a thread holding a `WriteTransaction` return `INT-001` (each under a 30 s timeout, so a deadlock fails instead of hanging), and the transaction then commits; outside a transaction they run. - `PreparedQuery::query` matches `PreparedQuery::execute`. - A cursor is `Send` and outlives its database handle. - **Fact log** (#430, `tests/fact_log_test.rs`) diff --git a/src/db.rs b/src/db.rs index 8aac4bcea..185a0b7c7 100644 --- a/src/db.rs +++ b/src/db.rs @@ -56,7 +56,7 @@ fn set_write_tx_active(val: bool) { WRITE_TX_ACTIVE.with(|f| f.set(val)); } -fn is_write_tx_active() -> bool { +pub(crate) fn is_write_tx_active() -> bool { WRITE_TX_ACTIVE.with(|f| f.get()) } @@ -1100,6 +1100,9 @@ impl Minigraf { } fn checkpoint_inner(&self) -> Result<()> { + if is_write_tx_active() { + bail_coded!(ErrorCode::Int001); + } self.reject_if_read_only("checkpoint")?; let mut ctx = self .inner @@ -1136,6 +1139,9 @@ impl Minigraf { } fn verify_inner(&self) -> Result { + if is_write_tx_active() { + bail_coded!(ErrorCode::Int001); + } let ctx = self .inner .write_lock @@ -1181,6 +1187,9 @@ impl Minigraf { } fn rebuild_indexes_inner(&self) -> Result<()> { + if is_write_tx_active() { + bail_coded!(ErrorCode::Int001); + } self.reject_if_read_only("rebuild_indexes")?; let mut ctx = self .inner @@ -1332,6 +1341,10 @@ impl Minigraf { ) -> Result { use crate::query::datalog::prepared::prepare_query; + if is_write_tx_active() { + bail_coded!(ErrorCode::Int001); + } + let cmd = parse_datalog_command(query_str)?; let query = match cmd { diff --git a/src/query/datalog/prepared.rs b/src/query/datalog/prepared.rs index 5108230b7..555b73ed9 100644 --- a/src/query/datalog/prepared.rs +++ b/src/query/datalog/prepared.rs @@ -1,5 +1,5 @@ use crate::cursor::Cursor; -use crate::error::{ErrorCode, MinigrafError, bail_coded}; +use crate::error::{ErrorCode, MinigrafError, bail_coded, err_coded}; use crate::graph::FactStorage; use crate::graph::storage::Snapshot; use crate::graph::types::Value; @@ -104,6 +104,9 @@ impl PreparedQuery { /// - Missing bind value for a slot present in the query. /// - Type mismatch (e.g. `Val` supplied for an `:as-of` slot). pub fn execute(&self, bindings: &[(&str, BindValue)]) -> Result { + if crate::db::is_write_tx_active() { + return Err(err_coded!(ErrorCode::Int001).into()); + } self.fact_storage .snapshot() .and_then(|snapshot| self.execute_inner(bindings, snapshot)) @@ -118,6 +121,9 @@ impl PreparedQuery { /// # Errors /// The same as [`PreparedQuery::execute`]. pub fn query(&self, bindings: &[(&str, BindValue)]) -> Result { + if crate::db::is_write_tx_active() { + return Err(err_coded!(ErrorCode::Int001).into()); + } let run = || { let snapshot = self.fact_storage.snapshot()?; let (executor, command) = self.bind(bindings, snapshot.clone())?; diff --git a/tests/write_tx_guard_test.rs b/tests/write_tx_guard_test.rs new file mode 100644 index 000000000..627a0b3c7 --- /dev/null +++ b/tests/write_tx_guard_test.rs @@ -0,0 +1,94 @@ +// File-backed (Minigraf::open, tempfile): native only, like the other file tests. +#![cfg(not(target_arch = "wasm32"))] +//! A database call on a thread that holds a `WriteTransaction` returns +//! `INT-001` instead of waiting on the transaction's own write lock (#734). + +use minigraf::{Minigraf, PreparedQuery}; +use std::sync::mpsc; +use std::thread; +use std::time::Duration; + +/// Runs `call` on a fresh thread holding a `WriteTransaction` on a file +/// database, and returns the error code `call` gave, or `None` if it +/// succeeded. A call that deadlocks fails the test after 30 s instead of +/// hanging it. The transaction must still commit after the call. +fn code_inside_tx( + call: fn(&Minigraf, &PreparedQuery) -> Option<&'static str>, +) -> Option<&'static str> { + let (tx, rx) = mpsc::channel(); + thread::spawn(move || { + let dir = tempfile::tempdir().unwrap(); + let db = Minigraf::open(dir.path().join("g.graph")).unwrap(); + db.execute(r#"(transact [[:alice :person/name "Alice"]])"#) + .unwrap(); + let prepared = db + .prepare("(query [:find ?n :where [?e :person/name ?n]])") + .unwrap(); + let mut wtx = db.begin_write().unwrap(); + wtx.execute(r#"(transact [[:bob :person/name "Bob"]])"#) + .unwrap(); + let code = call(&db, &prepared); + wtx.commit() + .expect("the transaction commits after the refusal"); + tx.send(code).unwrap(); + }); + rx.recv_timeout(Duration::from_secs(30)) + .expect("the call returned instead of deadlocking") +} + +#[test] +fn checkpoint_inside_write_tx_is_int001() { + let code = code_inside_tx(|db, _| db.checkpoint().err().map(|e| e.code())); + assert_eq!(code, Some("INT-001")); +} + +#[test] +fn verify_inside_write_tx_is_int001() { + let code = code_inside_tx(|db, _| db.verify().err().map(|e| e.code())); + assert_eq!(code, Some("INT-001")); +} + +#[test] +fn rebuild_indexes_inside_write_tx_is_int001() { + let code = code_inside_tx(|db, _| db.rebuild_indexes().err().map(|e| e.code())); + assert_eq!(code, Some("INT-001")); +} + +#[test] +fn prepare_inside_write_tx_is_int001() { + let code = code_inside_tx(|db, _| { + db.prepare("(query [:find ?n :where [?e :person/name ?n]])") + .err() + .map(|e| e.code()) + }); + assert_eq!(code, Some("INT-001")); +} + +#[test] +fn prepared_execute_inside_write_tx_is_int001() { + let code = code_inside_tx(|_, p| p.execute(&[]).err().map(|e| e.code())); + assert_eq!(code, Some("INT-001")); +} + +#[test] +fn prepared_query_inside_write_tx_is_int001() { + let code = code_inside_tx(|_, p| p.query(&[]).err().map(|e| e.code())); + assert_eq!(code, Some("INT-001")); +} + +/// Outside a transaction the same calls still run. +#[test] +fn guarded_calls_run_outside_a_write_tx() { + let dir = tempfile::tempdir().unwrap(); + let db = Minigraf::open(dir.path().join("g.graph")).unwrap(); + db.execute(r#"(transact [[:alice :person/name "Alice"]])"#) + .unwrap(); + db.checkpoint().unwrap(); + db.verify().unwrap(); + db.rebuild_indexes().unwrap(); + let prepared = db + .prepare("(query [:find ?n :where [?e :person/name ?n]])") + .unwrap(); + prepared.execute(&[]).unwrap(); + assert_eq!(prepared.query(&[]).unwrap().count(), 1, "one row"); +} From 073b23418b6201401a173a9531754610b0d58800 Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Sat, 24 Oct 2026 13:13:47 +0530 Subject: [PATCH 2/2] docs(#734): INT-001 names PreparedQuery's methods apart; note the blocked thread Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015NBT2Yx9JUQKYvv2CvC9py --- docs/ERROR_REFERENCE.md | 9 +++++---- tests/write_tx_guard_test.rs | 3 ++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/ERROR_REFERENCE.md b/docs/ERROR_REFERENCE.md index fdb5b79a7..0fd88f757 100644 --- a/docs/ERROR_REFERENCE.md +++ b/docs/ERROR_REFERENCE.md @@ -3414,10 +3414,11 @@ specific code, so this should now be rare. **Error text**: `a WriteTransaction is already in progress on this thread; use tx.execute() instead` -**Cause**: a `Minigraf` method was called on the same thread that already -holds an active `WriteTransaction`: `execute()`, `query()`, `fact_log()`, -`begin_write()`, `checkpoint()`, `verify()`, `rebuild_indexes()`, -`prepare()`, or `PreparedQuery::execute()`/`query()`. Acquiring the write +**Cause**: on the same thread that already holds an active +`WriteTransaction`, one of these was called: the `Minigraf` methods +`execute()`, `query()`, `fact_log()`, `begin_write()`, `checkpoint()`, +`verify()`, `rebuild_indexes()` or `prepare()`, or a `PreparedQuery`'s +`execute()` or `query()`. Acquiring the write lock again on that thread would deadlock, so this is detected and rejected instead. (Before v3.0.0, `checkpoint()`, `verify()` and `rebuild_indexes()` did not check and deadlocked, #734.) diff --git a/tests/write_tx_guard_test.rs b/tests/write_tx_guard_test.rs index 627a0b3c7..9d05594df 100644 --- a/tests/write_tx_guard_test.rs +++ b/tests/write_tx_guard_test.rs @@ -11,7 +11,8 @@ use std::time::Duration; /// Runs `call` on a fresh thread holding a `WriteTransaction` on a file /// database, and returns the error code `call` gave, or `None` if it /// succeeded. A call that deadlocks fails the test after 30 s instead of -/// hanging it. The transaction must still commit after the call. +/// hanging it. The transaction must still commit after the call. A +/// deadlocked thread is left blocked until the test process exits. fn code_inside_tx( call: fn(&Minigraf, &PreparedQuery) -> Option<&'static str>, ) -> Option<&'static str> {