fix(engine) #5670: an unreadable edge-list chunk retries the delete instead of skipping it - #5678

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

Up to standards ✅

_{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.}

Review: fix(engine) #5670 - unreadable edge-list chunk retries the delete

Thorough, well-reasoned fix. The root-cause analysis is convincing, the fix aligns the removal path with the contract the append path (getOrCreateEdgeList / StripedEdgeList.addChain) already honors, and the split between read-side best-effort (getEdgeHeadChunk) and write-side strict (getEdgeHeadChunkForWrite) is the right seam. Removing loadStripeHead rather than leaving it as dead code, and the deterministic + concurrent test trio (with precondition assertions and RID-matching on the exception message so they cannot pass on an unrelated throw) are exactly what this kind of change needs.

A few points worth considering:

1. getEdgeHeadChunkForWrite reads the head RID outside its try/catch (minor robustness gap)
The read-side getEdgeHeadChunk deliberately keeps getOutEdgesHeadChunk()/getInEdgesHeadChunk() INSIDE the try, because ImmutableVertex.getOutEdgesHeadChunk() calls checkForLazyLoading(), which can throw RecordNotFoundException if the vertex was deleted concurrently (its FINE-level branch handles exactly that). In getEdgeHeadChunkForWrite that call sits OUTSIDE the try, so a concurrently-deleted endpoint vertex surfacing at this point escapes as a bare RecordNotFoundException - which is NOT a NeedRetryException and would fail the transaction without retry.

In practice resolveEndpointToDisconnect has just materialized the vertex via getOutVertex(), so checkForLazyLoading() is usually a no-op and the window is narrow. But note the ordering: resolveEndpointToDisconnect checks existsRecord plus resolves the vertex, then getEdgeHeadChunkForWrite reads the head - a concurrent delete landing BETWEEN the two slips past both guards. Given the whole point of the PR is to convert exactly these transient windows into retries, it seems inconsistent to leave this one read able to throw a non-retryable exception. Moving the head-RID read inside the try (mapping it to CME, or to null as the read-side does) would close it.

2. Permanent corruption is now converted into a permanent failure (deliberate, worth confirming)
getEdgeHeadChunkForWrite and readChunk/loadChunkForWrite cannot distinguish a TRANSIENT cross-page publication race from a GENUINELY lost/corrupt chunk. Before, a truly corrupt endpoint edge-list let edge.delete() complete best-effort (leaving a dangling back-ref, but succeeding). Now it throws CME on every attempt and fails permanently after TX_RETRIES. That is the correct trade for the concurrency bug, and check database is the repair path, but it is the mirror image of the deleteVertex best-effort tolerance you deliberately kept for repairability. Worth confirming an edge whose endpoint list is genuinely broken (not racing) does not become un-deletable and thus block cleanup - the follow-up you filed for deleteVertex should probably note this coupling.

3. moveEdge inherits the stricter deleteEdge
moveEdge calls deleteEdge(edge) (GraphEngine.java:571), so it now can raise CME too. That is fine inside database.transaction(...) auto-retry, but a client-managed explicit transaction over RemoteDatabase calling a move will now see the CME where it previously did not. The release note covers edge.delete(); a one-line mention that moveEdge shares the contract would make the visible-behavior section complete.

Tests / style

• Good discipline lowering TX_RETRY_DELAY and restoring it in finally; @Tag("slow") on the concurrent test matches the repo convention.
• isCheckingDatabaseIntegrity() override with the explanatory comment is the right call for the two deterministic tests that intentionally corrupt the chain.
• Nit: the worker threads catch Exception and bump a failure counter, then the test asserts failures == 0 - a NeedRetryException that legitimately exhausts 10_000 retries would read as a failure identical to a real bug. Fine given the pool is drained one removal per iteration, but a comment that any surfaced exception here IS the bug would help a future reader.

I did not run the build/suite as part of this review; the PR reports a clean engine run. Overall this looks like a correct and carefully scoped fix - the items above are refinements, not blockers.

Thanks - all three points were valid. Addressed in 51c212b.

1. Head RID read outside the try — fixed, and it was reachable. Confirmed the mechanism: ImmutableVertex.getOutEdgesHeadChunk() → checkForLazyLoading() → ImmutableDocument.checkForLazyLoading() → LocalBucket.getRecord(rid), which raises RecordNotFoundException. The read moved inside the try.

Rather than just move it, I pinned it: headChunkForWriteRaisesRetryableConflictWhenTheVertexItselfVanishes resolves a handle with lookupByRID(rid, false) (so the record is not materialised), deletes the vertex record, then calls the strict lookup. On the previous commit it fails exactly as you predicted:

Expecting actual throwable to be an instance of:
  com.arcadedb.exception.ConcurrentModificationException
but was:
  com.arcadedb.exception.RecordNotFoundException: Record #1:0 not found
	at com.arcadedb.engine.LocalBucket.getRecord(LocalBucket.java:291)
	at com.arcadedb.database.ImmutableDocument.checkForLazyLoading(ImmutableDocument.java:216)
	at com.arcadedb.graph.ImmutableVertex.checkForLazyLoading(ImmutableVertex.java:251)

The message now carries the underlying cause, so the missing RID is still named.

2. Permanent corruption → permanent failure. Confirmed and deliberate. CHECK DATABASE is the repair path for a chain that cannot be loaded (GraphDatabaseCheckerChainRebuildTest covers exactly the head-chunk-unloadable case), so a genuinely broken graph is still cleanable — but you are right that it now disagrees with deleteVertex. The strict lookup's javadoc states the trade, and I recorded the coupling on #5680 so whoever picks that up settles both paths together instead of leaving edge deletion strict and vertex deletion tolerant by accident.

