fix(#6083): reclaim orphan edge records and report an exact edge count on a failed GraphBatch flush - #6089
Conversation
…t on a failed GraphBatch flush Follow-ups 1, 2, 4 and 5 from #6083 (#6070 / PR #6076). Items 1, 2 and 4 all sit on the same code path and are taken together. Item 2 - a failed edge flush left orphan edge records. A GraphBatch edge is written in two steps that do not share a transaction: PHASE 1 of flush() creates the record, PHASE 3 links it into its source vertex's edge list. Both flush paths commit in between - the parallel one must (the record pages have to be released before the per-bucket tasks touch them), the sequential one does whenever commitEvery > 0. A connect pass dying part-way therefore left edge RECORDS behind that no vertex points at: countType() counted them, no traversal reached them, and nothing ever reclaimed them. CHECK DATABASE does not rescue them either. Its checkEdges pass does scan every edge record and probe both endpoints for a back-reference, but a miss only increments the aggregate missingReferenceBack counter - no warning naming the record, no entry in corruptedRecords, so nothing for FIX to delete. The same counter is raised by a healthy UNIDIRECTIONAL edge, legitimately absent from its target's IN list, so it cannot be read as an orphan count. (FIX does reclaim orphaned edge SEGMENTS, but those are the internal linked-list chunks, not these records.) So the flush now cleans up after itself before the failure propagates, through database.deleteRecord - the one inverse correct for both creation routes (createRecordsBulk + indexEdgeProperties for a single-bucket flush, an ordinary save() for a multi-type one) and for every index kind, with no second copy of the key-derivation logic to keep in step. Best-effort: what it cannot reclaim is logged and counted, never allowed to replace the caller's failure. A partially-failed flush also used to skip PHASE 4 entirely, leaving the edges it HAD connected without their IN back-pointer - half-edges the integrity checker flags. PHASE 3's failure is now captured rather than thrown so PHASE 4 queues the incoming side for exactly the edges that survived, and the failure is rethrown after. A failed flush now leaves only whole edges. Item 4 - the partial-commit edge count was a lower bound, now exact. totalEdgesCreated advanced a whole flush at a time, so a load dying mid-flush reported zero for a flush that had made some of its edges durable. It now advances one durable commit at a time, on the failure path as well as the success one. A caller reconciling a failed bulk load re-sends exactly what is missing instead of over-sending a flush's worth and relying on the duplicates being harmless. Also fixed, found by the new test: the SEQUENTIAL outgoing path had no undo log for deferredOutHead, so a rolled-back segment RID survived in the map and batchUpdateVertexHeadChunks() stamped it onto the vertex at close() - CHECK DATABASE reports "out edges record is not valid" and no later read can follow it. This is the hazard #5950 cycle 3 fixed for the IN direction and cycle 4 for the parallel OUT direction; this path was the one left. Same remedy. The final commit moved inside the pass so the undo log covers it too. Item 1 - vertex_batch_size was clamped silently. The cap is right, but it applied with no log line, so a caller who asked for more got something else and was never told. Now a WARNING naming the requested and the effective value. Item 5 - the two contradictory precedents for reporting a failed bulk stream are settled and written down on graphBatchLoad: a failed stream terminates with onError and carries its counters in the trailers; insertStream is documented as the predating exception, not a second precedent. Items 3, 6 and 7 are not addressed here. 3 (the leader's gRPC address is not discoverable) needs a new field in the HA server-list format and a real cluster test, so it gets its own PR. 6 and 7 were recorded in the issue as deliberately declined. Tests: Issue6083OrphanEdgeRecordTest covers both flush paths for orphan cleanup and exact counting, plus the flush that fails before its first commit and so has nothing durable to reclaim. Every assertion was mutation-checked - disabling the reclaim, the exact counting and the new undo log each makes a test fail, as does removing the clamp warning. Issue6070GraphBatchLoadHardeningIT's trailer test now asserts the exact count and that countType agrees with a traversal, which is what it could not do before; it also gains the two clamp-warning tests, and its shared vertex helper now sets the property another test declares MANDATORY, so the class no longer depends on method order.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Coverage variation | ✅ -6.55% coverage variation |
| Diff coverage | ✅ 95.68% diff coverage |
Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (cf6273f) 155184 121450 78.26% Head commit (b945c83) 187657 (+32473) 134568 (+13118) 71.71% (-6.55%) 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 (#6089) 185 177 95.68% 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.
Review of #6089This is a carefully engineered fix with unusually thorough in-code rationale for a genuinely subtle two-phase-commit problem (edge record creation vs. linking). The overall design (capturing PHASE 3 failures instead of throwing immediately, reclaiming orphan records, tracking exact durable counts) is sound, and the reasoning in the comments checks out against the transaction/commit semantics elsewhere in the engine. One real accounting bug found, plus a couple of lower-severity notes. Bug:
|
…om both head-chunk undo logs Code review cycle 1 on PR #6089. Accounting bug in reclaimOrphanEdgeRecords. With commitEvery > 0 the reclaim pass commits several times, but it folded its reclaimed/leaked counters into the instance totals only once, after the loop. A commit failing LATER in the same pass therefore discarded the reclaims an earlier commit had already made durable, and the outer handler blamed the whole [from,to) range as leaked - under-reporting reclaims and counting records that were in fact already gone. It also blamed lightweight edges, which never had a record to leak. Both counters now advance as each commit returns, and the failure path charges only the slice that commit was covering, sized by countOrphanCandidates() so lightweight edges are excluded. This is the one path whose entire purpose is to leave an accurate count behind, so "close enough" was the wrong answer. Covered by a new test that injects a failure into the SECOND reclaim commit via a TEST_BEFORE_ORPHAN_RECLAIM_COMMIT_HOOK, following the fault-injection hooks this class already carries for the vertex and edge commit paths. It asserts what the database actually holds: stored-minus-reachable equals the leaked count, and reclaimed plus leaked accounts for every orphan candidate exactly once. Mutation-checked - restoring the deferred fold makes it fail. The test deliberately leaves unreclaimed orphans behind, so it removes them before the shared teardown's integrity check runs; they trip that check only because the bogus source bucket used to induce the failure makes them dangling links, not because they are orphans. Boxing in the undo logs. outHeadUndoLog is allocated on every sequential flush and takes an entry per distinct source vertex, which is exactly what LongObjectHashMap exists for in this class (~16 bytes/entry against 72-90 for HashMap<Long, RID>). Switched, iterating with forEach() in the catch block. A null VALUE still records "the vertex had no deferred head" faithfully: occupancy lives in the key array, so containsKey() answers true for a key put with a null value. The IN side's inHeadUndoLog is converted too. It predates this PR and had the same cost, and leaving two identical structures with different collection types a few hundred lines apart is a worse trap than the allocation. Covered by the existing #5950 tests.
|
Thanks - all three points were valid and are addressed in d7f2775. The accounting bug: fixed, and it was slightly worse than described. Confirmed exactly as reported: with
Both counters now advance as each commit returns, and the failure path charges only the slice that commit was covering, sized by a new Covered by (The test deliberately leaves unreclaimed orphans behind, so it deletes them before the shared teardown's integrity check. Worth noting why they trip that check: not because they are orphans - CHECK DATABASE has no finding for that, which is the whole premise of item 2 - but because the bogus source bucket used to induce the failure makes them dangling links.) The boxing: taken, and extended to the IN side. You were right that it is consistent with existing precedent rather than a new regression, but also right that it is now allocated on every sequential flush. I also converted the pre-existing The benchmark: my description was wrong, and I have corrected it. There is no |
ReviewI read through the full diff and traced the accounting/transaction logic in CorrectnessThis is a genuinely tricky area (two-phase edge write that spans commits, with both a parallel and a sequential connect path, each with their own partial-failure semantics), and the implementation holds together under close tracing:
I didn't find a correctness bug in the core accounting/reclaim logic. The mutation-testing claims in the PR description are consistent with what I'd expect given how tightly the boundaries are threaded through. Minor observations
Test coverageThe new ScopeItem 3 (leader gRPC address discoverability) being split into its own PR is a reasonable call given it needs a public config-surface change and a real cluster IT - keeps this PR focused on the flush/reclaim defect cluster. Nice work threading the durability boundaries through both connect paths without duplicating the key-derivation logic for cleanup. |
…cross more than one failed bucket Code review cycle 2 on PR #6089. No correctness findings this round; these are the two actionable observations. CapturingLogger now forwards every record to a real DefaultLogger as well as capturing it. The logger is a process-wide singleton shared with the embedded server under test, so a capturing logger that only captured swallowed anything the server logged while it was installed - including the diagnostics needed to understand a failure inside that window. Tee-ing makes the swap invisible to everything except the assertions, and removes the dependency on these ITs running sequentially. Added aParallelFlushReclaimsEveryFailedBucketNotJustTheFirst. The parallel path reclaims per failed bucket, and the existing test only ever failed one, so the loop was exercised exactly once and a body that stopped after the first bucket would have passed. Two distinct missing source buckets now fail together, and the test pins that both are reclaimed while the buckets that succeeded keep their edges. Mutation-checked - reclaiming only the first bucket makes it fail. The third observation, that the clamp warning could repeat per chunk, does not apply: the block that reads vertexBatchSize sits inside the batch == null first-chunk initialization, so it runs once per stream. The per-chunk options re-read the review referred to belongs to insertStream, a different RPC.
|
Thanks - two of the three taken, one does not apply. Pushed in 7bead5a. 1. Global logger swap: fixed. Valid, and cheap to remove entirely rather than just note. 2. Per-chunk clamp warning: does not apply. The block that reads 3. Multi-bucket reclaim coverage: added. You were right that this was only structurally implied, and the gap was slightly worse than "low risk": with a single failing bucket the reclaim loop runs exactly one iteration, so a body that stopped after the first bucket would have passed the whole suite. Also noted your point about CI is green on my side for the suites this touches: all |
ReviewI traced the durability/accounting logic in Correctness (verified, no issues found)
Minor observations (non-blocking)
Test coverageThorough - both flush paths, the multi-bucket parallel reclaim, the "fails before first commit" edge case, and the reclaim-pass-itself-fails-partway case are all covered, and the PR description states every assertion was mutation-checked. The StyleFollows existing conventions (no curly braces on one-line Nice work tracking down the two-phase-commit-without-shared-transaction hazard and its knock-on effects on the exact edge count. |
…depends on Code review cycle 3 on PR #6089, which found no correctness defects. The only actionable point was a request for exactly this comment: the tee added in cycle 2 stops the swap losing diagnostics, but the swap itself is still process-wide, and that is sound only because these ITs run sequentially in one forked JVM. Spelled out what would break under cross-class parallelism - a test asserting the ABSENCE of a warning could see another test's - so the constraint is written down rather than rediscovered. The other two observations are accepted trade-offs already documented in the code: one begin/commit per failed bucket in the parallel reclaim (per-commit granularity is what makes the accounting exact, and batching would widen the blast radius if the cleanup itself failed), and the endpoint walk deleteRecord performs for a record with no back-reference (the cost of reusing the one delete path correct for both creation routes and every index kind).
|
Thanks - no correctness defects this round, and the one actionable point is addressed in b945c83. Logger swap still being global: comment added. You asked for it to be written down if the execution model ever changes, so I have spelled out the constraint the helper depends on and, specifically, what would break: a test asserting the absence of a warning could see another test's under cross-class parallelism in a shared JVM ( The other two are accepted trade-offs, and both are already documented in the code - recording the reasoning here so it is not re-derived:
I consider the review satisfied: three cycles, one real defect found (the reclaim accounting, cycle 1) and fixed with a mutation-checked regression test, plus two test-quality improvements. Local runs are green for every suite this touches - all |
ReviewI traced the accounting logic by hand across all four flush-outcome combinations (parallel/sequential × success/partial-failure) plus the interrupted-reclaim-pass case, and didn't find a correctness bug. Notes below. What I checked closely
Sequential vs. parallel
Survivor-range construction in Compile-time checks (imports of Minor, non-blocking observations
Nice, thorough piece of work — the two-step create/link split and its failure-mode implications (orphan records, half-edges, the missing OUT undo log) are exactly the kind of thing that's easy to get subtly wrong, and the bookkeeping here holds up under a careful trace. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6089 +/- ##
==========================================
+ Coverage 69.50% 69.53% +0.03%
==========================================
Files 1813 1813
Lines 155184 155314 +130
Branches 32780 32800 +20
==========================================
+ Hits 107858 107999 +141
+ Misses 33773 33744 -29
- Partials 13553 13571 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Closes items 1, 2, 4 and 5 of #6083 (follow-ups from #6070 / PR #6076). Items 1, 2 and 4 all sit on the same code path, which is why they are taken together.
Item 2 - a failed edge flush left orphan edge records
A
GraphBatchedge is written in two steps that do not share a transaction: PHASE 1 offlush()creates the record, PHASE 3 links it into its source vertex's edge list. Both flush paths commit in between - the parallel one must (the record pages have to be released before the per-bucket tasks touch them), the sequential one does whenevercommitEvery > 0. A connect pass dying part-way therefore left edge records behind that no vertex points at:countType()counted them, no traversal reached them, nothing ever reclaimed them.The issue asked whether CHECK DATABASE detects and reclaims them. It does not, and the detail matters:
checkEdgesdoes scan every edge record and probe both endpoints for a back-reference (GraphDatabaseChecker.java:1150-1152, 1183-1185)...missingReferenceBackcounter. No warning naming the record, no entry incorruptedRecords, so nothing for theFIXvariant to delete.FIXdoes reclaim orphaned edge segments - but those are the internal linked-list chunks, not the edge records at issue.)So the flush now cleans up after itself before the failure propagates. Deletion goes through
database.deleteRecord, which is the one inverse correct for both creation routes this class uses (createRecordsBulk+indexEdgePropertiesfor a single-bucket flush, an ordinarysave()for a multi-type one) and for every index kind, without a second copy of the key-derivation logic to keep in step. Best-effort by construction: what it cannot reclaim is logged and counted (getOrphanEdgeRecordsLeaked()), never allowed to replace the failure the caller is already handling.A partial failure also left half-edges
Found while writing the test. A partially-failed flush skipped PHASE 4 entirely, so the edges it had durably connected never got their IN back-pointer - half-edges the integrity checker flags. PHASE 3's failure is now captured rather than thrown, so PHASE 4 queues the incoming side for exactly the edges that survived, and the failure is rethrown after. A failed flush now leaves only whole edges.
Item 4 - the partial-commit edge count is now exact, not a lower bound
totalEdgesCreatedadvanced a whole flush at a time, so a load dying mid-flush reported zero for a flush that had in fact made some of its edges durable. It now advances one durable commit at a time, on the failure path as well as the success one. A caller reconciling a failed bulk load re-sends exactly what is missing instead of over-sending a flush's worth and relying on the duplicates being harmless.Bonus: the sequential OUT path had no undo log (found by the new test)
connectOutgoingEdgesSortedwrotedeferredOutHead/outChunkRIDCachebefore the transaction holding those segments committed, with nothing to undo it. A rolled-back segment RID survived in the map andbatchUpdateVertexHeadChunks()stamped it onto the vertex atclose()- CHECK DATABASE reports"out edges record is not valid"and no later read can follow it.This is exactly the hazard #5950 cycle 3 fixed for the IN direction (
inHeadUndoLog) and cycle 4 for the parallel OUT direction (the per-bucket local maps). This path was the one left. Same remedy, and the final commit moved inside the pass so the undo log covers it too.Item 1 -
vertex_batch_sizewas clamped silentlyThe cap is right - a caller-supplied buffer must not be able to exhaust the server heap on one request - but it applied with no log line, so a caller who asked for more got something else and was never told. Now a
WARNINGnaming both the requested and the effective value.Item 5 - the reporting convention is settled and written down
The two contradictory precedents are resolved on
graphBatchLoad's javadoc: a failed bulk stream terminates withonErrorand carries its counters in the trailers, so the caller keeps its status code and generic error handling while still getting what it needs to reconcile.insertStreamis documented as the predating exception (its summary is a per-chunk report consumed on the success path too, so failure is not a separate channel there) rather than a second precedent.Not in this PR
Testing
Issue6083OrphanEdgeRecordTest(new) covers, for both flush paths:countType == reachable, not an upper bound)getTotalEdgesCreated()equals that same numberEvery assertion was mutation-checked. Disabling the orphan reclaim, reverting the exact counting, and removing the new OUT undo log each makes a test fail; so does removing the clamp warning. (The issue warned that three tests written during #6076 initially passed against the very bug they were meant to catch.)
Issue6070GraphBatchLoadHardeningIT:countTypeagrees with a traversal - which is precisely what it could not do before, and why it had been written around the symptomvertex()helper now also sets the property another test declares MANDATORY, so the class no longer passes or fails depending on method orderSuites run green: all
com.arcadedb.graph.*Testandcom.arcadedb.database.*Test(engine), all 172 grpcw unit tests + theIssue6070*ITs,RemoteGraphBatchTest(network),RemoteGraphBatchIT(server).On throughput (correcting the class name from the original description - there is no
GraphBatchBenchmark):GraphBatchTest.benchmarkBatchVsStandardcovers the parallel path (3.41x over the standard API, unchanged), andperformance/GraphBatchDrainPerfBenchmarkcovers both -parallelFlush=trueat ~653K edges/sec andparallelFlush=falseat ~742K, also unchanged. So the sequential path, which is where the new per-flush undo-log bookkeeping lives, is represented.🤖 Generated with Claude Code