fix: resolve SQLite transaction nesting errors (#135, #181)#205
Open
ghiemer wants to merge 5 commits intotirth8205:mainfrom
Open
fix: resolve SQLite transaction nesting errors (#135, #181)#205ghiemer wants to merge 5 commits intotirth8205:mainfrom
ghiemer wants to merge 5 commits intotirth8205:mainfrom
Conversation
…205#135, tirth8205#181) Python's sqlite3 with default isolation_level="" silently opens a DEFERRED transaction on the first DML statement. When remove_file_data() is called for stale/deleted files, the DELETE statements open an implicit transaction that is never committed. The subsequent store_file_nodes_edges() then executes BEGIN IMMEDIATE inside that already-open transaction, causing: sqlite3.OperationalError: cannot start a transaction within a transaction This affects both full_build (stale-file purge loop) and incremental_update (deleted-file handling). Fix: - graph.py: Add in_transaction guard before BEGIN IMMEDIATE in store_file_nodes_edges() to flush any pending implicit transaction - graph.py: Add public rollback() method to GraphStore - incremental.py: Commit after stale-file purge loop in full_build() - incremental.py: Commit after deleted-file removal in incremental_update() Tests: - test_store_after_remove_no_transaction_error (single remove → store) - test_store_after_multiple_removes_no_transaction_error (bulk removes → store) Closes tirth8205#135, tirth8205#181, relates to tirth8205#162, tirth8205#193, tirth8205#94 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The in_transaction guard from the previous commit addresses the symptom. This commit fixes the root cause: Python's sqlite3 default isolation_level="" silently opens implicit DEFERRED transactions on every DML statement. Setting isolation_level=None disables this legacy behavior entirely, making all transaction management explicit. Changes: - graph.py: Set isolation_level=None in sqlite3.connect() to disable implicit transactions — this is the actual root-cause fix - graph.py: Downgrade in_transaction guard log from debug to warning (it should never fire now; if it does, something is wrong) - tools/build.py: Wrap each _compute_summaries block in explicit BEGIN IMMEDIATE / COMMIT / ROLLBACK — previously these DELETE+INSERT sequences ran without transaction protection, risking partial writes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- incremental.py: Reword commit-after-delete comments to explain the reason (avoiding nested transactions) instead of describing the mechanism (flushing implicit transactions) - test_graph.py: Reword test comments to explain the scenario being guarded against rather than implementation details that changed with the isolation_level=None fix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… ops Audit of the entire codebase found two additional sqlite3.connect() calls that were still using the default isolation_level="" (implicit transactions), plus two batch-write functions without transaction protection. Connections: - registry.py: ConnectionPool.get() now passes isolation_level=None to prevent implicit transactions on pooled connections - embeddings.py: EmbeddingStore.__init__ now passes isolation_level=None to match GraphStore behavior Atomic batch operations: - flows.py: store_flows() wrapped in BEGIN IMMEDIATE / COMMIT / ROLLBACK to prevent partial flow data if an exception interrupts the loop - communities.py: store_communities() wrapped in BEGIN IMMEDIATE / COMMIT / ROLLBACK to prevent partial community data on crash Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Lock file was out of sync — still referenced v2.0.0 while pyproject.toml already declared v2.2.2. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes
sqlite3.OperationalError: cannot start a transaction within a transactionduringfull_buildandincremental_updatewhen deleted/stale files are present.Root cause: Python's
sqlite3module with the defaultisolation_level=""silently opens implicitDEFERREDtransactions on every DML statement. Whenremove_file_data()issuesDELETEstatements for stale files, the implicit transaction is never committed. The subsequentstore_file_nodes_edges()then executesBEGIN IMMEDIATEinside the already-open transaction — SQLite rejects this.This PR fixes the root cause by setting
isolation_level=None(disabling implicit transactions entirely), adds defense-in-depth guards, and wraps all batch-write operations in explicit transactions for atomicity.Changes
Root cause fix
isolation_level=Noneonsqlite3.connect()inGraphStore.__init__— eliminates implicit transactions project-wideConnectionPool.get()EmbeddingStore.__init__Defense-in-depth
in_transactionguard beforeBEGIN IMMEDIATEinstore_file_nodes_edges()— catches any residual open transaction from external_connaccess (flows.py, communities.py, etc.)rollback()method onGraphStorefull_build()and after deleted-file removal inincremental_update()Atomic batch operations
_compute_summariesblock (community_summaries, flow_snapshots, risk_index) inBEGIN IMMEDIATE / COMMIT / ROLLBACKstore_flows()in explicit transactionstore_communities()in explicit transactionTests
test_store_after_remove_no_transaction_error— single remove then storetest_store_after_multiple_removes_no_transaction_error— bulk removes then store (simulates full_build stale-file purge)Why not just a guard?
Four other PRs (#94, #162, #181, #193) propose symptom-level fixes (guards, savepoints, commit-after-delete). This PR goes further:
isolation_level=NoneTest plan
uv run pytest tests/)postprocess=nonethen separate postprocesspostprocess=minimalCloses #135, closes #181. Relates to #162, #193, #94, #110.
🤖 Generated with Claude Code