Summary
GraphStore.store_file_nodes_edges in code_review_graph/graph.py issues BEGIN IMMEDIATE without first checking whether the connection is already inside an implicit transaction. Python's sqlite3 module under its default deferred isolation auto-starts a transaction on the first DML statement, so any prior write on the same connection makes BEGIN IMMEDIATE raise:
sqlite3.OperationalError: cannot start a transaction within a transaction
This aborts any CLI path that reaches store_file_nodes_edges after such a write — I hit it in v2.2.1 during an automated update invocation chained off an editor hook.
Stack trace
Traceback (most recent call last):
File "/Users/.../bin/code-review-graph", line 10, in <module>
sys.exit(main())
File ".../code_review_graph/cli.py", line 501, in main
result = build_or_update_graph(...)
File ".../code_review_graph/tools/build.py", line 311, in build_or_update_graph
result = incremental_update(root, store, base=base)
File ".../code_review_graph/incremental.py", line 515, in incremental_update
store.store_file_nodes_edges(...)
File ".../code_review_graph/graph.py", line 239, in store_file_nodes_edges
self._conn.execute("BEGIN IMMEDIATE")
sqlite3.OperationalError: cannot start a transaction within a transaction
Deterministic reproduction
import tempfile, pathlib
from code_review_graph.graph import GraphStore, NodeInfo
with tempfile.TemporaryDirectory() as d:
store = GraphStore(pathlib.Path(d) / "graph.db")
# Force the connection into an implicit transaction (any DML works).
store._conn.execute(
"INSERT OR REPLACE INTO metadata (key, value) VALUES (?, ?)",
("repro", "1"),
)
assert store._conn.in_transaction
node = NodeInfo(kind="File", name="x.py", file_path="/x.py",
line_start=1, line_end=1)
store.store_file_nodes_edges("/x.py", [node], [], "deadbeef")
# ^ raises sqlite3.OperationalError
Root cause
sqlite3.connect(...) in GraphStore.__init__ uses the default isolation_level="" (deferred), where Python opens an implicit transaction on the first INSERT/UPDATE/DELETE and only closes it on commit() / rollback(). BEGIN IMMEDIATE issued via execute() while one of those is open hits SQLite's "cannot start a transaction within a transaction" rule.
Proposed minimal fix
--- a/code_review_graph/graph.py
+++ b/code_review_graph/graph.py
@@ -235,6 +235,14 @@ class GraphStore:
def store_file_nodes_edges(
self, file_path: str, nodes: list[NodeInfo], edges: list[EdgeInfo], fhash: str = ""
) -> None:
"""Atomically replace all data for a file."""
+ # Python's sqlite3 module auto-starts a deferred transaction on the
+ # first DML statement under default isolation. If the connection is
+ # already inside such an implicit transaction when we reach here,
+ # `BEGIN IMMEDIATE` raises "cannot start a transaction within a
+ # transaction". Flush any pending implicit transaction first so the
+ # IMMEDIATE write-lock acquisition succeeds cleanly.
+ if self._conn.in_transaction:
+ self._conn.commit()
self._conn.execute("BEGIN IMMEDIATE")
Audit
Grepping the package, BEGIN IMMEDIATE only appears once (graph.py:239), so this single guard fully closes the regression. Other commit() / rollback() call sites in graph.py look correct.
Optional broader hardening
A more durable fix would be to switch the connection to autocommit mode (sqlite3.connect(..., isolation_level=None)) and manage every transaction explicitly. That removes a whole class of "stray DML left an implicit transaction open" bugs, but it changes the semantics of upsert_node / upsert_edge / similar (they currently rely on the caller's commit() to flush a batch), so the diff is bigger and probably wants its own PR.
Regression test
import tempfile, unittest
from pathlib import Path
from code_review_graph.graph import GraphStore, NodeInfo
class StoreFileNodesEdgesTransactionTest(unittest.TestCase):
def setUp(self):
self._tmp = tempfile.TemporaryDirectory()
self.store = GraphStore(Path(self._tmp.name) / "g.db")
def tearDown(self):
self.store.close(); self._tmp.cleanup()
def test_succeeds_when_implicit_transaction_open(self):
self.store._conn.execute(
"INSERT OR REPLACE INTO metadata (key, value) VALUES (?, ?)",
("repro", "1"),
)
self.assertTrue(self.store._conn.in_transaction)
node = NodeInfo(kind="File", name="x.py", file_path="/x.py",
line_start=1, line_end=1)
self.store.store_file_nodes_edges("/x.py", [node], [], "deadbeef")
rows = self.store._conn.execute(
"SELECT COUNT(*) AS n FROM nodes WHERE file_path = ?", ("/x.py",),
).fetchone()
self.assertEqual(rows["n"], 1)
Without the fix: OperationalError: cannot start a transaction within a transaction.
With the fix: passes.
Environment
code-review-graph 2.2.1 (installed via uv tool install)
- Python 3.10.19
- macOS (darwin)
Summary
GraphStore.store_file_nodes_edgesincode_review_graph/graph.pyissuesBEGIN IMMEDIATEwithout first checking whether the connection is already inside an implicit transaction. Python'ssqlite3module under its default deferred isolation auto-starts a transaction on the first DML statement, so any prior write on the same connection makesBEGIN IMMEDIATEraise:This aborts any CLI path that reaches
store_file_nodes_edgesafter such a write — I hit it in v2.2.1 during an automatedupdateinvocation chained off an editor hook.Stack trace
Deterministic reproduction
Root cause
sqlite3.connect(...)inGraphStore.__init__uses the defaultisolation_level="" (deferred), where Python opens an implicit transaction on the first INSERT/UPDATE/DELETE and only closes it oncommit()/rollback().BEGIN IMMEDIATEissued viaexecute()while one of those is open hits SQLite's "cannot start a transaction within a transaction" rule.Proposed minimal fix
Audit
Grepping the package,
BEGIN IMMEDIATEonly appears once (graph.py:239), so this single guard fully closes the regression. Othercommit()/rollback()call sites ingraph.pylook correct.Optional broader hardening
A more durable fix would be to switch the connection to autocommit mode (
sqlite3.connect(..., isolation_level=None)) and manage every transaction explicitly. That removes a whole class of "stray DML left an implicit transaction open" bugs, but it changes the semantics ofupsert_node/upsert_edge/ similar (they currently rely on the caller'scommit()to flush a batch), so the diff is bigger and probably wants its own PR.Regression test
Without the fix:
OperationalError: cannot start a transaction within a transaction.With the fix: passes.
Environment
code-review-graph2.2.1 (installed viauv tool install)