From fa2cca39dc9a2a115d0c191e91be0a8357f5139b Mon Sep 17 00:00:00 2001 From: Kotob M Date: Thu, 9 Apr 2026 14:37:16 +0300 Subject: [PATCH] fix: rebuild crashes with "cannot start a transaction within a transaction" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running `code-review-graph build` on a large repo (~9k files) after an earlier interrupted build would crash almost immediately with: sqlite3.OperationalError: cannot start a transaction within a transaction I hit this on the Godot source tree. The first build ran to completion on the parse phase, then I Ctrl-C'd it while community detection was stuck (separate perf issue). On the retry, the rebuild blew up on the very first file. Why it happens: `full_build` starts by purging any files that used to exist in the DB but no longer exist on disk. The purge loop calls `store.remove_file_data()` for each stale path, which issues two `DELETE` statements and returns without committing. Under Python's default (legacy) sqlite3 isolation mode, those DELETEs silently open an implicit transaction. The next `store_file_nodes_edges()` call then runs an explicit `BEGIN IMMEDIATE` — and SQLite rejects it because one is already open. The reason nobody had tripped this before is that the purge loop is usually a no-op: if every file still exists, `existing_files - current_abs` is empty. But my interrupted build had left the DB with thousands of file rows whose absolute paths no longer matched the set the rebuild was computing, so every single one hit the purge path and the connection was left dirty before parsing even started. Fix is two parts: 1. `full_build` now calls `store.commit()` once after the purge loop, so the parse phase starts with a clean connection. 2. `store_file_nodes_edges` checks `conn.in_transaction` and flushes any dangling implicit transaction before its `BEGIN IMMEDIATE`. Defensive: if any future code path leaves DML uncommitted, the atomic per-file replace still works instead of crashing the build. Adds a regression test that primes an implicit transaction via `remove_file_data` and then asserts `store_file_nodes_edges` still succeeds. --- code_review_graph/graph.py | 5 +++++ code_review_graph/incremental.py | 9 +++++++-- tests/test_graph.py | 19 +++++++++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/code_review_graph/graph.py b/code_review_graph/graph.py index 2dfa97f..ea2fa6f 100644 --- a/code_review_graph/graph.py +++ b/code_review_graph/graph.py @@ -236,6 +236,11 @@ 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.""" + # Flush any pending implicit transaction opened by earlier DML on this + # connection so the explicit BEGIN IMMEDIATE below doesn't trip SQLite's + # "cannot start a transaction within a transaction" error. + if self._conn.in_transaction: + self._conn.commit() self._conn.execute("BEGIN IMMEDIATE") try: self.remove_file_data(file_path) diff --git a/code_review_graph/incremental.py b/code_review_graph/incremental.py index 863211b..f67e894 100644 --- a/code_review_graph/incremental.py +++ b/code_review_graph/incremental.py @@ -352,8 +352,13 @@ def full_build(repo_root: Path, store: GraphStore) -> dict: # Purge stale data from files no longer on disk existing_files = set(store.get_all_files()) current_abs = {str(repo_root / f) for f in files} - for stale in existing_files - current_abs: - store.remove_file_data(stale) + stale_files = existing_files - current_abs + if stale_files: + for stale in stale_files: + store.remove_file_data(stale) + # Commit the implicit transaction opened by the DELETEs so the + # subsequent BEGIN IMMEDIATE in store_file_nodes_edges succeeds. + store.commit() total_nodes = 0 total_edges = 0 diff --git a/tests/test_graph.py b/tests/test_graph.py index 5923f57..5ea757c 100644 --- a/tests/test_graph.py +++ b/tests/test_graph.py @@ -107,6 +107,25 @@ def test_store_file_nodes_edges(self): result = self.store.get_nodes_by_file("/test/file.py") assert len(result) == 2 + def test_store_file_nodes_edges_after_dangling_dml(self): + """Regression: store_file_nodes_edges must recover when the connection + is already inside an implicit transaction opened by a prior DML (e.g. + the stale-file purge loop in full_build). Previously this raised + 'cannot start a transaction within a transaction'. + """ + # Simulate the purge loop: run DELETEs without committing so the + # connection is left in an implicit transaction. + self.store.upsert_node(self._make_file_node("/stale.py")) + self.store.commit() + self.store.remove_file_data("/stale.py") + assert self.store._conn.in_transaction + + nodes = [self._make_file_node(), self._make_func_node()] + # Should not raise sqlite3.OperationalError. + self.store.store_file_nodes_edges("/test/file.py", nodes, []) + + assert len(self.store.get_nodes_by_file("/test/file.py")) == 2 + def test_search_nodes(self): self.store.upsert_node(self._make_func_node("authenticate")) self.store.upsert_node(self._make_func_node("authorize"))