Repository navigation
fix(#4908,#4909): retryable Bolt conflicts + nested $param.field in MATCH patterns - #4910
Conversation
… Bolt error so drivers auto-retry ArcadeDB's MVCC page-version conflicts surfaced to Bolt clients as non-retryable errors (DATABASE_ERROR for query execution, ClientError TransactionNotFound on commit), so a managed-transaction driver never auto-retried even though ArcadeDB asks the client to retry. - BoltErrorCodes: add TRANSIENT_CONFLICT_ERROR (Neo.TransientError.Transaction.DeadlockDetected), a TransientError classification drivers retry on, avoiding the two excluded titles. - BoltNetworkExecutor.classifyExecutionError(): classify any NeedRetryException in the cause chain (ConcurrentModificationException / LockTimeoutException) to the transient code; applied in the RUN, PULL and COMMIT error paths. - Test: BoltErrorClassificationTest covers conflict/lock-timeout/wrapped-cause -> transient and generic -> default.
…ATCH pattern property maps
A pattern property like {uuid: $edge_data.source_uuid} parses to a ChainedPropertyAccessExpression
whose base ParameterExpression resolves from the context alone, but MatchNodeStep only evaluated
pattern-property expressions when an input row was present. With a bare MATCH (no row) the expression
was left unevaluated, so the pattern silently matched nothing (breaking single edge saves in the
graphiti driver).
- MatchNodeStep: evaluate pattern-property expressions with a shared read-only empty result when
there is no input row, in both the scan path (matchesProperties) and the index path
(tryFindAndUseIndex); parameter-based expressions resolve, row-dependent ones return null and
fall back to scan.
- Test: NestedParameterInMatchTest covers scan path, index path, and the graphiti edge-save shape.
|
Tick the box to add this pull request to the merge queue (same as
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces error classification for Bolt transactions to map optimistic-concurrency conflicts to retryable Neo4j transient errors, and fixes an issue in OpenCypher where nested parameter expressions in MATCH patterns failed to resolve when no input row was present. The review feedback highlights two main areas of improvement: first, the static EMPTY_RESULT in MatchNodeStep should be changed to an instance field to avoid thread-safety issues with the mutable ResultInternal class; second, the exception cause traversal in classifyExecutionError should include a depth limit to prevent infinite loops in the event of circular exception references.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Read-only empty result used to evaluate context-only pattern property expressions (e.g. a parameter | ||
| // map field like $edge_data.uuid) when the pattern has no input row. Row-variable lookups against it | ||
| // return null gracefully. Shared because it is only ever read (issue #4909). | ||
| private static final Result EMPTY_RESULT = new ResultInternal(); |
There was a problem hiding this comment.
Using a shared static final instance of ResultInternal (EMPTY_RESULT) across all queries can lead to thread-safety issues. ResultInternal is a mutable class and is not thread-safe. If any query execution or custom function modifies the Result passed to it (e.g., by casting it to ResultInternal and calling setProperty), it could corrupt the state or cause race conditions for other concurrent queries.\n\nSince MatchNodeStep is instantiated once per query execution plan, making emptyResult a non-static instance field completely isolates it to the current query execution thread, eliminating any multi-threading risks with negligible overhead.
| // Read-only empty result used to evaluate context-only pattern property expressions (e.g. a parameter | |
| // map field like $edge_data.uuid) when the pattern has no input row. Row-variable lookups against it | |
| // return null gracefully. Shared because it is only ever read (issue #4909). | |
| private static final Result EMPTY_RESULT = new ResultInternal(); | |
| // Read-only empty result used to evaluate context-only pattern property expressions (e.g. a parameter\n // map field like $edge_data.uuid) when the pattern has no input row. Row-variable lookups against it\n // return null gracefully. Made an instance field to ensure thread-safety across concurrent queries (issue #4909).\n private final Result emptyResult = new ResultInternal(); |
| else if (propertyValue instanceof Expression) { | ||
| if (currentInputResult != null) | ||
| propertyValue = evaluator.evaluate((Expression) propertyValue, currentInputResult, context); | ||
| else | ||
| return null; // Cannot resolve expression without input row — skip index | ||
| propertyValue = evaluator.evaluate((Expression) propertyValue, | ||
| currentInputResult != null ? currentInputResult : EMPTY_RESULT, context); | ||
| if (propertyValue == null) | ||
| return null; // Couldn't resolve (e.g. row-dependent expression with no row) — skip index | ||
| } |
There was a problem hiding this comment.
Update the reference from the static EMPTY_RESULT to the instance-level emptyResult to maintain compilation and thread-safety.
else if (propertyValue instanceof Expression) {\n propertyValue = evaluator.evaluate((Expression) propertyValue,\n currentInputResult != null ? currentInputResult : emptyResult, context);\n if (propertyValue == null)\n return null; // Couldn't resolve (e.g. row-dependent expression with no row) — skip index\n }| if (expectedValue instanceof Expression) | ||
| expectedValue = evaluator.evaluate((Expression) expectedValue, currentResult != null ? currentResult : EMPTY_RESULT, context); |
There was a problem hiding this comment.
Update the reference from the static EMPTY_RESULT to the instance-level emptyResult to maintain compilation and thread-safety.
if (expectedValue instanceof Expression)\n expectedValue = evaluator.evaluate((Expression) expectedValue, currentResult != null ? currentResult : emptyResult, context);| static String classifyExecutionError(final Throwable error, final String defaultCode) { | ||
| for (Throwable t = error; t != null; t = t.getCause()) { | ||
| if (t instanceof NeedRetryException) | ||
| return BoltErrorCodes.TRANSIENT_CONFLICT_ERROR; | ||
| } | ||
| return defaultCode; | ||
| } |
There was a problem hiding this comment.
The cause chain traversal in classifyExecutionError does not guard against circular exception references (where an exception's cause chain eventually points back to itself). While rare, circular references in exception causes can cause an infinite loop, hanging the network executor thread and consuming 100% CPU.\n\nTo prevent this, we should implement a simple depth limit (e.g., 100 levels) or keep track of visited throwables to break any potential cycles.
static String classifyExecutionError(final Throwable error, final String defaultCode) {\n int depth = 0;\n for (Throwable t = error; t != null && depth < 100; t = t.getCause(), depth++) {\n if (t instanceof NeedRetryException)\n return BoltErrorCodes.TRANSIENT_CONFLICT_ERROR;\n }\n return defaultCode;\n }
Review: fix(#4908,#4909) - retryable Bolt conflicts + nested
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4910 +/- ##
=============================================
- Coverage 65.64% 36.03% -29.62%
- Complexity 815 1152 +337
=============================================
Files 1681 1681
Lines 134059 134095 +36
Branches 28646 28660 +14
=============================================
- Hits 88008 48315 -39693
- Misses 33932 76772 +42840
+ Partials 12119 9008 -3111 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the thorough review (@gemini-code-assist and the Claude reviewer). Addressed all points: #4909 - shared #4909 - index-path behavior change ✅ #4908 - cyclic cause chain ✅ Style nit - fully-qualified On the test-path nit ( Re-verified: full |
|
Thank you for the update, @lvca. The hardening of |
…eserve index behavior - MatchNodeStep: back EMPTY_RESULT with an immutable map (new ResultInternal(Collections.emptyMap())) so any accidental write fails fast instead of racing shared state across concurrent queries; restore the exact previous behavior of the index path when an input row is present (only the no-row case gets the null -> fall-back-to-scan handling). - BoltNetworkExecutor.classifyExecutionError(): bound the cause-chain walk (depth cap) so a cyclic getCause() cannot spin forever; add a cyclic-cause regression test. - NestedParameterInMatchTest: import Consumer instead of the fully-qualified name (also the sole Codacy CodeStyle finding).
Code Review — PR #4910Reviewed both fixes end to end. This is a clean, well-scoped PR: root-cause fixes with focused regression tests, careful backward-compatibility, and defensive hardening in the follow-up commit. Nice work. A few observations below, mostly minor. Strengths
Minor suggestions (non-blocking)
VerdictLooks good to merge. The two log-level/comment items are polish, not blockers. Both fixes are minimal, correct, backward-compatible, and properly tested per the repo's TDD guidance. |
…tch is intentional - BoltNetworkExecutor: extract isRetryableConflict() and, in the RUN/PULL handlers, log a NeedRetryException at FINE instead of WARNING - MVCC conflicts are expected under contention and auto-retried by the driver, so they should not flood WARNING with recoverable flow; genuine errors stay at WARNING. - Add a comment at the top-level dispatch catch clarifying that NeedRetry classification is handled in the per-message RUN/PULL/COMMIT handlers, so the generic DATABASE_ERROR fallback is intentional. - Test: cover isRetryableConflict directly (drives both classification and log-level demotion).
|
Addressed the two operational suggestions in Log level for expected conflicts ✅ Uncovered fallback catch ✅ Also added a direct Thanks for the operational eye - the FINE/WARNING split is the kind of thing that only bites in production. |
Review: fix(#4908,#4909) - retryable Bolt conflicts + nested
|
… wire test - BoltNetworkExecutor: move the NeedRetryException import into alphabetical position (after exception.CommandParsingException) instead of ahead of the bolt.message block. - Bolt4908TransientConflictIT: end-to-end wire test that a real optimistic-concurrency conflict over Bolt surfaces to the neo4j driver as a TransientException with code Neo.TransientError.Transaction.DeadlockDetected. Made deterministic (not racy) via two explicit transactions modifying the same record with ordered commits: the first commit bumps the page version, the second detects the stale version and conflicts. Locks in the wire-level contract the graphiti driver's managed-transaction retry depends on.
|
Addressed in Import ordering ✅ End-to-end Bolt wire test ✅ (took your optional suggestion - it's a contract worth pinning) On the two awareness-only notes:
Full bolt unit suite green (231), and the new IT passes deterministically. |
PR Review: Retryable Bolt conflicts + nested
|
…/PULL handlers Keep isRetryableConflict(e) only for the FINE/WARNING log level and let classifyExecutionError(e, default) own the error-code mapping, so the retryable->transient-code selection lives in one place instead of being inlined. Cosmetic; error-path only.
|
Addressed in DRY in RUN/PULL handlers ✅ On the two awareness notes - both left as-is, agreed:
Bolt unit suite green (231) and the MVCC-conflict IT still deterministic. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
🟢 Coverage 47.06% diff coverage · -36.59% coverage variation
Metric Results Coverage variation ✅ -36.59% coverage variation Diff coverage ✅ 47.06% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (7ef8ab3) 134059 100164 74.72% Head commit (7f8329c) 165898 (+31839) 63249 (-36915) 38.13% (-36.59%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4910) 17 8 47.06% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
ReviewNice, tightly-scoped pair of root-cause fixes. Both are backed by regression tests, the comments explain the why (not just the what), and the iterative hardening across commits (immutable empty result, bounded cause-walk, log-level demotion) shows good attention to review feedback. I verified the exception hierarchy ( #4908 - Bolt transient conflicts
#4909 - Nested
|
…ATCH patterns (#4910) * fix(#4908): map optimistic-concurrency conflicts to a Neo4j transient Bolt error so drivers auto-retry ArcadeDB's MVCC page-version conflicts surfaced to Bolt clients as non-retryable errors (DATABASE_ERROR for query execution, ClientError TransactionNotFound on commit), so a managed-transaction driver never auto-retried even though ArcadeDB asks the client to retry. - BoltErrorCodes: add TRANSIENT_CONFLICT_ERROR (Neo.TransientError.Transaction.DeadlockDetected), a TransientError classification drivers retry on, avoiding the two excluded titles. - BoltNetworkExecutor.classifyExecutionError(): classify any NeedRetryException in the cause chain (ConcurrentModificationException / LockTimeoutException) to the transient code; applied in the RUN, PULL and COMMIT error paths. - Test: BoltErrorClassificationTest covers conflict/lock-timeout/wrapped-cause -> transient and generic -> default. * fix(#4909): resolve nested parameter field access ($param.field) in MATCH pattern property maps A pattern property like {uuid: $edge_data.source_uuid} parses to a ChainedPropertyAccessExpression whose base ParameterExpression resolves from the context alone, but MatchNodeStep only evaluated pattern-property expressions when an input row was present. With a bare MATCH (no row) the expression was left unevaluated, so the pattern silently matched nothing (breaking single edge saves in the graphiti driver). - MatchNodeStep: evaluate pattern-property expressions with a shared read-only empty result when there is no input row, in both the scan path (matchesProperties) and the index path (tryFindAndUseIndex); parameter-based expressions resolve, row-dependent ones return null and fall back to scan. - Test: NestedParameterInMatchTest covers scan path, index path, and the graphiti edge-save shape. * review(#4908,#4909): harden shared empty result, bound cause-walk, preserve index behavior - MatchNodeStep: back EMPTY_RESULT with an immutable map (new ResultInternal(Collections.emptyMap())) so any accidental write fails fast instead of racing shared state across concurrent queries; restore the exact previous behavior of the index path when an input row is present (only the no-row case gets the null -> fall-back-to-scan handling). - BoltNetworkExecutor.classifyExecutionError(): bound the cause-chain walk (depth cap) so a cyclic getCause() cannot spin forever; add a cyclic-cause regression test. - NestedParameterInMatchTest: import Consumer instead of the fully-qualified name (also the sole Codacy CodeStyle finding). * review(#4908): log expected MVCC conflicts at FINE, note top-level catch is intentional - BoltNetworkExecutor: extract isRetryableConflict() and, in the RUN/PULL handlers, log a NeedRetryException at FINE instead of WARNING - MVCC conflicts are expected under contention and auto-retried by the driver, so they should not flood WARNING with recoverable flow; genuine errors stay at WARNING. - Add a comment at the top-level dispatch catch clarifying that NeedRetry classification is handled in the per-message RUN/PULL/COMMIT handlers, so the generic DATABASE_ERROR fallback is intentional. - Test: cover isRetryableConflict directly (drives both classification and log-level demotion). * review(#4908): fix import ordering, add end-to-end Bolt MVCC-conflict wire test - BoltNetworkExecutor: move the NeedRetryException import into alphabetical position (after exception.CommandParsingException) instead of ahead of the bolt.message block. - Bolt4908TransientConflictIT: end-to-end wire test that a real optimistic-concurrency conflict over Bolt surfaces to the neo4j driver as a TransientException with code Neo.TransientError.Transaction.DeadlockDetected. Made deterministic (not racy) via two explicit transactions modifying the same record with ordered commits: the first commit bumps the page version, the second detects the stale version and conflicts. Locks in the wire-level contract the graphiti driver's managed-transaction retry depends on. * review(#4908): reuse classifyExecutionError for code selection in RUN/PULL handlers Keep isRetryableConflict(e) only for the FINE/WARNING log level and let classifyExecutionError(e, default) own the error-code mapping, so the retryable->transient-code selection lives in one place instead of being inlined. Cosmetic; error-path only. (cherry picked from commit 23962d1)
1 participant
Summary
Two follow-ups to #4905, flagged during review of #4906. Both are root-cause fixes in ArcadeDB for workarounds the getzep/graphiti driver currently carries; landing them lets the driver drop those workarounds. Diagnosed by @agc-63.
Closes #4908, closes #4909.
#4908 - Bolt: optimistic-concurrency conflicts are now retryable
ArcadeDB's MVCC page-version conflicts surfaced to Bolt clients as non-retryable errors (
Neo.DatabaseError.General.UnknownErrorfor query execution,Neo.ClientError.Transaction.TransactionNotFoundon commit), so a managed-transaction driver (session.execute_write(...)) never auto-retried - even though ArcadeDB's own message says "Please retry the operation".BoltErrorCodes: addTRANSIENT_CONFLICT_ERROR = Neo.TransientError.Transaction.DeadlockDetected- aTransientErrorclassification the Neo4j drivers retry on (avoiding the two excluded titlesTransaction.Terminated/Transaction.LockClientStopped).BoltNetworkExecutor.classifyExecutionError(): classify anyNeedRetryExceptionin the cause chain (ConcurrentModificationException/LockTimeoutException) to the transient code; applied in the RUN, PULL and COMMIT error paths.#4909 - OpenCypher: nested parameter field access in MATCH patterns
MATCH (source:Entity {uuid: $edge_data.source_uuid})matched nothing (no error): the pattern property parses to aChainedPropertyAccessExpressionwhose baseParameterExpressionresolves from the context alone, butMatchNodeSteponly evaluated pattern-property expressions when an input row was present. With a bare MATCH (no row) the expression was left unevaluated and never matched - which is why the graphiti driver had to wrap single edge saves inUNWIND [$edge_data] AS edge.MatchNodeStep: evaluate pattern-property expressions with a shared read-only empty result when there is no input row, in both the scan path (matchesProperties) and the index path (tryFindAndUseIndex). Parameter-based expressions resolve; row-dependent ones return null and fall back to scan.Tests
BoltErrorClassificationTest(5) - conflict / lock-timeout / wrapped-cause -> transient, generic -> default, and the transient code is a driver-retryable classification.NestedParameterInMatchTest(3) - scan path, index path, and the graphitiMATCH ... MERGEedge-save shape.Verification
com.arcadedb.query.opencypher.**: 6640 tests, 0 failures / 0 errors.boltmodule: 229 tests, 0 failures / 0 errors.