3. moveEdge. Correct — it disconnects through deleteEdge and shares the contract. The release note's visible-behaviour section now names it.

Test nit — taken: the worker catch now says that any exception surfacing there IS the bug, not a tolerated retry.

Re-ran the graph package after the change: 320 tests, 0 failures, including Issue4420TolerantDeleteTest and Issue4432CorruptVertexDeleteTest.

Review: #5678 - unreadable edge-list chunk retries the delete instead of skipping it

Thorough, well-argued change. The core idea is sound and consistent with the codebase: the removal path now draws the same line the append path (getOrCreateEdgeList, StripedEdgeList.addChain) already drew, treating a present-but-unreadable chunk as a retryable ConcurrentModificationException rather than as "nothing to remove". The read/write split (getEdgeHeadChunk vs getEdgeHeadChunkForWrite), removing the now-redundant loadStripeHead, and the second commit's fix (moving the head-RID read inside the try so a concurrently-deleted lazy vertex maps to a retryable conflict rather than a raw RecordNotFoundException) are all correct and clearly reasoned. The Javadoc stating the deliberate trade-off is exemplary.

I verified statically that readChunk/loadChunkForWrite are only reached from the three removal walks + the striped write path, and that getEdgeHeadChunkForWrite is only called from deleteEdge. So the new strictness is confined to genuine mutation paths; read/iteration/count paths stay best-effort. Good.

Main point worth a second look: deleteVertex's edge-deletion loop is no longer tolerant of a corrupt neighbour

The PR deliberately preserves deleteVertex's best-effort collection of its own edges (tracked in #5680), but there is a second-order effect that isn't discussed. In GraphEngine.deleteVertex (around lines 657-662):

for (Identifiable edge : edgesToDelete)
  try {
    edge.asEdge().delete();
  } catch (RecordNotFoundException e) {
    // ALREADY DELETED, IGNORE IT
  }

edge.asEdge().delete() now routes through the strict deleteEdge, which disconnects from both endpoints - including the neighbour vertex - via getEdgeHeadChunkForWrite + the strict removal walk. That path can now throw ConcurrentModificationException, which this loop does not catch (only RecordNotFoundException), so it propagates out of deleteVertex.

