Connection/transaction lifecycle bugs in KuzuBackend: wedged COPY retries + silently non-atomic writes
- Dominant language
- Rust
- Stars
- 88
- Forks
- 16
- PR merge metrics
- No merged PRs in 30d
Description
## Summary
Two real bugs in the Kùzu local backend share one root cause: `GraphStore` hands out bare `Connection` values and leaves every caller to manage connection lifetime and transaction boundaries by hand. Some callers reuse one connection across independent statements that never needed to share state (wedging a later, unrelated write); another set of callers believes it's inside a real transaction when it isn't, because of how `raw_query` handles transaction-control statements.
## Bug 1: connection reuse wedges a later, unrelated COPY
**Observed:** a `COPY Symbol` bulk load retries a few times after bad-primary-key errors (expected — drops the offending rows, retries). Immediately after, on the *same* connection, a `COPY CALLS` bulk load fails on its first attempt with Kùzu's internal `Invalid transaction type to rollback.` and falls back to the slower per-row `UNWIND` path. Self-healing (UNWIND produces correct data), but real and reproducible.
**Root cause:** `import_scip_index` (`crates/infigraph-core/src/scip/mod.rs`) opens one `Connection` at the top of the function and reuses it across an inline Symbol-node `COPY` block, then `copy_edges_with_bad_record_retry` (`crates/infigraph-core/src/graph/store_util.rs`) for `CALLS` edges, then again for `INHERITS` edges. None of these three bulk loads are wrapped in an explicit `BEGIN TRANSACTION` — each `COPY` already auto-commits independently regardless of connection sharing, so sharing the connection buys zero atomicity here while creating exposure to whatever internal state a caught COPY failure leaves behind. Same helper is also called from `resolve/calls.rs` and `store_parquet.rs`'s full-reindex bulk path.
**Proposed fix:** `copy_edges_with_bad_record_retry` should take `&GraphStore` instead of `&Connection`, calling `store.connection()?` fresh and unconditionally on every retry-loop iteration and before the `UNWIND` fallback — no "is this a retry" bookkeeping needed, since `GraphStore::connection()` already mints a fresh `Connection::new(&self.db)` cheaply on every call. The inline Symbol-`COPY` block in `import_scip_index` needs the same treatment.
## Bug 2: `raw_query`'s transaction no-op silently breaks real atomicity elsewhere
`KuzuBackend::raw_query` opens a fresh connection per call, so it deliberately no-ops `BEGIN TRANSACTION`/`BEGIN`/`COMMIT`/`ROLLBACK` when passed as a query string (intentional — fails safe rather than erroring on a stray `COMMIT`). Two call sites don't know this and rely on it for real atomicity:
- `concerns/mod.rs::write_concerns`: `BEGIN TRANSACTION`, `DETACH DELETE` all existing `Concern` nodes, loop `CREATE`-ing new ones, `COMMIT` — believing this is one atomic unit.
- `reflection/mod.rs`: same shape.
Because `raw_query`'s `BEGIN`/`COMMIT` are no-ops, every statement in these loops auto-commits independently and immediately. **A crash mid-loop today already means the old data is gone and only part of the new data landed** — live data-loss exposure, currently masked because the no-op returns `Ok` instead of erroring loudly.
**Proposed fix:** introduce `GraphStore::transaction(&self, f: impl FnOnce(&Connection) -> Result) -> Result` — opens one connection, issues a real `BEGIN TRANSACTION`, runs the closure against that connection, commits on `Ok`, rolls back on `Err`. Migrate `write_concerns`, `reflection/mod.rs`'s equivalent, `KuzuBackend::write_calls_service_edges` (already correct today but duplicates the boilerplate), and the two hand-rolled transaction blocks in `KuzuBackend`'s bulk-index path onto it.
## Suggested tests
- Force a bad-PK retry on one table's COPY immediately followed by a COPY on a different table on what would previously have been the same connection — assert the second COPY succeeds cleanly.
- Inject a mid-loop failure into `write_concerns` and assert old `Concern` nodes are still present afterward (this should *fail* against current code, proving the live bug).
- `transaction()` itself: commit-on-success, rollback-on-error, and no stuck `BEGIN` on panic.
Happy to open a PR for this if useful.
Contributor guide
Research direction
Start with GraphStore and copy_edges_with_bad_record_retry in crates/infigraph-core/src/graph/store_util.rs, then trace its callers in crates/infigraph-core/src/scip/mod.rs, resolve/calls.rs, and store_parquet.rs. Read write_concerns in concerns/mod.rs and the equivalent reflection path to compare their transaction handling. Done means COPY retries use fresh connections, transaction blocks commit or roll back atomically, and the suggested retry, rollback, and panic tests pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100