fix(#5667): GraphBatch resumes over an already-promoted super-node vertex instead of hard-failing

[robfrank]

Closes #5667

Root cause

GraphBatch (the bulk graph importer) manages edge segments directly, bypassing EdgeLinkedList/StripedEdgeList. It never calls EdgeLinkedList.tryPromoteToSuperNode(), so a bulk-loaded hub never gets the striped super-node layout (#5156) no matter how many edges land on it.

Worse, if the graph already contains a promoted vertex (created through the standard API, or by a previous non-bulk write), resuming a bulk load over it used to fail outright:

• Sequential flush (getOrCreateOutSegmentDeferred / getOrCreateInSegmentDeferred, used by connectOutgoingEdgesSorted / connectIncomingEdgesSequential): threw a documented IllegalStateException ("Bulk edge import into the super-node promoted vertex ... is not supported").
• Parallel flush (getOrCreateOutEdgeChunk / getOrCreateInEdgeChunk, used by connectOutEdgesRangeLocal / connectIncomingEdgesRangeLocal via the async executor): had no guard at all - it blindly cast the vertex's head chunk record to EdgeSegment, which threw a raw, undocumented ClassCastException (StripeDirectory cannot be cast to EdgeSegment) instead.

Scope of this fix

This PR fixes the hard failure (both shapes above, sequential and parallel), which is the more severe half of the issue - an unhandled exception that aborts the whole batch mid-import, arriving after potentially hours of work. Edges for an already-promoted vertex are now routed through the standard, MVCC-safe StripedEdgeList write path instead of being rejected.

Out of scope: GraphBatch still does not promote a vertex to the super-node layout during a bulk load - a very high-degree vertex loaded entirely through GraphBatch still ends up as a long chained-segment list rather than striped. Implementing promotion inside GraphBatch's own bulk segment-management code (four independent overflow call sites across OUT/IN x sequential/parallel, each needing correct cross-flush degree tracking) is a materially larger change with its own performance and correctness surface; it is intentionally left as a follow-up. The GraphBatch class javadoc now documents this limitation explicitly, and points at arcadedb.graph.supernodeThreshold=0 as the database-wide workaround if a bulk-loaded super-node's degraded traversal performance is a concern.

Changes

engine/src/main/java/com/arcadedb/graph/GraphBatch.java:

• getOrCreateOutSegmentDeferred / getOrCreateInSegmentDeferred (sequential flush): instead of throwing, set new instance fields (lastSegmentPromoted, lastPromotedVertex, lastPromotedDirectory) and return null. Safe as instance-field signalling because these two methods run only on the single-threaded sequential connect path.
• connectOutgoingEdgesSorted / connectIncomingEdgesSequential: check lastSegmentPromoted first and, if set, route the group through the new addGroupThroughStripedEdgeList helper instead of the bulk segment write.
• connectOutEdgesRangeLocal / connectIncomingEdgesRangeLocal (parallel flush, runs concurrently across async-executor threads): check for a StripeDirectory head locally (no shared instance state - see the thread-safety note below) before calling getOrCreateOutEdgeChunk / getOrCreateInEdgeChunk, and route through addGroupThroughStripedEdgeList when found. getOrCreateOutEdgeChunk / getOrCreateInEdgeChunk themselves are left unchanged (no StripeDirectory awareness) since the promoted case is now fully handled by their callers.
• New helper addGroupThroughStripedEdgeList: constructs a StripedEdgeList over the resolved vertex + direction + directory and calls its add() once per buffered edge in the group.
• Class javadoc: documents that GraphBatch never promotes during bulk load, that it resumes correctly over pre-existing promoted vertices, and the supernodeThreshold=0 workaround.

Thread-safety pitfall caught during development

The first version of this fix used the shared lastSegmentPromoted instance-field pattern for getOrCreateOutEdgeChunk / getOrCreateInEdgeChunk too, mirroring the sequential-path methods. That is wrong: those two methods are also called from connectOutEdgesRangeLocal / connectIncomingEdgesRangeLocal, which the parallel flush path dispatches to the async executor - multiple buckets run concurrently on different threads. A shared mutable instance field read immediately after being set raced across threads and silently attributed one group's promoted vertex to a different, unrelated group, inflating an edge count by one in testing. Fixed by keeping the parallel path's promotion check entirely in local variables, never touching shared state. See the regression test's parallel-flush variant, which reproduces both the original ClassCastException (fix reverted) and this thread-safety bug (during development, not present in the final diff).

Tests

engine/src/test/java/com/arcadedb/graph/Issue5667GraphBatchSuperNodeResumeTest.java (new):

• sequentialFlushResumesOverPromotedHub - pre-promotes a hub's OUT and IN lists via the standard API, then bulk-loads one more edge in each direction with parallelFlush(false). Verifies no exception, both edges land, both lists remain StripeDirectory.
• parallelFlushResumesOverPromotedHub - same scenario with parallelFlush(true) (the default), exercising the async range-local path.

Both tests were verified to fail with the pre-fix code: the sequential test reproduces the documented IllegalStateException, the parallel test reproduces the previously-undocumented ClassCastException (StripeDirectory -> EdgeSegment).

Verification run

• mvn -q -pl engine -am compile / test-compile: clean.
• Targeted: Issue5667GraphBatchSuperNodeResumeTest - 2/2 pass.
• Regression sweep: GraphBatchTest, GraphBatchCommitRetryTest, GraphBatchUniqueIndexTest, GraphBatchWALRestoreTest, SuperNodeStripingTest, SuperNodeDefaultThresholdTest, SuperNodeBothSizeQueryTest, Issue5147SuperNodeChunkRaceTest, Issue5666ConcurrentGraphBatchTest - 47/47 pass.
• Full com.arcadedb.graph.*Test sweep (-DexcludedGroups=slow,benchmark) - 208/208 pass, 0 failures/errors.

Test plan

• New regression test Issue5667GraphBatchSuperNodeResumeTest (sequential and parallel flush variants) - both verified to fail against the pre-fix code (IllegalStateException / ClassCastException respectively) and pass against the fix.
• mvn -q -pl engine -am compile and test-compile clean.
• Regression sweep listed above - 47/47 pass.
• Full com.arcadedb.graph.*Test sweep (-DexcludedGroups=slow,benchmark) - 208/208 pass, 0 failures/errors.

Review cycle 1

claude[bot] reviewed and confirmed the core fix is sound (traced the neighbour-vertex argument through all four call sites against EdgeLinkedList.add()'s expected semantics, confirmed the thread-safety fix is correct). "Nothing here blocks merging" - all notes below are optional follow-up, not requested changes, so none were applied in this cycle:

• Stale-cache edge case (pre-existing behavior, not introduced by this diff): a vertex promoted by a third party while this GraphBatch instance is running could have its promotion missed by the RID cache / knownNewVertexKeys fast path. Out of scope per this PR's own stated limits ("promoted before this bulk load ran").
• Test coverage only exercises single-edge groups (groupSize == 1) against a promoted vertex; a multi-edge-group test would additionally cover StripedEdgeList.add()'s stripe-rollover branches.
• Resuming over a large pre-promoted hub re-checks instanceof StripeDirectory on every flush group rather than caching "this vertex is promoted" - consistent with the PR's documented "ordinary (non-bulk) speed" trade-off, not treated as a bug.
• Noted getOrCreateOutEdgeChunkExact/getOrCreateInEdgeChunkExact are unused dead code, unrelated to this PR.

[mergify]

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

• Queue this pull request

[codacy-production]

Not up to standards ⛔

🔴 Issues 3 minor

Alerts:
⚠ 3 issues (≤ 0 issues of at least minor severity)

Results:
3 new issues

Category	Results
CodeStyle	3 minor

View in Codacy

🟢 Metrics 5 complexity

Metric	Results
Complexity	5

View in Codacy

🟢 Coverage 100.00% diff coverage · -6.37% coverage variation

Metric	Results
Coverage variation	✅ -6.37% coverage variation
Diff coverage	✅ 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (ea4f6af)	152855	117429	76.82%
Head commit (41a4ffb)	185521 (+32666)	130714 (+13285)	70.46% (-6.37%)

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 (#5948)	32	32	100.00%

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

[claude]

Review

Reviewed against the code merged into this checkout (d131176, GraphBatch.java, StripedEdgeList.java, EdgeLinkedList.java) plus the new test.

Correctness

The core fix is sound. All four call sites that used to hard-fail on a promoted vertex now route through addGroupThroughStripedEdgeList instead:

• Sequential: getOrCreateOutSegmentDeferred / getOrCreateInSegmentDeferred signal via the new lastSegmentPromoted/lastPromotedVertex/lastPromotedDirectory instance fields, safe because these two methods only ever run on the single-threaded sequential connect path.
• Parallel: connectOutEdgesRangeLocal / connectIncomingEdgesRangeLocal check instanceof StripeDirectory with purely local variables before calling getOrCreateOutEdgeChunk/getOrCreateInEdgeChunk, correctly avoiding the shared-field race the PR description calls out (multiple bucket slots run concurrently on different async-executor threads).

I traced the neighbour-vertex argument through all four call sites (tmpVertexBucketIds/localTmpVertexBucketIds, sourced from edgeDstBucketIds for OUT groups and inVertexBucketIds/localTmpVertexBucketIds for IN groups) against EdgeLinkedList.add(edgeRID, vertexRID)'s expected "neighbour" semantics — all four are correct (dst for OUT, src for IN).

StripedEdgeList is the same MVCC-safe path Vertex.newEdge() already uses, so no new write-path risk is introduced; addGroupThroughStripedEdgeList correctly needs only a read-only Vertex (never mutates the vertex record itself, only the directory/stripe chunks), so it's fine that the sequential path passes the non-.modify()'d VertexInternal while the parallel path happens to pass an already-.modify()'d instance (reused from the non-promoted branch).

Minor / non-blocking observations

1. Stale-cache edge case is still open, out of scope but worth a one-line doc callout. outChunkRIDCache/inChunkRIDCache never cache a StripeDirectory head (the promoted branch returns before the put), so a first encounter with a promoted vertex is always detected correctly. But if a vertex is promoted externally by another writer while this GraphBatch instance is running (between two flush() calls touching the same vertex), a classic-segment RID already cached from an earlier flush would be reused without re-checking, and — more seriously — the knownNewVertexKeys fast path (used for vertices this same GraphBatch just created) skips the vertex load/head-chunk check entirely, so a vertex promoted by a third party in that narrow window would get a plain segment written and then have its StripeDirectory head silently overwritten in batchUpdateVertexHeadChunks(). This is a pre-existing property of the caching/fast-path design (not introduced by this diff) and the PR's own scope note already restricts itself to "promoted before this bulk load ran," so I wouldn't block on it — just flagging in case it's worth a short caveat in the javadoc alongside the other limitations already documented there.

2. Test coverage: only single-edge groups are exercised. Both new tests add exactly one new OUT edge and one new IN edge to the pre-promoted hub, so addGroupThroughStripedEdgeList's for (int k = 0; k < count; k++) loop never iterates more than once. A test that buffers several new edges to the same promoted hub in one flush (forcing groupSize > 1) would additionally exercise StripedEdgeList.add()'s "concurrent head already replaced" / stripe-chunk-rollover branches from within GraphBatch, which the current tests don't reach.

3. Repeated promotion re-detection cost. Since a promoted vertex's head is never cached, every flush group for a hot, already-promoted vertex re-does the database.lookupByRID + instanceof check from scratch rather than caching "this vertex is promoted, skip the classic path" the way the classic-segment RID is cached. Given the PR explicitly documents addGroupThroughStripedEdgeList as "ordinary (non-bulk) speed," this is consistent with the stated trade-off and not worth optimizing here, just noting it's the reason resuming over a large pre-promoted hub will stay slow across many flushes, not just the first one.

4. Dead code note (pre-existing, unrelated to this diff): getOrCreateOutEdgeChunkExact/getOrCreateInEdgeChunkExact are unused and weren't touched by this PR, so they don't need the promotion check added — just mentioning so a future promotion-related change to those doesn't get missed since they look like siblings of the two methods that were patched.

Style / conventions

• Adheres to the repo's final-parameter and brace-omission conventions.
• No new dependencies, no System.out debug leftovers, javadoc-only class comment addition looks consistent with the existing (deliberately unclosed) <p> paragraph style already used in the file.
• The companion docs/5667-*.md file follows the existing per-issue doc convention already used elsewhere in docs/.

Test coverage assessment

The two new tests (sequentialFlushResumesOverPromotedHub, parallelFlushResumesOverPromotedHub) directly reproduce both original failure shapes (IllegalStateException sequential, ClassCastException parallel) and assert both the edge counts and that the layout remains StripeDirectory post-fix — good regression coverage for the reported issue. Combined with the described sweep across SuperNodeStripingTest/GraphBatchTest/etc., this looks like solid coverage for the stated scope. The main gap is #2 above (multi-edge groups against a promoted vertex in one flush).

Overall: this is a careful, well-scoped fix with good reasoning about the concurrency hazard (and evidence the thread-safety bug was actually caught during development, not just asserted). Nothing here blocks merging; the notes above are suggestions for a possible follow-up rather than requested changes.

[claude]

Reviewed the diff (GraphBatch.java + the new Issue5667GraphBatchSuperNodeResumeTest). Overall this is a well-scoped, carefully reasoned fix - the root-cause writeup and the thread-safety pitfall section in the description are genuinely useful, and routing the promoted-vertex case through the real StripedEdgeList write path (rather than trying to special-case the bulk segment writer for stripes) is the right call given the complexity that would add.

Code quality

• Good instinct to keep the sequential path's promotion signal as instance fields (lastSegmentPromoted/lastPromotedVertex/lastPromotedDirectory) but switch to purely local variables on the parallel range-local paths. The comments on both call sites and on the field declarations explain why, which will save the next reader from reintroducing the exact race caught during development.
• addGroupThroughStripedEdgeList is a clean, minimal adapter - it doesn't try to batch/vectorize into StripedEdgeList, it just loops add(), consistent with "ordinary, non-bulk speed" being the explicitly accepted tradeoff for this rare path.
• Nice touch resetting lastSegmentPromoted = false at the top of both getOrCreateOutSegmentDeferred/getOrCreateInSegmentDeferred so a stale true from a previous call can never leak into an unrelated group.

Potential bugs

• Pre-existing, out of scope for this PR, but worth flagging since it's directly adjacent: getOrCreateOutEdgeChunkExact / getOrCreateInEdgeChunkExact (GraphBatch.java ~L1830/~L1857) still do the raw unguarded cast (EdgeSegment) database.lookupByRID(headChunk, true) that this PR just fixed on getOrCreateOutEdgeChunk/getOrCreateInEdgeChunk and the two getOrCreate*SegmentDeferred methods. They appear to be unused/dead code today (no call sites anywhere in engine/), so there's no live risk, but if anyone wires them up later they'll reintroduce the same ClassCastException this PR is fixing elsewhere. Might be worth a quick note/TODO, or just deleting them if they're truly dead.
• The new StripedEdgeList.add() calls can throw ConcurrentModificationException (a NeedRetryException) under a genuine concurrent writer (e.g. loadChunkForWrite hitting an unresolved chain). flush() doesn't retry on NeedRetryException the way createVerticesWithRetry does for vertex creation - it rolls back and rethrows. That mirrors GraphBatch's existing behavior on its other write paths (bulk segment writes aren't retried either), so it's not a regression, but it's worth being aware of if this promoted-vertex fallback is ever expected to tolerate a concurrent external writer during a bulk load rather than assuming exclusive access.

Performance

• The tradeoff is explicitly documented (class javadoc + PR description) and seems reasonable: falling back to one-edge-at-a-time StripedEdgeList.add() only fires for the rare "vertex was already promoted before/during this load" case, not the hot bulk path. Good that the promoted branch is checked first, before the (now unreachable in that case) segment-overflow slow path.
• One consequence worth confirming is intentional: since a promoted vertex's group is never cached into outChunkRIDCache/inChunkRIDCache (the deferred lookup methods return null without caching), every flush() that touches the same hub re-does the vertex load + head-chunk read + instanceof check from scratch rather than caching "this vertex is promoted, always use StripedEdgeList". For a hub that receives edges across many separate flush() batches this is a repeated (cheap, but non-zero) lookup each time - probably fine given how rare resuming over a promoted hub should be, just flagging in case it's not what was intended.

Security

Nothing notable - purely internal storage-engine logic, no new external input surface, no new dependencies.

Test coverage

• The two new tests (sequentialFlushResumesOverPromotedHub, parallelFlushResumesOverPromotedHub) directly reproduce both the old IllegalStateException and the previously-undocumented ClassCastException, and correctly assert the list stays StripeDirectory afterward (i.e. no silent demotion/corruption). Good use of assertThat(...) per project convention, and the config save/restore in @BeforeEach/@AfterEach is clean.
• Both tests only ever buffer a single edge per hub per direction, so addGroupThroughStripedEdgeList's count loop only ever runs with count == 1. Given a big part of the fix's value is in per-group behavior (groupSize in the sequential/parallel connectors), a variant that buffers several edges into the same promoted hub within one flush (forcing groupSize > 1 through the new code path) would give more confidence in the multi-edge case.
• The description's "Thread-safety pitfall caught during development" section is a great catch, but as written it's only verified by memory/manual testing ("during development, not present in the final diff") rather than locked in by an automated test. A parallel-flush test with two (or more) promoted hubs whose edges land in different bucket partitions/async slots, asserting exact per-hub edge counts afterward, would directly guard against that specific regression reappearing later.

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.11%. Comparing base (ea4f6af) to head (41a4ffb).
⚠️ Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
...e/src/main/java/com/arcadedb/graph/GraphBatch.java	87.50%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5948      +/-   ##
==========================================
+ Coverage   68.03%   68.11%   +0.07%     
==========================================
  Files        1797     1797              
  Lines      152855   153178     +323     
  Branches    32397    32442      +45     
==========================================
+ Hits       103999   104338     +339     
+ Misses      35466    35461       -5     
+ Partials    13390    13379      -11     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.