• For the transient/concurrent case this is fine and intended: CME is a NeedRetryException, the tx retries.
• But for a genuinely broken neighbour edge list (a truly lost chunk, indistinguishable from a transient one by design), deleting an otherwise-healthy vertex now fails after exhausting TX_RETRIES instead of completing best-effort. Previously deleteEdge swallowed that internally, so the vertex delete succeeded. This is a real tolerance regression for corrupt-graph deletion, distinct from the deleteVertex-own-list tolerance that deleteVertex can delete a vertex without collecting its edges when a chunk is transiently unreadable (the #5670 window, vertex side) #5680 tracks.

Issue4432CorruptVertexDeleteTest looks like it only stays green because DELETE VERTEX FROM ... happens to visit the corrupt vertex (rids[0]) before its neighbour (rids[1]): once rids[0] is gone, resolveEndpointToDisconnect sees existsRecord == false and tolerates it. If the scan order were reversed (neighbour first, corrupt vertex still present), getEdgeHeadChunkForWrite on the corrupt vertex would read a bogus/unreadable head and could raise a non-tolerated exception. Worth confirming this is order-independent, or documenting the coupling alongside the #5680 note.

Minor / for consideration

• Genuinely-lost chunk -> repeated failure. As the Javadoc honestly states, a permanently-lost chunk now fails every retry rather than completing. That is the accepted trade, but since it converts a silent corruption into a hard user-visible failure, consider emitting a distinguishable log/metric on retry exhaustion so operators can tell a retry storm from real corruption vs. healthy contention and reach for CHECK DATABASE.
• EdgeIteratorFilter.handleCorruption now wraps the pruning tx in try { ... } catch (NeedRetryException). Since that runs with joinCurrent = true, any chunk pages anchored by the aborted removeEdgeRID before the CME remain in the joined (read) transaction. It is caught and best-effort as intended, and strictly better than the previous uncaught behaviour, but worth a sanity check that the partially-anchored pages cannot affect the enclosing read tx's later commit.

Tests

Strong coverage. The two deterministic tests assert their preconditions (chain spans multiple chunks, victim sits behind the hole, degree correct before corruption) and match the offending chunk RID in the exception message, so they cannot pass on an unrelated throw - exactly right. headChunkForWriteRaisesRetryableConflictWhenTheVertexItselfVanishes pins the second-commit gap precisely. The @Tag("slow") stress test's worker catch correctly treats any surfacing exception as the bug given the generous retry budget. Good call overriding isCheckingDatabaseIntegrity() since the tests intentionally corrupt chains, and using countEdges(...) (accurate full scan) rather than count(*) (cached) for ground truth.

One suggestion: add a deterministic test for the deleteVertex-deletes-neighbour-of-a-corrupt-vertex ordering above, to lock in whichever behaviour you intend there.

Note: I reviewed statically; Maven could not run in my environment, so I relied on the author's reported suite results (10587 tests, 0 failures) for execution confirmation.

Addressed in ae9a67b. Thanks — the second-order effect was real and worth stating; one supporting claim did not hold up, so I measured both.

The neighbour effect — confirmed, and kept strict deliberately. You are right that deleteVertex disconnects the vertex at the other end too, and that its loop catches only RecordNotFoundException. Measured on the same fixture (a healthy vertex whose only edge points at a hub whose IN head chunk was removed):

before: delete SUCCEEDED   (edge record gone, neighbour still pointing at it)
after:  ConcurrentModificationException

Keeping it strict. Succeeding there deletes the edge record while the neighbour keeps the back-reference — precisely the corruption this PR fixes, inflicted on a vertex the caller never named. Now pinned by deletingAVertexWhoseNeighbourListIsUnreadableReportsAConflictRatherThanDanglingTheReference, and stated in the strict lookup's javadoc and the release note. Recorded on #5680, including the observation that force does not currently help here — the per-edge loop sits outside its promise, which is the natural shape for a fix if that issue resolves toward tolerance.

Issue4432CorruptVertexDeleteTest — the suspicion does not hold. It is not green by scan order. LocalBucket detects the invalid record size on the first read and deletes the record:

SEVER [LocalBucket] Invalid record size 33554444 for record #1:0: deleting record

so the corrupt vertex fails at resolution — which stays tolerant — and never reaches getEdgeHeadChunkForWrite. Deleting the neighbour first with the corrupt vertex still present raises the identical RecordNotFoundException with and without this branch. The reversed ordering you describe is therefore already covered; it just resolves through a different door than expected.

EdgeIteratorFilter partially-anchored pages. Checked, and the behaviour is a bit different from the framing: with joinCurrent = true, LocalDatabase.transaction's catch (NeedRetryException …) calls rollback() on the joined transaction before retrying, so the anchored pages do not survive into a later commit — they are discarded with the tx. The wart is that the enclosing read transaction is rolled back under the caller, but that is pre-existing for any NeedRetryException on this path (promoted super-nodes have been able to raise CME here via StripedEdgeList.addChain), and the new catch strictly improves it by not also propagating the exhausted exception into a read. Not introduced here, so left alone.

Log/metric on retry exhaustion. Agreed in principle, but the right home is the generic retry-exhaustion point in LocalDatabase.transaction, not this path — a warning at the conversion site would fire on ordinary healthy contention, which is exactly the noise it is meant to distinguish from. Better folded into the #5680 decision than bolted on here.

Graph package after the change: 321 tests, 0 failures, including Issue4420TolerantDeleteTest and Issue4432CorruptVertexDeleteTest.

Review: #5670 unreadable edge-list chunk retries the delete instead of skipping it

Reviewed the diff, the surrounding GraphEngine/EdgeLinkedList/StripedEdgeList code paths, and the new test. This is a careful, well-argued fix. The root-cause analysis holds up: the removal walk read "chunk unreadable" as "nothing to remove" in three places while the edge record was deleted anyway, and the remedy - promote a transient invisibility to a retryable ConcurrentModificationException - is exactly the line the append path (getOrCreateEdgeList, StripedEdgeList.addChain) already draws. Symmetry restored rather than a new mechanism invented.

What I verified

• readChunk is only reached from the three removal walks (removeEdge, removeEdgeRID, removeVertex in EdgeLinkedList), so converting its miss to a CME does not leak into read/iteration paths. The Javadoc claim checks out.
• Read/repair callers stay best-effort: GraphDatabaseChecker and CSRBuilder keep using getEdgeHeadChunk (null-on-miss), and EdgeIteratorFilter.handleCorruption now catches NeedRetryException and leaves the ghost for a later pass - correct, since it prunes from inside a READ.
• Super-node path is covered: buildEdgeList wraps a StripeDirectory head as StripedEdgeList, so deleteEdge -> getEdgeHeadChunkForWrite gets the strict treatment on striped chains too; addChain(strict=true) already threw the matching conflict. Folding loadStripeHead into loadChunkForWrite removes real duplication with no behavior loss.
• No orphaned imports after the loadStripeHead removal (RecordNotFoundException/ConcurrentModificationException still used by addChain).
• deleteEdge no longer wraps chain mutation in a swallowing catch - only endpoint vertex resolution is tolerant (resolveEndpointToDisconnect), and no raw RecordNotFoundException escapes the mutation (both chunk-load points convert it), so the delete never fails with a non-retryable exception where a retryable one was intended.

The main thing to weigh (already acknowledged, worth stating plainly)

The deliberate trade in getEdgeHeadChunkForWrite's contract is the real behavioral change: an endpoint edge list that is genuinely broken (not transiently invisible) is indistinguishable from a transient one, so edge.delete() - and, transitively, deleteVertex when a neighbour's list is unreadable - now fails-and-retries until the budget is exhausted instead of best-effort completing. On a pre-existing corrupted database this converts a formerly-succeeding delete into a hard failure whose only exit is CHECK DATABASE. That is the right correctness call and it is documented in the release notes, but two things are worth confirming:

1. CHECK DATABASE is reachable while deletes are blocked - i.e. the repair path itself uses the best-effort getEdgeHeadChunk (it does, per GraphDatabaseChecker) and is not gated behind the same strict walk. Worth a one-line note in the release doc that repair-then-retry is the recovery, since a user hitting this will see a delete that never succeeds.
2. Retry pressure on hot super-nodes: promoting the cross-page publication window to a CME means a very hot vertex under heavy delete+append contention will retry more often than before. The reported workload passes with the default budget, and TX_RETRY_DELAY mitigates, but this is a genuine (accepted) shift from "silently wrong" to "occasionally retries" - fine, just flagging that the cost lands on the exact super-node shape the bug targeted.

Minor

• resolveEndpointToDisconnect does existsRecord(endpointRID) and then getOutVertex()/getInVertex(), i.e. up to two resolutions of the same endpoint. This matches the pre-existing code so it is not a regression, but on a delete-heavy path the existsRecord check is arguably redundant with the subsequent lazy load that would surface the same RecordNotFoundException (now caught and mapped to null). Not blocking.
• Tests: the deterministic cases assert their preconditions plus the offending RID in the message, so they cannot pass on an unrelated throw - good discipline. The reproduction test is correctly @Tag("slow") and restores TX_RETRY_DELAY. Nice touch overriding isCheckingDatabaseIntegrity() since the fixtures intentionally corrupt chains.

Style / conventions

Consistent with CLAUDE.md: final throughout, imports (no FQNs), no System.out, no Claude attribution, assertThat(...) style, @Tag("slow") on the long test. Docs updated. The Javadoc-as-rationale density is high but genuinely useful here given how subtle the invariants are.

Overall: solid fix with a correctly-scoped blast radius and an honestly-documented trade-off. My only ask before merge is the release-note line making the CHECK DATABASE recovery explicit for anyone who hits the "genuinely broken list -> delete now fails" case, and confirmation that #5680 (deleteVertex tolerance) is the right home for the remaining asymmetry. LGTM otherwise.

Both asks addressed in ba56346.

1. CHECK DATABASE recovery is now explicit in the release note. Verified your reading before writing it down: GraphDatabaseChecker uses getEdgeHeadChunk exclusively (4 call sites, all best-effort), and getEdgeHeadChunkForWrite has exactly one caller, deleteEdge — so the repair path is not gated behind the strict walk and cannot be blocked by the delete being blocked. The note now gives the user the whole loop: the symptom (a delete that keeps failing however often it retries, whereas ordinary contention succeeds on a retry), the recovery (CHECK DATABASE ... FIX, then retry), and why the repair still works.

2. Retry pressure on hot super-nodes — recorded in the same section as the accepted shift it is, "from quietly wrong to occasionally repeated", landing on exactly the super-node shape the bug affected, with arcadedb.txRetryDelay / arcadedb.txRetries named as the levers.

3. #5680 is the right home — confirmed, and it already carries the full picture: the original deleteVertex-own-list tolerance, the neighbour case from review 2, and the observation that force does not currently help (the per-edge loop sits outside its promise), which is the natural shape for a fix if that issue resolves toward tolerance.

On the minor point — the existsRecord check turns out not to be redundant, and I have commented it rather than removed it. Edge.getOutVertex()/getInVertex() load with loadContent=false, which returns a lazy handle without touching the bucket, so a deleted endpoint does not surface at that resolution at all. It surfaces later, inside getEdgeHeadChunkForWrite, which maps it to a retryable conflict. So existsRecord is precisely the discriminator between "vertex gone → nothing to disconnect, tolerate" and "vertex present but its list unreadable → retry"; dropping it would silently reclassify the first as the second. headChunkForWriteRaisesRetryableConflictWhenTheVertexItselfVanishes exercises exactly that lazy path. Good catch to raise it — it is the kind of thing that reads as dead weight and is load-bearing.

Graph package after the change: 334 tests, 0 failures.

Code review: #5678 - edge-delete dangling back-reference fix

Reviewed all five changed files (GraphEngine, EdgeLinkedList, EdgeIteratorFilter, StripedEdgeList, and the new test), the diff, and the release note. This is a high-quality, well-reasoned fix. Summary and a few observations below.

What's good

• Root cause is precise and the fix is symmetric with existing code. Turning an unreadable chunk on the removal walk into a retryable ConcurrentModificationException mirrors what the append path (getOrCreateEdgeList, EdgeLinkedList.add head-flip guard, StripedEdgeList.addChain) already does. Making the write path strict while leaving reads best-effort is the right line to draw, and it is drawn consistently across getEdgeHeadChunkForWrite, loadChunkForWrite, and readChunk.
• The OUT/IN split (resolveEndpointToDisconnect) is the crux and it is correct. Tolerating only a vanished endpoint vertex while letting a failure on a still-existing vertex's list reach the caller is exactly what closes the window. The existsRecord + lazy getOutVertex()/getInVertex() interplay is subtle, but the inline comment justifies why the check is not redundant (lazy handle vs. materialized load), and headChunkForWriteRaisesRetryableConflictWhenTheVertexItselfVanishes pins that the lazy-load path maps RNF to a NeedRetryException.
• Dead code removed (StripedEdgeList.loadStripeHead) rather than left behind - confirmed no remaining references.
• Test coverage is excellent. Two deterministic reproducers (head-chunk and mid-chain windows) plus the @Tag("slow") concurrent reproducer, plus the vertex-vanish and neighbour-unreadable second-order cases. The deterministic tests assert preconditions (chain spans several chunks, victim sits behind the hole, degree is right) and match the offending chunk RID in the exception message, so they cannot pass on an unrelated throw. isCheckingDatabaseIntegrity() is correctly disabled since the tests deliberately corrupt chains, and the slow test still runs an explicit CHECK DATABASE on its clean rounds.
• Follows the repo conventions: @Tag("slow"), final params, assertThat(...) style, no new dependencies, authorship credited to the maintainer (not Claude).

Observations / questions (none blocking)

1. New failure mode: SchemaException on the chain-mutation path now propagates. Previously the whole disconnection sat in catch (SchemaException | RecordNotFoundException); now resolveEndpointToDisconnect catches those only around endpoint resolution, while getEdgeHeadChunkForWrite converts only RecordNotFoundException to CME. A SchemaException surfacing during buildEdgeList/chain walk on a healthy vertex would now fail the delete outright (not retryable). This looks intentional and consistent with the strict design, but it is worth confirming no realistic transient raises SchemaException (rather than RNF) on the write walk under concurrency, since that would fail instead of retry.

2. Read-side strictness on super-nodes. chainsForNeighbour(..., strict=true) feeds not only removals but the neighbour-keyed reads isVertexConnectedTo/containsVertex/getFirstEdgeConnectedToVertex, so those reads can raise CME on a super-node with a transiently unreadable stripe chain. This is pre-existing (unchanged by this PR), so the release note's "read paths are unchanged" is accurate in context - just flagging it since it is an exception to the general "reads are best-effort" contract for anyone auditing later.

3. Concurrent test robustness (minor). In concurrentDeleteAndAppendNeverLeaveADanglingBackReference, worker threads catch Exception but not Error, and done.await() has no timeout. An unexpected Error (or an AssertionError thrown off the main thread) would leave the latch un-counted and hang the test rather than failing it. Not a correctness concern for the fix; a bounded done.await(timeout, ...) would make a future regression fail fast instead of hanging CI.

4. Pre-existing stale comment (drive-by, out of scope). In deleteEdge, the edge-record deletion carries the comment "Use the database's delete method to ensure proper index cleanup instead of directly calling bucket.deleteRecord()" immediately above a direct bucket.deleteRecord(...) call. The comment contradicts the code; not introduced by this PR, but a natural spot to correct it.

Verification note

I could not run the Maven build in this review sandbox (it requires an interactive approval unavailable here), so I did not independently reproduce the suite results. The PR reports the full engine suite green across three runs with the deterministic reproducers failing on main and passing with the fix, and the code and tests read as consistent with that.

Overall: clean, well-documented, correctly scoped fix with strong regression coverage. LGTM pending the maintainer's own green CI run.

Thanks — items 3 and 4 are fixed in e484f23; 1 and 2 I looked into and am leaving as-is, reasoning below.

3. Concurrent test robustness — fixed. Workers now catch Throwable (an Error could previously skip the countDown) and the latch wait is bounded at 5 minutes against a round that takes seconds. A wedged worker now fails the test instead of hanging CI.

4. Stale comment — fixed, after checking which half was wrong. Rather than assume the comment, I deleted an edge carrying an indexed property and watched the index: 1 entry → 0, lookup empty. So the code is right and the comment was inverted. LocalDatabase.deleteRecordNoLock cleans the index entries and fires the delete events before dispatching an Edge to deleteEdge, so this call is deliberately the physical removal alone — routing back through the database would repeat that work, not add it. The comment now says that, with the reason.

1. SchemaException on the chain-mutation path — intentional, and I would rather not map it. The realistic source is the bucket lookup in loadChunkForWrite / buildEdgeList, i.e. the edge bucket no longer resolving — which means the schema genuinely changed (type or bucket dropped), not that a page is momentarily unpublished. A retry cannot fix that, so mapping it to CME would burn TX_RETRIES and then report "concurrent commit in flight" for what is actually a schema error. Failing directly with the real exception is the more useful outcome, and it is strictly better than the old behaviour, which swallowed it and deleted the edge record anyway — the same silent-corruption shape this PR is closing. Worth noting the precedent points the same way: #5608 changed TransactionContext.edgeSegmentPageKey to a null-tolerant bucket lookup specifically so a transient miss became retryable, while leaving genuine schema errors as errors.

2. Read-side strictness via chainsForNeighbour(strict=true) — agreed, and confirmed unchanged by this PR (it predates it and I have not touched addChain). Flagging it in the thread is the right place for it; if it deserves to be reconsidered, that is its own change with its own reasoning, not a rider here.

Graph package after the change: 334 tests, 0 failures. Full engine suite was green on this branch before the merge from main (10587 tests, 0 failures) and CI is now running it on the merged tree.

Review: fix(engine) #5670 - unreadable edge-list chunk retries the delete instead of skipping it

Thorough, well-reasoned change. The core idea is sound: the append path (getOrCreateEdgeList, StripedEdgeList.addChain) already converts the "cross-file commit publication" window into a retryable ConcurrentModificationException, and the removal path was the asymmetric hole that read the same transient as "nothing to remove" while deleting the edge record anyway. Making the removal path strict is the right line to draw, and the split of endpoint resolution (resolveEndpointToDisconnect) from chain mutation (getEdgeHeadChunkForWrite) is a clean way to keep "vertex gone = tolerated" apart from "vertex present but list unreadable = conflict".

I traced every caller of the now-strict methods (removeEdge, removeEdgeRID, readChunk, loadChunkForWrite, StripedEdgeList.addChain(strict)):

• GraphEngine.deleteEdge (lines 514/521): intended strict path.
• EdgeIteratorFilter.handleCorruption (line 108): correctly kept best-effort - it runs the prune inside database.transaction(...) and now catches NeedRetryException, leaving the ghost for a later pass. Good.
• GraphDatabaseChecker (1114/1150) and CSRBuilder (689): read-only containsEdge/iteration through best-effort getEdgeHeadChunk, unaffected.

Atomicity is preserved: if OUT removeEdge mutates and then IN resolution/getEdgeHeadChunkForWrite throws CME, the whole transaction rolls back before the edge record is deleted, so the retry re-reads a consistent view. Confirmed the head read stays inside the try in getEdgeHeadChunkForWrite so a vertex deleted since resolution surfaces as a retryable conflict rather than a raw RecordNotFoundException (pinned by headChunkForWriteRaisesRetryableConflictWhenTheVertexItselfVanishes).

Points worth confirming

1. Broadened behavior change via deleteVertex -> deleteEdge. The PR frames the visible change as "edge.delete() can now raise CME", but it also reaches DELETE VERTEX: each neighbour-edge disconnect goes through the strict getEdgeHeadChunkForWrite on the neighbour's list, so deleting a healthy vertex whose neighbour list is momentarily unreadable now aborts with CME (correctly pinned by deletingAVertexWhoseNeighbourListIsUnreadable...). Under transient contention the retry resolves it. The concern is a genuinely corrupt (non-transient) neighbour list: such a vertex is now undeletable via the normal path and needs CHECK DATABASE. Issue4420/Issue4432 pin the vertex's own broken list, not a neighbour's. This is exactly the exposure deleteVertex can delete a vertex without collecting its edges when a chunk is transiently unreadable (the #5670 window, vertex side) #5680 tracks, so it is acknowledged, but it is the broadest-impact part of the change and deserves the same prominence in the release notes as the edge-delete case.

2. Permanent-failure trade-off. A genuinely lost chunk (not transiently invisible) is indistinguishable from a transient one here, so edge.delete() now fails on every retry instead of completing best-effort. This is the deliberate design, documented in the getEdgeHeadChunkForWrite Javadoc and release notes, with CHECK DATABASE as the repair path. Just flagging that this converts a previously-silent "success" into a hard, retry-exhausting failure for that (rare) case - correct, but a real operational change.

3. Minor performance on the delete path. resolveEndpointToDisconnect adds an existsRecord(endpointRID) per endpoint (2 per edge delete). existsRecord takes the DB read lock and does a page-slot existence check. It is load-bearing for the tolerance distinction, so it is justified, but on a super-node teardown (many edges) it is a new per-edge lock+lookup on top of the chain walks. Likely negligible next to the chain I/O, but worth being aware of.

4. existsRecord(null) throws IllegalArgumentException. In resolveEndpointToDisconnect, if endpointRID were ever null the call throws IllegalArgumentException, which is not caught by the catch (SchemaException | RecordNotFoundException) and would escape as a non-retryable error. Edges always carry both endpoints, so this is theoretical, but a defensive null check (return null) would match the "nothing to disconnect" intent.

Test coverage

Excellent. The deterministic pair asserts real preconditions (chain spans several chunks, victim sits behind the hole, degree correct before corruption) and matches the offending chunk RID in the exception message, so they cannot pass on an unrelated throw. The @Tag("slow") concurrency test reproduces the reported shape and counts any throwable as a failure rather than tolerating retries. isCheckingDatabaseIntegrity() is correctly disabled since the tests deliberately corrupt chains, and assertIntegrityClean() is asserted explicitly at the end of the concurrent test instead. Tagging follows CLAUDE.md.

Nits

• The inline comments are extremely verbose, but that matches the established house style in this package/module, so no change requested.

Overall this is a high-quality, well-tested fix that closes a real correctness hole and is honest about the trade-offs it makes and the follow-ups it leaves (#5679, #5680). LGTM, with the release notes ideally calling out the DELETE VERTEX reach (point 1) as explicitly as the edge-delete case.

Both actionable items in d73cad5.

1. DELETE VERTEX prominence — agreed, and you are right that it is the widest-reaching of the three. The note led with edge.delete() and mentioned the vertex reach only at the tail of the trade-off paragraph. The visible-effect section now opens with "and it is not limited to deleting an edge", lists the three operations that share the contract (edge.delete()/DELETE EDGE, moveEdge, vertex.delete()/DELETE VERTEX), and calls out that the vertex case lands the strict read on a neighbour's list — so a vertex that is itself perfectly healthy can fail to delete. The trade-off paragraph now states the operational consequence in the same terms: a healthy vertex next to a corrupted one is not deletable by the normal path until the corruption is repaired.

4. Null endpoint — guarded. You are right that existsRecord(null) raises IllegalArgumentException, which the catch (SchemaException | RecordNotFoundException) does not cover and which is not retryable — a hard failure escaping the one method whose job is to decide what is tolerable. resolveEndpointToDisconnect now returns null for a null endpoint RID, matching the "nothing to disconnect" intent. Theoretical since an edge always carries both endpoints, but it is a guard worth having in exactly that method.

2. Permanent-failure trade-off — acknowledged, and now stated in the release note in operational terms alongside the CHECK DATABASE ... FIX recovery and the symptom that distinguishes it from ordinary contention (a delete that keeps failing however often it retries).

3. existsRecord cost on super-node teardown — noted and accepted. It is load-bearing (it is what separates "vertex gone, tolerate" from "list unreadable, retry"), and a page-slot existence check per endpoint is small next to the chain walk that follows it. If it ever shows up in a teardown profile, the fix is to fold the existence answer into the resolution rather than to drop the distinction.

Graph package after the change: 334 tests, 0 failures — including main's new EdgesConnectedToTest from the #5660 merge.

Up to standards ✅

_{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.}

Review: fix(engine) 5670 - unreadable edge-list chunk retries the delete

Thorough, well-reasoned change. The core insight is sound: the removal path was treating 'chunk transiently unreadable' (a timing fact) as 'nothing to remove here' (a graph fact), and then deleting the edge record anyway - dangling the back-reference. Making the write/removal path symmetric with the append path (getOrCreateEdgeList / StripedEdgeList.addChain), which already answered this exact window with a retryable ConcurrentModificationException, is the right line to draw. I verified the claims below against the tree.

Strengths

• Read/write split is clean. getEdgeHeadChunk stays best-effort (null on a miss) for the many read/count/iterate callers; the new getEdgeHeadChunkForWrite has exactly one caller (deleteEdge), so the strict contract does not leak into read paths. Confirmed GraphDatabaseChecker still uses only the best-effort getEdgeHeadChunk, so CHECK DATABASE ... FIX remains a viable repair path even when the strict delete refuses - the documented recovery actually holds.
• buildEdgeList extraction removes the duplicated striped-vs-classic head dispatch between the two methods - good reuse, no behavior drift.
• Removing loadStripeHead rather than leaving it as dead code is the right call; the StripedEdgeList javadoc/comment cross-references were updated to point at loadChunkForWrite consistently.
• The head-RID read moved inside the strict try (commit 2) is a real catch: on a lazy ImmutableVertex, getInEdgesHeadChunk() lazy-loads and can raise a non-retryable RecordNotFoundException, which would have defeated the whole point. Nice that it is pinned by headChunkForWriteRaisesRetryableConflictWhenTheVertexItselfVanishes.
• Test coverage is excellent. Two deterministic tests that assert their own preconditions (chain really spans multiple chunks, victim sits behind the hole, degree is right) and match the offending chunk RID in the exception message, so they cannot pass on an unrelated throw; plus the reported concurrent shape under Tag('slow'). isCheckingDatabaseIntegrity() correctly disabled since the fixtures deliberately corrupt the chain. Good adherence to the repo assertThat(...) style and Tag conventions.

Points worth considering

1. EdgeIteratorFilter catches NeedRetryException, which is broader than the CME the comment describes. The comment explains the intent (opportunistic prune inside a READ, leave the ghost for a later pass), and swallowing here is correct for that intent - but the catch will also absorb any other future NeedRetryException subclass surfacing from removeEdgeRID. That is probably fine given the best-effort contract, but a catch (final ConcurrentModificationException ...) would match the stated reasoning more tightly and avoid silently masking an unrelated retryable condition later.

2. buildEdgeList still casts the head record to EdgeSegment unguarded. Under the very window this PR targets, a deleted chunk slot could in principle be reused, so lookupByRID could return a record that is neither StripeDirectory nor EdgeSegment, yielding a ClassCastException (not retryable) rather than the intended CME. This is pre-existing (the cast was inlined in getEdgeHeadChunk before) and low-risk since edge chunks live in their own bucket, so I would not block on it - but since this change is explicitly about turning transient-window failures into retries on the write path, it may be worth mapping a ClassCastException from the write-side lookup to a CME too, for completeness.

3. deleteVertex edge-delete loop propagates the new CME uncaught (the for-loop over edgesToDelete only catches RecordNotFoundException). This is intended and documented - the CME is a NeedRetryException so the surrounding transaction rolls back and retries cleanly - but it does mean a healthy vertex can now fail to delete because of a neighbour unreadable list. The release note gives this appropriate prominence (good), and issue 5680 tracks the tolerance question. Worth double-checking that the server-side auto-retry path and any batch/DELETE VERTEX callers that manage their own transactions surface a sensible message rather than a raw stack, given the increased retry pressure on super-nodes you flagged.

Nits

• The docs and comments are unusually rich - genuinely helpful for a subtle MVCC/visibility bug. No action needed; just noting it lands on the right side of over-documenting for code this timing-sensitive.

Overall: correct root-cause, minimal and symmetric fix, strong deterministic + stress coverage, and the behavioral change is called out with a concrete recovery procedure. The points above are minor/optional. Nice work.

All three checked; one produced a change (b0271ab), two I am deliberately not making — reasoning below.

1. EdgeIteratorFilter catching NeedRetryException rather than CME. I checked the narrowing before applying it, and it would make behaviour worse. NeedRetryException has exactly two subclasses: ConcurrentModificationException and LockTimeoutException. The second says the same thing about this prune — come back for it later — and letting it escape would fail a read because an optional repair could not get a lock, which is exactly the outcome the catch exists to prevent. So the comment was the half that was wrong: it now says the condition being absorbed is "retry later", not one specific cause. Code unchanged, comment now matches it.

2. Unguarded EdgeSegment cast in buildEdgeList. Agreed it is pre-existing and low-risk (edge chunks live in their own *_in_edges/*_out_edges and stripe-pool buckets, so a reused slot is still an edge-list record). I would rather not map ClassCastException to CME, though: unlike an unreadable chunk, a vertex head pointing at something that is not a chunk is not a timing fact — it is corruption a retry cannot fix. Converting it would burn TX_RETRIES and then report "concurrent commit in flight" for a structural problem, trading a precise stack for a misleading message. The distinction this PR is built on is exactly "transient ⇒ retry, structural ⇒ report", and a CCE falls on the structural side.

3. Server-side surfacing of the new CME. Verified rather than assumed: AbstractServerHttpHandler already catches NeedRetryException and answers HTTP 503 "Cannot execute command", logging at FINE with the exception message — a retryable status and a clean message, not a raw 500 stack. So DELETE VERTEX hitting the new conflict over HTTP surfaces sensibly today, and a client that retries on 503 behaves correctly. Nothing to change.

Graph package: 334 tests, 0 failures.

Review: #5670 - unreadable edge-list chunk retries the delete instead of skipping it

This is a genuinely high-quality fix. The root-cause narrative is precise, the fix follows a line the codebase already draws on the append path (getOrCreateEdgeList / StripedEdgeList.addChain answering the same transient with a retryable ConcurrentModificationException), and it stays inside the established page-level MVCC/retry contract rather than inventing a new mechanism. I verified the core claims against the tree and they hold up.

Correctness - verified

• readChunk's three callers are all removal walks (removeEdge, removeEdgeRID, removeVertex). Converting RecordNotFoundException to a retryable CME there does not touch any read/iteration path, so the "reads stay best-effort" contract is intact.
• getEdgeHeadChunkForWrite has exactly one caller (deleteEdge), and GraphDatabaseChecker reads through the best-effort getEdgeHeadChunk - so the "CHECK DATABASE is not blocked by the strict delete" recovery story is real, not aspirational.
• The buildEdgeList extraction preserves read-side behaviour - getEdgeHeadChunk still catches only RecordNotFoundException and its lookupByRID cannot surface a CME, so the read side never sees the new exception.
• deleteVertex's per-edge delete loop catches only RecordNotFoundException, so the new CME from a neighbour's unreadable list correctly propagates out and rolls the whole vertex delete back - exactly what deletingAVertexWhoseNeighbourListIsUnreadableReportsAConflictRatherThanDanglingTheReference pins. The edge collection loops keep their broad catch (Exception) + getEdgeHeadChunk, so deleteVertex retains its documented tolerance while collecting. That split is the right seam.
• The EdgeIteratorFilter swallow is safe: the CME is raised inside the removal walk before any chunk is mutated (the hop readChunk is a pure read; loadChunkForWrite may anchor a page but never modifies before throwing, and an unmodified anchored page is pruned at commit), so absorbing it and leaving the ghost for a later pass cannot corrupt the enclosing read transaction.
• No dead imports: StripedEdgeList still uses RecordNotFoundException (line 453) after loadStripeHead was removed; EdgeIteratorFilter's new NeedRetryException import is used and the existing ones remain live.

Test coverage

Strong. The two deterministic tests assert their preconditions (chain really spans several chunks, victim really sits behind the hole, degree is right beforehand) and match the offending chunk RID in the exception message, so they cannot pass on an unrelated throw. headChunkForWriteRaisesRetryableConflictWhenTheVertexItselfVanishes closes the non-retryable-escape gap the review caught mid-PR. The @Tag("slow") concurrent test reproduces the reported shape and now catches Throwable with a bounded await, so a wedged worker fails rather than hangs. Good adherence to the @Tag("slow") convention in CLAUDE.md.

Minor observations (non-blocking)

1. Global config mutation in the slow test. concurrentDeleteAndAppendNeverLeaveADanglingBackReference sets GlobalConfiguration.TX_RETRY_DELAY to 1 and restores it in finally. Since that global is process-wide, a JVM running test classes in parallel could observe the temporary value. It is restored correctly and engine tests generally run sequentially per class, so this is a theoretical note rather than a defect - TX_RETRY_DELAY is SCOPE.DATABASE, so if you wanted full isolation you could set it on the database instance instead of the global.
2. Comment/doc density. The javadoc and inline commentary here is unusually thorough (a real asset for a subtle concurrency fix), though a few blocks restate the same publication-window explanation across loadChunkForWrite, readChunk, getEdgeHeadChunkForWrite and the release note. Not worth trimming given the subtlety, just noting it.

Style / conventions

Matches the surrounding code: final on locals and params, single-statement if without braces, imported class names rather than FQNs, assertThat(...).isTrue() style in tests, Apache headers on the new file. No System.out, no new dependencies, no author attribution added. The corrected bucket.deleteRecord() comment (index cleanup already done upstream in LocalDatabase.deleteRecordNoLock) is a nice catch - the old comment said the opposite of the code.

Performance

The hot removal walk gains only a try/catch around readChunk/loadChunkForWrite; a try with no exception thrown is free on the JVM, and the retry-pressure shift on hot super-nodes is explicitly acknowledged and bounded by txRetryDelay/txRetries. No new allocations on the success path.

Verdict: LGTM. No blocking issues found. The behaviour change (edge/vertex/move delete can now raise a retryable CME under contention) is correctly surfaced in the release notes with a concrete recovery path, and the "left out deliberately" section (#5680 for deleteVertex's tolerant collection) draws a defensible boundary.

Thanks. Checked the one actionable minor; it turns out not to apply, and I am leaving the test as it is.

1. TX_RETRY_DELAY per-database instead of global. Good instinct — the setting is declared SCOPE.DATABASE, and engine/CLAUDE.md even recommends exactly that for the lock timeouts. But the retry loop does not read it that way:

// LocalDatabase.transaction(...), line 1387
final int retryDelay = GlobalConfiguration.TX_RETRY_DELAY.getValueAsInteger();

That is the global static, not the database's ContextConfiguration, so setting it on the instance would have no effect on the retry delay and the test would silently lose the tightening it is asking for. Set-global-and-restore-in-finally is the only form that works here, and it matches the sibling ConcurrentEdgeAppendMergeTest. (The mismatch between the declared SCOPE.DATABASE and the global read is a real if minor inconsistency in the engine — worth its own look, not a rider on this PR.)

2. Comment density — noted, and I agree with your conclusion; leaving it.

That closes every point raised across the review rounds. Summary of what changed as a result of them:

Follow-ups filed: #5679 (load-sensitive ExplicitLockingTransactionTest) and #5680 (deleteVertex tolerance, incl. the neighbour case and that force does not currently help).

Codecov Report

❌ Patch coverage is 55.00000% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.70%. Comparing base (af0dc71) to head (b0271ab).
⚠️ Report is 3 commits behind head on main.

@@             Coverage Diff              @@
##               main    #5678      +/-   ##
============================================
- Coverage     67.46%   66.70%   -0.77%     
- Complexity        0     1113    +1113     
============================================
  Files          1770     1770              
  Lines        150057   150083      +26     
  Branches      31806    31811       +5     
============================================
- Hits         101236   100110    -1126     
- Misses        35645    36938    +1293     
+ Partials      13176    13035     -141     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.