Fix Neo4j delete: IN TRANSACTIONS is illegal via execute_query - #9
Merged
Conversation
The Cypher merged in the graph-store PR had never run against a server. It does not work.
Neo.DatabaseError.Transaction.TransactionStartFailed
A query with 'CALL { ... } IN TRANSACTIONS' can only be executed in an implicit
transaction, but tried to execute in an explicit transaction.
driver.execute_query() runs inside an explicit transaction and IN TRANSACTIONS is only
legal in an implicit one, so delete_repository always raised. sync_repository calls it
first, so the entire ingest path failed 100% of the time -- it only looked healthy because
the caller swallows sync failures by design and falls back to SQL.
delete_repository now loops bounded LIMIT batches instead. That keeps each transaction
small without needing implicit-transaction semantics, and it also drops the
CALL (n) { ... } scoped-variable form, which only exists from Neo4j 5.23. It returns the
number of nodes deleted so a caller can tell the difference between "nothing there" and
"did not run".
WHY THE UNIT TESTS DID NOT CATCH THIS
They assert `"DETACH DELETE" in query` against a fake driver. That still passes. A fake
driver does not enforce transaction semantics, so no amount of mocking could have found
it. Added tests/integration/test_neo4j_live.py: 9 tests against a real server, skipped
unless NEO4J_TEST_URI and NEO4J_TEST_PASSWORD are set, so CI and a laptop without Docker
stay green. They cover the regression directly plus degree, transitive traversal in both
directions, shortest-path direction, cycle detection, re-sync replacing rather than
unioning, cross-repository isolation, and a delete larger than one batch.
Verified against neo4j:5-community (kernel 5.26.29): 9/9 live tests pass, and the full
suite is 142 passed with the 9 live tests skipping cleanly when no server is configured.
ruff clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
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.
Docker is available now, so I ran the Cypher merged in #6 against a real Neo4j. It does not work.
driver.execute_query()runs inside an explicit transaction, andIN TRANSACTIONSis only legal in an implicit one. Sodelete_repositoryalways raised — andsync_repositorycalls it first, which means the entire Neo4j ingest path failed 100% of the time.It looked healthy only because the caller swallows sync failures by design and falls back to SQL. That fallback is what kept this invisible.
The fix
delete_repositoryloops boundedLIMITbatches. That keeps each transaction small without needing implicit-transaction semantics, and it drops theCALL (n) { ... }scoped-variable form, which only exists from Neo4j 5.23. It now returns the number of nodes deleted, so a caller can distinguish "nothing there" from "did not run".Why the unit tests didn't catch it
They assert
"DETACH DELETE" in queryagainst a fake driver. That still passes. A fake driver doesn't enforce transaction semantics, so no amount of mocking would have found this — the bug was invisible to mocks by construction.Added
tests/integration/test_neo4j_live.py: 9 tests against a real server, skipped unlessNEO4J_TEST_URI/NEO4J_TEST_PASSWORDare set, so CI and a machine without Docker stay green. Beyond the regression they cover degree viaCOUNT {}, transitive traversal in both directions, shortest-path directionality, cycle detection, re-sync replacing rather than unioning, cross-repository isolation, and a delete larger than one batch.Verification
Against
neo4j:5-community, kernel 5.26.29:ruff check src testsEverything #6 claimed is now actually true, rather than asserted. The remaining honest caveat: this validates the store in isolation, not a full index-then-graph run through the API.
🤖 Generated with Claude Code