fix(graph): stop reusing a connection across independent bulk COPY writes - #68
pradeepmouli wants to merge 1 commit into
Conversation
…ites copy_edges_with_bad_record_retry took a borrowed Connection and reused it across every retry attempt and the UNWIND fallback. None of these bulk loads are wrapped in an explicit transaction, so sharing the connection bought no atomicity -- it only created exposure to whatever internal state a caught COPY failure leaves behind. Observed in production: a Symbol-table COPY's bad-PK-drop-and-retry cycle left the connection wedged such that the very next COPY on it (a different table, CALLS) failed immediately with Kuzu's internal "Invalid transaction type to rollback." and fell back to the slower per-row UNWIND path. Fix: copy_edges_with_bad_record_retry now takes &GraphStore instead of &Connection and asks for a fresh connection on every retry-loop iteration and before the UNWIND fallback -- no "is this a retry" bookkeeping needed, since GraphStore::connection() already mints a fresh, cheap Connection on every call. The inline Symbol-node COPY-with-retry block in import_scip_index (a near-duplicate of the same pattern for nodes instead of edges) gets the same treatment. Threading &GraphStore down to the one caller of this helper that didn't already have it (resolve_with_map) also let resolve_inherits drop its now- entirely-unused &Connection parameter. Self-healing today via the UNWIND fallback (byte-for-byte identical output, per store_bench::test_parquet_quality), so no data-loss exposure -- but real and reproducible. Also silences one pre-existing, unrelated clippy::chunks_exact_to_as_chunks lint in embed/mod.rs (newer clippy than this branch's baseline; the workspace-wide pre-commit hook blocks on it otherwise) -- no behavior change, matches clippy's own suggested suppression.
…ites Port of upstream intuit#68 (tracks #119). copy_edges_with_bad_record_retry took a borrowed Connection and reused it across every retry attempt and the UNWIND fallback. None of these bulk loads are wrapped in an explicit transaction, so sharing the connection bought no atomicity -- it only created exposure to whatever internal state a caught COPY failure leaves behind. Observed in production (sittir): a Symbol-table COPY's bad-PK-drop-and-retry cycle left the connection wedged such that the very next COPY on it (a different table, CALLS) failed immediately with Kuzu's internal "Invalid transaction type to rollback." and fell back to the slower per-row UNWIND path. Self-healing today via the UNWIND fallback, so no data-loss exposure -- but real and reproducible. Fix: copy_edges_with_bad_record_retry now takes &GraphStore instead of &Connection and asks for a fresh connection on every retry-loop iteration and before the UNWIND fallback -- no "is this a retry" bookkeeping needed, since GraphStore::connection() already mints a fresh, cheap Connection on every call. The inline Symbol-node COPY-with-retry block in import_scip_index (a near-duplicate of the same pattern for nodes instead of edges) gets the same treatment. Threading &GraphStore down to resolve_with_map/resolve_inherits (this fork's versions additionally thread a &WriteLock witness parameter that upstream doesn't have -- preserved as-is, not touched). Also silences two pre-existing, unrelated clippy::chunks_exact_to_as_chunks lints in embed/mod.rs (newer clippy than this branch's baseline; the workspace-wide pre-commit hook blocks on them otherwise) -- no behavior change, matches clippy's own suggested suppression.
murari316
left a comment
murari316
left a comment
There was a problem hiding this comment.
Automated review (Fable, adversarially verified). Recommendation: request-changes.
The PR fixes a real production bug: a caught bulk-COPY failure (bad-PK retry) leaves Kuzu's transaction bookkeeping on that connection wedged, so the next statement on the same connection fails with "Invalid transaction type to rollback". It threads &GraphStore into copy_edges_with_bad_record_retry and takes a fresh, cheap Connection per attempt, converting the function to return Result and adding fresh-connection points in the SCIP importer. The core change is sound and correctly scoped to write paths that already hold the WriteLock (no advisory-lock, kuzu-open-validation, or ignore-file invariants are affected), but CI is red on 2 of 3 platforms, the branch is unmergeable against current main (which refactored the CALLS write into write_resolved_calls, still using the shared-connection pattern this PR fixes), and no test covers the exact failed-COPY-then-next-COPY scenario the PR exists to fix.
[BLOCKER] test-failure — crates/infigraph-core/src/scip/mod.rs:236
CI is red on ubuntu and windows (Check jobs failed; macOS passed; run is from Aug 25, ~3 weeks stale): scip::tests::scip_parameter_descriptor_does_not_become_a_new_symbol panics at scip/mod.rs:1069. Root cause is a shared fixed temp path: import_scip_index writes new-symbol batches to std::env::temp_dir().join("infigraph_scip_symbols.parquet") (scip/mod.rs:236) before COPY. Sibling tests in the same binary (e.g. non_implementation_relationship_does_not_create_inherits_edge, scip/mod.rs:885, which imports Animal/Dog through the same path) run in parallel and overwrite the file between write and COPY, so one test bulk-loads another test's rows into its database. Same class of bug the PR is about — a shared mutable resource reused across independent bulk COPY writes — and the same fixed paths exist at scip/mod.rs:481 (infigraph_scip_calls.parquet), scip/mod.rs:549 (infigraph_scip_inherits.parquet), resolve/calls.rs:610 (infigraph_resolve_calls.parquet), and resolve/inherits.rs:186 (infigraph_resolve_inherits.parquet). Outside tests, two concurrent indexing processes can cross-contaminate each other's graph DBs the same way. Fix: use unique_tmp_dir() as store_parquet.rs already does. Repo invariant requires the full suite green across macOS/Linux/Windows.
Failure scenario: CI run 32899444920: assertion failed at scip/mod.rs:1069 — 'no node should exist for the parameter descriptor' — left: ["Animal", "Dog"], right: ["mintFn"]. stats.symbols_added==1 passes (counted in memory) but the DB contains the parallel inherits-test's Animal/Dog rows and no mintFn, exactly what a parquet overwrite between write_node_parquet and COPY produces.
[MAJOR] stale-branch — crates/infigraph-core/src/resolve/calls.rs:769
The PR is unmergeable (GitHub reports mergeable: false) because main has since refactored the CALLS write out of resolve_with_map into write_resolved_calls (calls.rs:769-791 on main), which still calls copy_edges_with_bad_record_retry(conn, ...) on the shared connection and then reuses that same conn for write_external_calls. The PR's calls.rs hunks conflict structurally with main. On rebase the compiler will force a signature fix, but the author must thread &GraphStore into write_resolved_calls (and resolve_pairs callers) so the fresh-connection fix actually lands in main's newer code path — otherwise the exact wedge bug this PR fixes (Symbol COPY bad-PK retry wedging the connection used by the following CALLS COPY / external-calls write) survives.
Failure scenario: Rebase onto current main: calls.rs hunks conflict; if resolved by keeping main's write_resolved_calls with its &Connection parameter and passing a shared conn through, a caught COPY CALLS failure inside write_resolved_calls wedges the connection that write_external_calls uses next, reproducing the production 'Invalid transaction type to rollback' failure on the new path.
[MAJOR] test-coverage — crates/infigraph-core/src/graph/store_util.rs:187
No test covers the exact scenario the PR fixes: a caught bad-PK COPY failure followed by an independent bulk COPY on the same write path. The diff touches zero test files. The scip test module already has TestEnv infrastructure that makes this cheap: seed a Symbol table, call copy_edges_with_bad_record_retry with a batch containing an endpoint id that does not exist (forces the caught-failure retry path on one connection), then run a second table's COPY through the same path and assert its edges land. That directly encodes the production sequence (Symbol-table bad-PK retries → CALLS-table COPY) and would regress loudly if someone reintroduces a shared connection.
Failure scenario: A future refactor reintroduces a long-lived shared connection into copy_edges_with_bad_record_retry (e.g. for perf, caching Connection::new). All existing tests pass because none exercises failed-COPY-then-next-COPY on one connection; the 'Invalid transaction type to rollback' wedge silently returns and is only caught in production.
[MINOR] concurrency — crates/infigraph-core/src/scip/mod.rs:380
PLAUSIBLE (no direct evidence UNWIND failures wedge — only COPY is confirmed): the PR adds a fresh connection before the enrichment UNWIND loop (scip/mod.rs:380-383 at head) and before the Symbol UNWIND fallback loop (scip/mod.rs:349-352), but each loop then reuses that one connection across all chunks with errors swallowed via let _ = conn.query(...). By the PR's own wedge model — 'a caught failure can leave the connection wedged for whatever query runs next on that same connection' — a failed chunk k can silently poison chunks k+1..N. Either take a fresh connection per chunk on failure, or stop swallowing the error.
Failure scenario: During SCIP import of a large index, enrichment chunk 3 of 40 fails (e.g. transient Kuzu error) and wedges the shared connection; chunks 4-40's UNWIND SET queries all fail silently, leaving ~92% of docstring enrichments missing with no error surfaced — stats still report symbols_enriched as if they were written.
[NIT] error-handling — crates/infigraph-core/src/graph/store_util.rs:204
Non-blocking: on the new early-error paths, the temp parquet file leaks. If store.connection()? fails on attempt N>0 (after write_edge_parquet already wrote edge_pq on a prior attempt), or before the UNWIND fallback (new let conn = store.connection()?; after the loop), the function returns Err without the std::fs::remove_file(edge_pq) cleanup that every other exit performs.
Failure scenario: Connection creation fails under resource pressure mid-retry; infigraph_index_calls.parquet is left in the temp dir. Mostly cosmetic, but with the fixed-name temp paths from the blocker finding, a stale leftover file can also be picked up by a later COPY.
[NIT] api-design — crates/infigraph-core/src/graph/store_parquet.rs:196
Non-blocking footgun, not a bug today: upsert_all_parquet_conn(&self, conn, ...) now performs node COPYs on the caller-supplied conn but edge COPYs on fresh connections from self. Verified the only caller (upsert_all_parquet, store_parquet.rs:192) passes self.connection(), so both target the same DB — but the signature now silently permits a caller to pass a connection to a different database and split nodes and edges across two DBs. Consider dropping the conn parameter (derive it from self) or documenting that conn must belong to self. Separately, the embed/mod.rs clippy allow is unrelated scope creep (CI's clippy accepts the lint name — macOS Check passed clippy — so it is not a CI risk, just worth a mention in the commit message).
Failure scenario: A future multi-repo/combined-graph caller passes a combined-DB connection with a per-repo GraphStore as self: Module/File/Symbol nodes land in the combined DB while CALLS/INHERITS/etc. edges land in the per-repo DB, producing a graph with orphaned edges that fail bad-PK retries en masse.
Generated with Claude Code (Fable). Findings verified by an adversarial refute pass; false positives removed.
2 participants
Fixes #67 (Bug 1 of 2 described there — the connection-reuse wedging).
What
copy_edges_with_bad_record_retrytook a borrowedConnectionand reused it across every retry attempt and theUNWINDfallback. None of these bulk loads are wrapped in an explicit transaction, so sharing the connection bought no atomicity — it only created exposure to whatever internal state a caughtCOPYfailure leaves behind.Observed in production: a
Symbol-tableCOPY's bad-PK-drop-and-retry cycle left the connection wedged such that the very nextCOPYon it (a different table,CALLS) failed immediately with Kùzu's internalInvalid transaction type to rollback.and fell back to the slower per-rowUNWINDpath. Self-healing (identical output either way, perstore_bench::test_parquet_quality), so no data-loss exposure — but real and reproducible.Fix
copy_edges_with_bad_record_retrynow takes&GraphStoreinstead of&Connectionand asks for a fresh connection on every retry-loop iteration and before theUNWINDfallback — no "is this a retry" bookkeeping needed, sinceGraphStore::connection()already mints a fresh, cheapConnectionon every call. The inline Symbol-nodeCOPY-with-retry block inimport_scip_index(a near-duplicate of the same pattern for nodes instead of edges) gets the same treatment.Threading
&GraphStoredown to the one caller of this helper that didn't already have it (resolve_with_map) also letresolve_inheritsdrop its now-entirely-unused&Connectionparameter.Also silences one pre-existing, unrelated
clippy::chunks_exact_to_as_chunkslint inembed/mod.rsthat the workspace-wide pre-commit hook caught (newer clippy than this branch's last-tested baseline) — no behavior change, matches clippy's own suggested suppression.Testing
cargo test -p infigraph-core --lib(310 tests, all pass)cargo test -p infigraph-core --test resolve_calls(15 tests, all pass — exercisesresolve_inherits/resolve_with_map)cargo test -p infigraph-core --test graph_queries(_conn-suffixed tests, all pass)cargo fmt --all -- --checkandcargo clippy --all-targets -- -D warningsboth cleanwrite_lock_perf,groups_watch_perf,index_perf) all passBug 2 from #67 (the
raw_querytransaction no-op silently breakingwrite_concerns/reflectionatomicity) is a separate, larger change (introducingGraphStore::transaction()) and will follow as its own PR.