fix(#5664): bound GraphBatch head-chunk RID caches and drain deferred incoming edges early

[robfrank]

Closes #5664

Review cycle 1

The claude bot review found one 🔴 critical, correctness-blocking issue and one 🟡 performance
concern. Status below.

🔴 Critical (fixed): known-new vertex + LRU eviction could silently orphan a segment and lose edges

Bounding outChunkRIDCache/inChunkRIDCache with an LRU broke an invariant the "known-new vertex"
fast path (knownNewVertexKeys, populated only by GraphBatch.createVertices()) relied on: that a
cache miss for such a vertex could only mean "no segment exists yet for it". Before this PR the two
RID caches were plain, never-evicted ConcurrentHashMaps with the exact same lifecycle as
knownNewVertexKeys, so that was true. Once the RID cache is LRU-bounded, a known-new vertex's
entry can be evicted mid-batch - trivially reached once more than chunkCacheCapacity other
distinct vertices are touched between two edges of the same vertex, the exact large-stream scenario
this issue targets. On the next touch, getOrCreateOutSegmentDeferred/getOrCreateInSegmentDeferred
(and the parallel-path twins connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal) took the
"known-new" branch again, wrongly assumed no segment existed, and created a second, unlinked
segment (no setPrevious()) - overwriting the pointer to the first segment in both the RID cache and
deferredOutHead/deferredInHead. The first segment's already-committed edges become permanently
unreachable once batchUpdateVertexHeadChunks() writes the final head pointer at close(). Silent
data loss, no exception.

Fix: all four call sites now consult deferredOutHead/deferredInHead - unbounded for the whole
batch lifetime and always accurate for a vertex's current head - as a fallback before assuming "brand
new" on a known-new-vertex cache miss. If a deferred head is found, the existing segment is reused
(and correctly overflow-linked via the normal setPrevious() path on the next append) instead of a
second unlinked segment being created.

New regression test (GraphBatchBoundedStateTest.knownNewVertexSurvivesCacheEvictionBetweenItsOwnEdgesSequential/
...Parallel): creates vertices via importer.createVertices(...) (not database.newVertex(), which
never populates knownNewVertexKeys - why the prior test suite missed this), uses a small
withChunkCacheCapacity(20), gives a vertex a first OUT and first IN edge, then interleaves 200 edges
touching 400 other distinct vertices (far more than the cache capacity) to force eviction, then gives
the same vertex a second OUT and second IN edge. Verified this fails pre-fix: OUT/IN edge count
came back 1 instead of 2, plus TestHelper's database-integrity check independently flagged the
orphaned edges ("edge ... was not connected from the incoming/outgoing vertex ..."). Passes post-fix
for both the sequential and parallel flush paths.

🟡 Performance (deferred, not fixed): Collections.synchronizedMap(LRUCache) serializes the parallel-flush hot path

Confirmed as described: previously outChunkRIDCache/inChunkRIDCache were ConcurrentHashMap,
giving lock-free/finely-striped concurrent access from every parallel async slot in
connectOutgoingEdgesParallel/connectIncomingEdgesParallel. Collections.synchronizedMap(new LRUCache<>(...)) puts every get()/put() behind one global intrinsic lock, and because LRUCache
is access-order (LinkedHashMap(..., true)), even get() mutates the internal linked list - there is
no cheap read path left. This could cut into (or negate) parallelFlush=true throughput on a
multi-core box.

Deferred, not implemented in this cycle, for two reasons: (1) it is explicitly not
correctness-blocking per the reviewer's own framing, and this cycle's budget went to verifying the
critical fix is solid (reproduced pre-fix, fixed, regression-tested, full com.arcadedb.graph.*Test
sweep green - 210/210, 0 failures/errors); (2) the reviewer's own suggested mitigation (a sharded LRU:
N Collections.synchronizedMap(LRUCache) shards keyed by vertexKey % N, each sized
chunkCacheCapacity / N) is real surgery to a hot path used by every GraphBatch caller - it needs a
before/after parallelFlush=true throughput benchmark to justify the added complexity, which this
cycle didn't produce. Flagging here as an accepted, tracked follow-up rather than silently dropping
it: worth doing if/when a benchmark shows a real regression on the exact large-parallelFlush
workloads this issue targets.

---

What does this PR do?

Bounds two kinds of state in GraphBatch that previously grew with the lifetime of the batch
instead of batchSize:

1. outChunkRIDCache / inChunkRIDCache (head-chunk RID lookup accelerators) were plain
   ConcurrentHashMaps, never cleared. Every distinct vertex an edge touched added ~80-90 bytes
   across the two maps that stayed until close() - 16-18 GB at 100M distinct vertices.
2. The deferred incoming-edge buffer (six parallel primitive arrays, doubled on overflow) was only
   drained by connectDeferredIncomingEdges(), called exclusively from close(). 100M edges in one
   batch held 3.6 GB steady, up to ~2.5x that during a doubling copy.

batchSize only sizes the outgoing edge buffer, reset every flush() - the knob a user reaches
for touches neither leak, which is exactly the "big batches vs. many streams" false dilemma
reported against the 663M-vertex / 14B-edge gRPC GraphBatchLoad stream in discussion #5597 (the
user had to recycle the RPC every 4M records to keep the server alive).

Fix

Both call sites for the two RID caches already fall back to reading the vertex's head chunk from
disk on a miss (getOrCreate{Out,In}{Segment,EdgeChunk}{Deferred,Exact}), so bounding them is a
safe drop-in:

1. Bounded head-chunk RID caches. outChunkRIDCache/inChunkRIDCache are now
   Collections.synchronizedMap(new LRUCache<>(chunkCacheCapacity)) - the same bounded-LRU pattern
   already used by CypherStatementCache. Configurable via GraphBatch.Builder.withChunkCacheCapacity(int),
   default 1,000,000 entries per cache (~100-150 MB total). LRUCache is not thread-safe on its
   own; wrapping in Collections.synchronizedMap is required because
   getOrCreateOutEdgeChunk/getOrCreateInEdgeChunk and the parallel-flush range handlers hit
   these maps from multiple async slots.

   • Review cycle 1: this bound broke the known-new-vertex fast path's invariant - see above.
     Fixed by falling back to the unbounded deferredOutHead/deferredInHead on a known-new-vertex
     cache miss.

2. Periodic draining of the deferred incoming-edge buffer. flush() now checks, after
   accumulating a flush's edges into the deferred IN buffer, whether inEdgeCount has crossed a
   configurable cap (GraphBatch.Builder.withMaxDeferredIncomingEdges(int), default 5,000,000) and
   if so calls connectDeferredIncomingEdges() early instead of waiting for close(). This
   amortizes the incoming-edge connection pass over the load rather than landing as one
   multi-minute pass at the end (issue GraphBatch: Stuck without error after index compacting #5470's 86-second close-time wait is the shape of cost this
   removes). Setting the cap to 0 opts back into the pre-GraphBatch retains per-vertex state for the whole batch lifetime, so a long-lived bulk stream grows without bound #5664 behavior of deferring everything to
   close().

   • connectDeferredIncomingEdges() already null'd out the six deferred-buffer arrays when it
     finished (an existing optimization to let them be GC'd at close()); calling it mid-batch
     meant a later accumulateIncomingEdges() would NPE on the null arrays. Fixed by lazily
     re-allocating them (at the same initial batchSize capacity the constructor uses) the first
     time accumulateIncomingEdges() sees them null.

deferredOutHead/deferredInHead/knownNewVertexKeys were intentionally left unbounded for the
batch's lifetime: they already use zero-boxed structures (LongObjectHashMap/LongHashSet, ~5-7x
lighter than the boxed alternative), are already cleared on every batchUpdateVertexHeadChunks()
pass, and their size is bounded by the number of distinct vertices that got a new or overflowed
segment. As of review cycle 1, deferredOutHead/deferredInHead are also the correctness-critical
fallback the known-new-vertex fast path depends on, so they cannot be bounded the same way the RID
caches are without reopening this exact bug - any future work to bound them needs a different
mechanism (e.g. draining them via batchUpdateVertexHeadChunks() early, which persists vertex
records and clears knownNewVertexKeys too - a materially bigger change, out of scope here). See
"Impact / follow-up" below.

Changes

• engine/src/main/java/com/arcadedb/graph/GraphBatch.java
  • outChunkRIDCache/inChunkRIDCache: ConcurrentHashMap -> bounded LRUCache wrapped in
    Collections.synchronizedMap, capacity configurable via the builder.
  • New builder methods: withChunkCacheCapacity(int), withMaxDeferredIncomingEdges(int).
  • New constants: DEFAULT_CHUNK_CACHE_CAPACITY (1,000,000), DEFAULT_MAX_DEFERRED_INCOMING_EDGES
    (5,000,000).
  • flush(): drains the deferred incoming-edge buffer early once it crosses the configured cap.
  • accumulateIncomingEdges(): lazily re-allocates the deferred buffer arrays if an earlier
    in-flush drain freed them.
  • New package-private test accessors getOutChunkRIDCacheSize() / getInChunkRIDCacheSize().
  • Class-level javadoc updated to document both bounds (issue's ask to "document them").
  • Review cycle 1: getOrCreateOutSegmentDeferred, getOrCreateInSegmentDeferred,
    connectOutEdgesRangeLocal, connectIncomingEdgesRangeLocal - on a known-new-vertex RID-cache
    miss, fall back to deferredOutHead/deferredInHead before assuming "no segment yet".
• engine/src/test/java/com/arcadedb/graph/GraphBatchBoundedStateTest.java: regression coverage per
  the issue's own "Verification" section - runs a GraphBatch over many more distinct vertices than
  the configured cap and asserts the retained cache/buffer size stays bounded, for both the
  sequential and parallel flush paths, plus correctness (every edge still traversable in both
  directions despite evictions/early draining), plus builder validation and the
  0-disables-early-drain opt-out.
  • Review cycle 1: added knownNewVertexSurvivesCacheEvictionBetweenItsOwnEdgesSequential/
    ...Parallel, exercising the createVertices() + eviction-between-two-edges scenario the
    critical finding describes.

Related issues

Closes #5664. Referenced in discussion #5597.

Impact / follow-up for monitoring

• Both new caps are conservative defaults chosen to bound memory to low-hundreds-of-MB even on an
  extreme (100M+ vertex/edge) stream, while being large enough that typical/bounded imports never
  reach them - no behavior or performance change for the common case.
• Operators running very large streams should watch for the new INFO log lines from
  connectDeferredIncomingEdges() firing more often (once per drain instead of once at close()) -
  this is expected and is the amortization the fix is for, not a symptom of trouble.
• deferredOutHead/deferredInHead/knownNewVertexKeys remain unbounded for the batch's lifetime
  (bounded only by distinct-vertex count, not edge count) - and, as of review cycle 1, are also
  correctness-critical rather than purely an optimization. If a future report shows this scaling to
  be a problem in practice, extending the periodic-drain mechanism to call
  batchUpdateVertexHeadChunks() early is the natural next step, flagged here for future reference
  rather than acted on now.
• Deferred follow-up (review cycle 1, 🟡 not correctness-blocking): Collections.synchronizedMap(LRUCache)
  serializes outChunkRIDCache/inChunkRIDCache access across all parallel-flush async slots where a
  ConcurrentHashMap used to give lock-free access. Worth benchmarking parallelFlush=true
  throughput before/after on a multi-core box; if it regresses meaningfully, shard the LRU (N
  Collections.synchronizedMap(LRUCache) shards keyed by vertexKey % N, sized
  chunkCacheCapacity / N each) to restore concurrency while keeping the bound.

Test plan

• New regression test GraphBatchBoundedStateTest (7/7 passed), per the issue's own
  "Verification" ask plus review cycle 1's critical-bug coverage: runs a GraphBatch over far
  more distinct vertices than the configured cache capacity and asserts the retained OUT/IN
  cache sizes and the deferred incoming-edge count stay bounded, for both the sequential and
  parallel flush paths; asserts correctness is unaffected (every edge still traversable in both
  directions after evictions/early draining, including the known-new-vertex eviction-between-
  two-edges scenario); asserts the 0-disables-early-drain opt-out and builder argument
  validation.
• Review cycle 1: new tests verified to fail pre-fix (reproduced the exact silent-loss
  symptom: OUT/IN edge count 1 instead of 2, plus an independent database-integrity check
  flagging the orphaned edges) and pass post-fix.
• Existing GraphBatchTest suite - no regression.
• mvn -pl engine -am -DskipITs=true -Dtest='com.arcadedb.graph.GraphBatch*Test,com.arcadedb.graph.Issue566*Test' test -
  all passed.
• Review cycle 1: mvn -pl engine -am -DskipITs=true -Dtest='com.arcadedb.graph.*Test' -DexcludedGroups='slow,benchmark' test -
  210/210 passed, 0 failures/errors, across the full graph test package.
• mvn -pl engine -am compile - success.
• mvn -pl engine,grpcw,server,network,integration -am compile - success (confirms no
  downstream break for the gRPC GraphBatchLoad stream, HTTP batch handler, RemoteGraphBatch,
  and the Neo4j importer; all new builder methods are additive).
• mvn clean package (full build) - left to CI / reviewer per repo convention for this size of
  change.

Review cycle 1

claude[bot] posted two reviews on this PR, both against the same initial commit (landed 22:46 and 22:51 UTC), flagging a 🔴 critical, correctness-blocking issue:

Bounding outChunkRIDCache/inChunkRIDCache with an LRU while leaving knownNewVertexKeys unbounded broke an invariant the "known-new vertex" fast path relied on - a cache miss for such a vertex used to mean "definitely no segment yet" (the caches were never evicted before this PR). Once the LRU evicts that vertex's entry mid-batch (easy at the 100M+ vertex scale this PR targets), the fast path wrongly re-creates an unlinked segment, silently orphaning the first segment's already-committed edges.

Fixed (commit 1dedd8161): consult deferredOutHead/deferredInHead (unbounded for the whole batch, always authoritative for a touched vertex's current head) before assuming "no segment exists yet" in the knownNewVertexKeys branch. New regression tests (knownNewVertexSurvivesCacheEvictionBetweenItsOwnEdgesSequential/...Parallel) reproduce the exact loss via createVertices() + a small chunkCacheCapacity forcing eviction between a vertex's own two edges; verified RED pre-fix (edge count short, database-integrity warnings about disconnected edges), GREEN post-fix.

Broadened the fix (commit 529fcd1d6, self-caught re-reading the second review's wording): the same staleness applies to a pre-existing vertex NOT created via createVertices() - it's never in knownNewVertexKeys, so it falls to an on-disk read that's equally stale mid-batch (the on-disk head is only persisted at close()). Restructured all four call sites (getOrCreateOutSegmentDeferred/getOrCreateInSegmentDeferred, and the inline equivalents in connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal) to check deferredOutHead/deferredInHead unconditionally, before branching on knownNewVertexKeys at all. Two more regression tests (preExistingVertexSurvivesCacheEvictionBetweenItsOwnEdgesSequential/...Parallel) reproduce this second shape with plain database.newVertex()-created vertices; verified RED against the 1dedd8161 partial fix, GREEN against 529fcd1d6.

Full com.arcadedb.graph.*Test sweep (excluding slow/benchmark): 204/204 passed after the broadened fix.

Deferred, not fixed (🟡 non-blocking): Collections.synchronizedMap(LRUCache) serializes outChunkRIDCache/inChunkRIDCache access across all parallel-flush async slots, where a ConcurrentHashMap previously gave lock-free access. The reviewer suggested benchmarking parallelFlush=true throughput and, if it regresses meaningfully, sharding the LRU (N Collections.synchronizedMap(LRUCache) shards keyed by vertexKey % N) to restore concurrency while keeping the bound. Not implemented in this cycle - correctness took priority over this optimization, and it's independent of the data-loss fix above. Left for a follow-up PR/benchmark if parallelFlush=true throughput on a multi-core box shows a real regression.

Review cycle 1, follow-up

A third claude[bot] review comment landed at 23:17 UTC re-raising point 1, but timestamp-checking showed it only reflects commit 3 (1dedd8161) - it doesn't account for commit 4 (529fcd1d6, pushed 23:05:57 UTC, 11 minutes earlier), which already drops the knownNewVertexKeys gate and covers exactly the "regular vertex, sequential flush, eviction between two edges" scenario it asks for. Replied on the PR with the specifics. Fixed the one genuinely new item it raised: the stale deferredOutHead/deferredInHead class-level comment claiming the parallel paths "do NOT touch these fields directly" - no longer true since the point-1 fix added direct reads there. Updated in 0e7f4b13c to document why those reads are safe.

Review cycle 3

claude[bot] found a correctness bug specific to this PR's early-drain feature (issue #5664), separate from and more serious than the minor cache-repopulation nit fixed alongside it.

Bug: a partially-failed early drain gets blindly reprocessed by close(), duplicating already-committed incoming edges

flush() now calls connectDeferredIncomingEdges() directly once inEdgeCount crosses maxDeferredIncomingEdges. That method commits the deferred buffer in slices - the sequential path every commitEvery edges, the parallel path one commit per destination-bucket async.transaction task - but only reset inEdgeCount and nulled the buffer arrays after the whole method returned successfully. If a slice failed partway through (a NeedRetryException that exhausts retries, a unique-index violation, disk-full), earlier slices were already durably committed, but the buffer still held the full original set. flush()'s catch block resets only the outgoing buffer, never the incoming one. The class's own documented try (GraphBatch batch = ...) { ... } usage then triggers close() on unwind, which unconditionally re-runs connectDeferredIncomingEdges() over the same stale buffer - reprocessing (and duplicating) the slices that already committed in the first, failed attempt. This is a real regression introduced by this PR's early-drain feature: before it, connectDeferredIncomingEdges() was only ever called once, from close(), so there was no "retry over a partially-succeeded buffer" scenario.

Fix

• Sequential (connectIncomingEdgesSequential): a new inEdgesResumeSortIndex field records how far into the sort index a fully-committed prefix reaches, advanced only right after each successful database.commit(). A retry starts from this cursor instead of index 0, skipping already-committed groups. Reset to 0 alongside the buffer arrays only on the drain's full success.
• Parallel (connectIncomingEdgesParallel): a new completedIncomingBuckets set tracks which destination buckets already committed. Each bucket is scheduled as its own independent async.transaction; a bucket already in the set is skipped on a retry. Marked complete from the transaction's OkCallback (fired strictly after database.commit() succeeds), never from inside the bucket-processing lambda, which runs before the surrounding commit and could still fail there.

A second, deeper bug found while building the reproduction test

Both deferredInHead/inChunkRIDCache get mutated as a side effect of processing a group - before the transaction containing that group's writes commits (persistNewSegment, and the parallel path's inline segment creation). When a slice's transaction rolls back, those maps were left pointing at segment records that were never made durable. On the retry, resuming past the rollback point correctly skipped the committed groups, but the very next group's fallback lookup (getOrCreateInSegmentDeferred / the parallel path's inline equivalent) hit the phantom RID and crashed with RecordNotFoundException - a different, more severe symptom of the same underlying "stale state survives into a retry" defect class.

Fixed with two complementary mechanisms:

• Sequential: an undo log snapshots each touched vertex's pre-slice deferredInHead value (once per vertex per slice) and restores it - or removes the entry entirely if it was absent before - if the slice fails. LongObjectHashMap only supported put/get, so remove(long) was added (tombstone-based deletion, a new reserved sentinel Long.MIN_VALUE + 2, all existing probe/resize/forEach/keysArray logic updated to skip tombstones). 17 unit tests added to LongObjectHashMapTest covering removal, probe-chain integrity past a tombstone, slot revival, and tombstone reclamation on resize.
• Parallel: each bucket task now writes its new-segment head pointers into a local (non-shared, non-thread-safe-but-safe-because-single-threaded-per-bucket) map instead of directly into the class-level caches; that local map is merged into deferredInHead/inChunkRIDCache only in the single-threaded pass after waitCompletion(), and only for buckets that are in completedIncomingBuckets. Also found and fixed the same eager-write-before-commit pattern in connectIncomingEdgesRangeLocal's fallback branch for a not-yet-touched, not-known-new vertex (previously called getOrCreateInEdgeChunk(), which persists and caches inline) - restructured to defer to the same local-map-then-merge-on-success path used by the knownNewVertexKeys branch.

Test

New TEST_BEFORE_INCOMING_EDGE_COMMIT_HOOK (mirrors the existing TEST_BEFORE_VERTEX_COMMIT_HOOK pattern) fires right before each deferred incoming-edge commit - both the sequential path's commitEvery commits and the parallel path's per-bucket commit - so a test can force a failure at a precise point. New Issue5950IncomingEdgeDrainReentrancyTest (engine/src/test/java/com/arcadedb/graph/):

• sequentialRetryDoesNotDuplicateAlreadyCommittedGroups: 10 destination vertices, commitEvery=2, hook throws on the 3rd commit (after 4 groups already durable). Verified RED pre-fix: destination vertex Bump mongo-java-driver from 3.12.4 to 3.12.10 #4 came back with 2 incoming edges instead of 1 (exact duplicate reproduction). GREEN post-fix.
• parallelRetryDoesNotDuplicateAlreadyCommittedBuckets: 2 destination buckets, database.async().setParallelLevel(1) for deterministic bucket-task ordering, hook throws on the 2nd bucket's commit. Verified RED pre-fix two ways as the fix was built up: first a duplicate-edge assertion failure (same shape as the sequential case), then - after adding the resume-cursor/completed-bucket-set fix alone - a RecordNotFoundException from the second, deeper bug above. GREEN once both fixes were in place.

Both tests assert every destination vertex, across both the already-committed-before-failure group and the genuinely-pending group, ends up with exactly one incoming edge after close().

Verification

• mvn -pl engine -am test -Dtest='com.arcadedb.graph.GraphBatch*Test,com.arcadedb.graph.Issue5664*Test,com.arcadedb.graph.Issue5665*Test,com.arcadedb.graph.Issue5666*Test,com.arcadedb.graph.Issue5950*Test,com.arcadedb.utility.LongObjectHashMapTest' - 52/52 passed.
• mvn -pl engine -am test -Dtest='com.arcadedb.graph.*Test' -DexcludedGroups='slow,benchmark' - 206/206 passed, 0 failures/errors.
• mvn -pl engine -am compile - clean.

Also included: the minor cache-repopulation nit from the previous review cycle

connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal's deferredOutHead/deferredInHead fallback branch now repopulates outChunkRIDCache/inChunkRIDCache on the fallback hit, matching the sequential path's existing behavior - so a vertex touched again later in the same parallel round doesn't keep paying for the fallback lookup. Purely a minor consistency fix, no correctness impact (deferredOutHead/deferredInHead remain authoritative either way).

Related finding on the outgoing side - CORRECTED in review cycle 3, fixed in cycle 4

This section previously argued the same eager-write-before-commit pattern on the outgoing side
(getOrCreateOutEdgeChunk) was safe to defer, on the grounds that outgoing edges have no
retry-across-attempts mechanism. That reasoning was wrong and is corrected in review cycle 4 below:
it only considered retries across flush()/close() calls and missed that async.transaction(..., 3, ...)
retries the same bucket lambda on a ConcurrentModificationException within a single flush(). The gap
is fixed in cycle 4.

Commits: 6d296b284.

Review cycle 4

Three findings, all verified against the code and all fixed. Commit d7cf46879.

🔴 The outgoing-side gap is reachable within a single flush() - my cycle-3 deferral was wrong

The reviewer is correct and my earlier reasoning was incomplete. I had only considered retries across
flush()/close() calls. But connectOutgoingEdgesParallel schedules each bucket via
async.transaction(..., 3, ...), and DatabaseAsyncTransaction.execute() retries the same lambda up to
three times on a ConcurrentModificationException, rolling back and re-invoking connectOutEdgesRangeLocal
over the same range. Since page-level CME is the designed, expected outcome of two edges appending into the
same edge-list page (see engine/CLAUDE.md), this is a normal-conditions path, not an exotic one.

getOrCreateOutEdgeChunk() persisted a new segment and wrote its RID straight into the class-level
outChunkRIDCache (and flipped the vertex head pointer) before the bucket's transaction committed. That write
survives the rollback, so attempt 2 reads the cache first and dereferences a RID that was never made durable.

There was a second sub-case too: parallelDeferredOutHead/parallelOutChunkCache were merged into
deferredOutHead/outChunkRIDCache unconditionally after waitCompletion(), including entries from
buckets that exhausted all 3 retries - poisoning deferredOutHead, which is the authoritative head-chunk
fallback the cycle-1/2 fix depends on, for the rest of the batch.

Fix: the OUT path now gets exactly the treatment the IN path received in cycle 3 - per-bucket local
head-pointer maps, merged into the shared state only for buckets whose commit succeeded (tracked from the
transaction's post-commit OkCallback), and new-segment creation deferred to the isNew branch rather than
the eager persist-and-cache helper. Caching an existing, already-durable on-disk head chunk stays inline,
since that RID is committed regardless of this transaction's fate. Both directions additionally clear their
per-bucket local maps at the start of each attempt so a CME retry cannot inherit a rolled-back attempt's
entries.

Verified RED pre-fix: new Issue5950OutgoingEdgeRetryCacheTest failed with
RecordNotFoundException: Record #2:0 not found - the retry dereferencing the rolled-back segment RID,
exactly as predicted. GREEN post-fix. Driven by a new TEST_BEFORE_OUTGOING_EDGE_COMMIT_HOOK throwing a
ConcurrentModificationException on the first bucket attempt (forcing a real CME on a chosen bucket is not
deterministic). The test deliberately creates its vertices with plain database.newVertex(), not
GraphBatch.createVertices(), since only then are they absent from knownNewVertexKeys and routed into the
fallback branch this fix changes.

🟡 LongObjectHashMap.remove() tombstones were uncounted against the resize threshold - infinite loop

Confirmed as a real hang, not a theoretical one. resize() triggered off live size only, so tombstones
accumulated uncounted. Since every probe here terminates only on EMPTY_KEY, a workload with many more
removes than size-growing puts can fill the table with tombstones + live entries and leave zero empty
slots, at which point get(), put(), remove() and containsKey() all spin forever on a miss.

Fix: tombstones are counted, and the threshold check is size + tombstones >= threshold -
exactly the non-empty slot count - which guarantees an empty slot always remains. The rehash now sizes from
the live entry count rather than always doubling, so a churn-heavy workload reclaims tombstoned slots in
place instead of growing capacity without bound.

Verified RED pre-fix: the test class hung outright (over 400s for a class that normally finishes in
0.08s), running 0 tests to completion. Worth noting: my first attempt at this test used a plain
@Timeout, which does not work here - JUnit's default same-thread timeout can only report a breach
after the method returns, which an infinite loop never does, so the build would hang rather than fail. The
committed tests use threadMode = SEPARATE_THREAD so a regression actually fails the build. 19/19 GREEN
post-fix in 0.077s.

🟡 The incoming-drain resume state assumed "no intervening newEdge()", unenforced

Confirmed: my own javadoc stated the assumption and nothing enforced it. A caller that catches the exception
from a failed early drain and keeps calling newEdge() - a pattern this class supports elsewhere, e.g.
createVerticesWithRetry for #4724's Raft hiccups - appends onto the still-undrained buffer. Since
partitionIncomingByDestBucket is a counting sort, that changes bucketCounts/bucketOffsets and therefore
what a given raw index, and a given bucket's [from,to) range, refers to. The recorded
inEdgesResumeSortIndex and completedIncomingBuckets then apply to a differently-shaped index.

I chose the reviewer's option (b), robustness, over (a), fail-fast: for a bulk loader, turning a survivable
transient failure into a dead batch is a worse outcome than making the resume mechanism correct.

Fix: each drain pass pins the row count it covers (inEdgesDrainPrefix) and a retry reuses that exact
pin, so the sort index has an identical shape across attempts and both resume mechanisms stay sound. After a
pinned prefix completes, it is dropped from the buffer and any rows appended past the pin are drained as their
own pass (connectDeferredIncomingEdges() now loops). On the normal path prefix == inEdgeCount, so the
compaction is a counter reset with no copying - zero overhead on the hot path.

Verified RED pre-fix with two new tests, each reproducing a distinct symptom, which also required fixing
my first test design (it accidentally appended rows that sorted after the prefix, so it could not fail):

• sequential lost #4:0 - the wave-2 row in a bucket sorting below wave 1's, which the raw cursor skipped past;
• parallel lost #7:3 - the wave-2 row added to a bucket already marked complete, which the bucket-skip dropped.

Both GREEN post-fix.

Nits

• Third public static volatile test hook. Kept, and a third added for the OUT side. It matches the
  established TEST_BEFORE_VERTEX_COMMIT_HOOK convention, each is cleared in @AfterEach, and proving a
  concurrency fix without fault injection is not feasible - forcing a real CME on a specific bucket is
  nondeterministic. On the parallel-execution concern: forkCount=1 and no
  junit.jupiter.execution.parallel.enabled anywhere, so tests run sequentially in one JVM; if that ever
  changes, the pre-existing vertex hook has the identical exposure and they should be addressed together.
• setParallelLevel(1) not restored. Verified it cannot leak: parallelLevel is per
  DatabaseAsyncExecutor, and TestHelper builds a fresh database (hence a fresh executor) in its
  constructor for every test method and drops it in @AfterEach. Restoring would only churn the thread pool
  of a database about to be dropped. Documented in-place rather than adding a no-op restore that would imply
  otherwise.

Verification

• mvn -pl engine -am test -Dtest='com.arcadedb.graph.*Test,com.arcadedb.utility.LongObjectHashMapTest' -DexcludedGroups='slow,benchmark' - 232/232 passed, 0 failures/errors.
• mvn -pl engine -am compile - clean.
• Each of the three fixes independently verified RED before and GREEN after, by reverting only that fix.

Note on branch history

This branch was rebased onto current main (f2754aee3) partway through the review cycles and force-pushed once, to pick up sibling PRs #5948 (#5667 super-node striping) and #5949 (#5665 async WAL policy) after they merged - all three touched GraphBatch.java and the branch had developed a real conflict. The #5667 promotion routing was merged by hand in connectIncomingEdgesSequential and connectIncomingEdgesRangeLocal, combined with this PR's resume-cursor/local-map machinery; Issue5667GraphBatchSuperNodeResumeTest passes alongside this PR's own tests. History is linear and all pre-rebase commits remain reachable (6d296b284 and earlier) if any earlier review reference needs resolving. Flagged here for transparency since the rebase rewrote SHAs cited in earlier review replies - those old SHAs still resolve on GitHub.

Performance verification (closes the cycle-1 and cycle-4 perf questions)

Benchmarked on a 12-core machine (11 async worker threads, so the parallel-flush lock contention under
question is genuinely exercised, not understated). 100k distinct vertices, 1M edges, bidirectional,
median of 5 alternating iterations per configuration. Harness committed as
engine/src/test/java/performance/GraphBatchDrainPerfBenchmark.java - it uses only builder methods
that exist both before and after this PR, so the identical source runs against the pre-PR tree for a
true before/after.

Configuration	pre-PR baseline (f2754aee3)	this PR	delta
parallelFlush=true (chunk-RID cache contention, cycle 1)	1754 ms · 570k edges/s	1734 ms · 577k edges/s	~1% faster
parallelFlush=false (incoming-head undo log, cycle 4)	1612 ms · 620k edges/s	1586 ms · 631k edges/s	~1.5% faster

No measurable regression from either concern. Both deltas are within run-to-run noise (±3%; a repeat
sample put the sequential figure at 1613 ms), and both moved slightly in the faster direction, so
neither the Collections.synchronizedMap(LRUCache) serialization nor the boxed-Long undo log costs
measurable throughput at this scale and parallelism.

What the memory bound costs once it actually binds

GraphBatchEvictionCostBenchmark varies only withChunkCacheCapacity on this tree (a before/after
framing does not apply - unbounded is the pre-PR behavior this issue removes):

chunkCacheCapacity	median	vs no-eviction
200,000 (> vertex count, never evicts = pre-#5664 cache behavior)	1732 ms	baseline
50,000	1776 ms	+2.5%
10,000	1778 ms	+2.7%
1,000 (100x smaller than default)	1754 ms	+1.3%

Effectively free, and the ordering is non-monotonic, which is the signature of noise rather than a real
cost. The reason is worth stating explicitly, because it is a consequence of the cycle-1/2 fix and it
bounds how far this result generalizes: a chunk-cache miss now falls back to
deferredOutHead/deferredInHead before reading from disk, and those maps are still unbounded for the
batch's lifetime. Every vertex in this benchmark is created by createVertices() within the batch, so it
is present in the deferred map and an evicted cache entry costs only a second in-memory lookup - never a
disk read.

Two honest limits follow from that:

1. A workload whose vertices were not touched earlier in the same batch (e.g. a bulk load resuming
   over a pre-existing graph) would pay a real disk read on an evicted miss. That case is not measured
   here.
2. It reinforces the already-documented follow-up that deferredOutHead/deferredInHead/
   knownNewVertexKeys remain unbounded by distinct-vertex count. At the 663M-vertex scale from
   discussion Ingesting Massive DB with 600 Million Vertices and 14 Billion Edges #5597 those maps, not the now-bounded chunk caches, are the remaining memory ceiling.

Full-suite verification

• Full engine module unit suite on this HEAD: 11,252 tests, 0 failures, 0 errors, 23 skipped
  (mvn -pl engine -am test -DexcludedGroups=slow,benchmark, read from Maven's own Results: summary).
  (Addresses the "verification perimeter is narrower than the blast radius" concern - the earlier cycles
  had only run the graph package.)
• com.arcadedb.graph.*Test + LongObjectHashMapTest: 232/232.
• Each of the fixes across cycles 1-4 was independently confirmed RED-before / GREEN-after by reverting
  only that fix.

Review cycle 5

Two non-blocking items from the latest reviews, both fixed in c31233a02:

• get()/containsKey() did not short-circuit on TOMBSTONE_KEY the way put()/remove() already did, so
  containsKey(TOMBSTONE_KEY) would probe and "match" the first tombstoned slot it walked over, wrongly
  answering true. Unreachable through this map's own API (put() rejects the key, so it can never be a real
  mapping), but guarded anyway since this is a general-purpose utility.

  Worth calling out how the regression test was built, because the obvious version of it cannot fail: a
  tombstone placed anywhere in the table isn't enough, since the probe stops at the first EMPTY slot and
  returns false for the wrong reason - that test passes with the fix reverted. The committed test therefore
  replicates the map's MurmurHash3 finalizer to plant the tombstone at exactly the slot the sentinel's own probe
  starts on. Verified RED (expected false but was true) against the reverted guard, GREEN with it.

• The class javadoc now states that the memory bound is partial. It previously listed the two bounded caches
  under "optimizations" with no statement of what remains unbounded, which reads as though GraphBatch retains per-vertex state for the whole batch lifetime, so a long-lived bulk stream grows without bound #5664 is fully
  solved. deferredOutHead/deferredInHead/knownNewVertexKeys still grow with distinct-vertex count, and
  since this PR they are load-bearing rather than an optimization - they are the authoritative fallback that
  keeps a cache eviction from orphaning an earlier segment. Stated plainly so the remaining ceiling isn't
  rediscovered later.

Note on the red integration-tests check

It is a pre-existing main breakage, not this PR. The same job fails on this branch's own merge-base
(f2754aee3) and on a049d9325, b380e9dca, 81362c771. The job log ends partway through
com.arcadedb.server.HTTPDocumentIT with every class above it green and no assertion failure or BUILD FAILURE
line - the shape of a timeout/kill rather than a test failure. Worth its own issue; nothing to fix here.

[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 2 medium

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

Results:
2 new issues

Category	Results
BestPractice	2 medium

View in Codacy

🟢 Metrics 37 complexity

Metric	Results
Complexity	37

View in Codacy

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

Thanks for the detailed writeup and tracking doc — this is a well-motivated fix for a real memory issue. I dug into the actual mechanics of the LRU bound and found one issue serious enough to flag before merge.

🔴 Critical: bounding outChunkRIDCache/inChunkRIDCache with an LRU can silently orphan segments and lose edges for vertices created via createVertices()

getOrCreateOutSegmentDeferred/getOrCreateInSegmentDeferred (GraphBatch.java:1680-1742) and their parallel-path twins connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal (GraphBatch.java:2249-2270, :1524-1546) use this pattern:

final RID cachedRID = outChunkRIDCache.get(vertexKey);
if (cachedRID != null) { ... }               // reuse existing segment
if (!knownNewVertexKeys.contains(vertexKey)) { ... load from disk ... }
// falls through here when knownNewVertexKeys DOES contain vertexKey:
// assumes "no segment exists yet" and creates a brand-new one, no setPrevious() link

Before this PR, outChunkRIDCache/inChunkRIDCache were plain, never-evicted ConcurrentHashMaps with the same lifecycle as knownNewVertexKeys (both cleared together in batchUpdateVertexHeadChunks(), called only from close()). So for a known-new vertex, "cache miss" could only ever mean "this really is the first segment" — the invariant the knownNewVertexKeys fast path depends on.

This PR bounds the two RID caches with an LRU (capacity chunkCacheCapacity, default 1,000,000) but leaves knownNewVertexKeys unbounded for the full batch lifetime (only cleared at close()). That breaks the invariant: if a known-new vertex's cache entry gets evicted (easily reached on a large stream — e.g. the 663M-vertex case in the issue, once more than chunkCacheCapacity other distinct vertices are touched between two edges of the same vertex) and the vertex is touched again, the code takes the "known-new" branch again, believes there is still no segment, and creates a second, unlinked segment (new MutableEdgeSegment(...) with no setPrevious(), unlike the genuine-overflow paths at :1956, :1637, :2281, :1557, which do link). The new segment overwrites the old RID in outChunkRIDCache/inChunkRIDCache and in deferredOutHead/deferredInHead, so the pointer to the first segment (holding real, already-committed edges) is lost for good — those edges become unreachable from the vertex once batchUpdateVertexHeadChunks() writes the final head pointer at close().

Note deferredOutHead/deferredInHead are themselves unbounded and always accurate for the vertex's current head pointer, but they're never consulted as a fallback in the "known-new" branch — only the bounded RID cache is checked. Consulting deferredOutHead/deferredInHead before falling back to "assume brand new" would close this hole and doesn't need to be bounded any differently than it already is.

This escapes the new test suite because GraphBatchBoundedStateTest creates vertices with plain database.newVertex()/save(), not importer.createVertices(...), so knownNewVertexKeys is never populated in the new tests — the exact code path that trips over LRU eviction is untested. The existing GraphBatchTest uses createVertices() but with VERTEX_COUNT = 5_000, far below the default 1,000,000 cache capacity, so no eviction ever happens there either. A regression test would need createVertices() for enough vertices (or a small withChunkCacheCapacity) combined with edges spaced out enough to force eviction between a vertex's own edges, then assert the edge count/traversal is still complete.

🟡 Performance: Collections.synchronizedMap(LRUCache) serializes a hot path that used to be lock-free

connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal read and write outChunkRIDCache/inChunkRIDCache directly (not via a per-slot local map merged at the end, unlike deferredOutHead/deferredInHead) from every parallel async slot in connectOutgoingEdgesParallel/connectIncomingEdgesParallel. Previously this was a ConcurrentHashMap, giving essentially lock-free/finely-striped concurrent access. Collections.synchronizedMap(new LRUCache<>(...)) puts every get() and put() from every parallel slot behind one global intrinsic lock — and because LRUCache uses access-order (LinkedHashMap(..., true)), even get() calls mutate the internal linked list, so there's no cheap read path at all anymore. For the exact large-parallelFlush workloads this PR targets, this could meaningfully cut into (or negate) the benefit of parallelFlush=true, since a large fraction of per-vertex work now serializes on this single lock. Worth benchmarking parallelFlush=true throughput before/after on a multi-core box; if it regresses, a striped LRU (e.g. N Collections.synchronizedMap(LRUCache) shards keyed by vertexKey % N, sized chunkCacheCapacity / N each) would keep the bound while preserving concurrency.

🟢 Minor / already acknowledged

• The tracking doc is upfront that deferredOutHead/deferredInHead/knownNewVertexKeys remain unbounded for the batch's full lifetime. Worth being explicit with the issue reporter that for the exact reproduction case (663M vertices, all presumably created via createVertices()), knownNewVertexKeys alone (a LongHashSet of every created vertex) is still O(vertex count) for the whole load — likely several GB at that scale — so this PR only partially resolves the original report, not just as a "future follow-up" but as something that may still matter at the scale that was reported.
• LRUCache javadoc explicitly says "not thread safe, wrap it in Collections.synchronizedMap()" — that part is correctly applied, so no bug there, just the performance tradeoff above.

Nice test coverage otherwise for the two things it does target (bounded cache size, bounded deferred buffer, the 0-disables-early-drain opt-out, and builder validation) — I'd just want the createVertices() + eviction scenario covered before this ships, given it's a silent-data-loss risk rather than something that fails loudly.

[claude]

Review

I traced how the newly-bounded outChunkRIDCache/inChunkRIDCache interact with the rest of GraphBatch's deferred-write bookkeeping (knownNewVertexKeys, deferredOutHead/deferredInHead), and found a correctness bug that I believe is serious enough to block merging as-is. Details and suggested fix below, plus a few smaller notes.

1. Bounding the RID caches while leaving knownNewVertexKeys unbounded can silently orphan edges (data loss)

getOrCreateOutSegmentDeferred/getOrCreateInSegmentDeferred (and the equivalent inline logic in connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal) resolve a vertex's current head chunk with this order:

1. Check outChunkRIDCache/inChunkRIDCache - a hit is authoritative and cheap.
2. On a miss, check knownNewVertexKeys. If present, assume "we know there's no existing segment" and create a brand-new segment with no setPrevious() link, then overwrite the cache and deferredOutHead/deferredInHead entries for that vertex.
3. Otherwise, load the vertex record from disk and read its head-chunk pointer.

Before this PR, step 2's assumption held because the caches were ConcurrentHashMaps that were never evicted - a miss for a knownNewVertexKeys member could only happen on that vertex's very first touch in the whole batch. Now that the caches are bounded LRUs, a miss can also mean "this vertex already got a segment earlier in the batch, but its cache entry was evicted." Step 2 can't tell the difference, and it picks the wrong branch.

Concrete repro shape:

• createVertices() creates vertex V (added to knownNewVertexKeys, which is only cleared in close() -> batchUpdateVertexHeadChunks(), i.e. it lives for the whole batch).
• Edge V -> X is flushed: cache miss + knownNewVertexKeys.contains(V) -> creates segment S1, persists it, sets outChunkRIDCache[V]=S1 and deferredOutHead[V]=S1.
• Enough other distinct vertices get flushed afterward that V's entry is LRU-evicted from outChunkRIDCache (entirely plausible with the default cap of 1M against the 100M-vertex scenario this PR is written for).
• Edge V -> Y is flushed later: outChunkRIDCache.get(V) misses again, knownNewVertexKeys.contains(V) is still true -> a second, unlinked segment S2 is created, and outChunkRIDCache[V]/deferredOutHead[V] are overwritten to point at S2.
• At close(), batchUpdateVertexHeadChunks() sets V's persisted outEdgesHeadChunk to S2. S1 (holding edge V -> X) is now unreachable from V - the edge silently disappears from V's outgoing adjacency, even though it was "successfully" created and committed.

The same root cause (a cache miss being treated as authoritative rather than "unknown") also affects non-knownNewVertexKeys vertices: on a miss, the fallback reads vertex.getOutEdgesHeadChunk()/getInEdgesHeadChunk() straight from the on-disk record, but that field is only updated at close() via batchUpdateVertexHeadChunks() - mid-batch it's stale. If that vertex's true current head (tracked only in deferredOutHead/deferredInHead) has since moved past what's on disk, appending against the stale head and then overflowing into a new segment (newChunk.setPrevious(currentSegment)) links the new chain onto the stale segment rather than the real head, and overwrites deferredOutHead/deferredInHead to point away from the real head - same orphaning effect, just with a slightly different trigger.

Suggested fix: before falling back to knownNewVertexKeys / disk in either helper, check deferredOutHead/deferredInHead (already unbounded, already authoritative for "the most recent segment created this batch") and re-seed the RID cache from there on a hit. That restores the invariant the rest of the design (batchUpdateVertexHeadChunks() applying deferredOutHead/deferredInHead at close) already relies on, without needing to bound those maps too.

2. The new regression test can't catch #1

GraphBatchBoundedStateTest builds a path graph (vertexRIDs[i] -> vertexRIDs[i+1]), so every vertex is touched at most once as a source and once as a destination. That structurally can't exercise "cache entry evicted, then the same vertex is touched again," which is exactly the condition that triggers the bug above. It also creates vertices via the plain database.newVertex() API rather than GraphBatch.createVertices(), so knownNewVertexKeys is never populated in the test at all - the fast-path branch most likely to be hit in the real 100M-vertex scenario described in the PR body is untested. Worth adding a case with (a) vertices created via createVertices(), (b) a degree > 1 per vertex, and (c) enough distinct-vertex churn between an individual vertex's edges to force eviction of its cache entry - then verifying every edge is still traversable, which is precisely what the existing test already checks but can't currently trigger.

3. Minor: Collections.synchronizedMap on the parallel-flush hot path

LRUCache needs accessOrder=true to be an LRU at all, which means even get() mutates the map (moves the entry to the tail), so a ReadWriteLock-style optimization wouldn't help here - synchronizedMap is the right call for correctness. But it does turn what were previously lock-free ConcurrentHashMap.get() calls from connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal into calls that now serialize behind one mutex across every async flush slot (parallelLevel threads). Given outChunkRIDCache/inChunkRIDCache are read on essentially every edge group in the hot parallel-flush path, it'd be worth a quick before/after throughput check on parallelFlush=true with a realistic parallelLevel, since this is exactly the code path the PR's own motivating scenario (663M-vertex gRPC stream) exercises.

4. Minor: stale javadoc

getOrCreateOutSegmentDeferred's comment ("For known-new vertices, skip loading - we know there's no existing segment") documents the invariant that issue #1 breaks. Once fixed, it'd be good to update this comment to explain why a miss is no longer sufficient on its own.

What looks solid

• The LRU capacity/Collections.synchronizedMap wrapping pattern itself matches the existing CypherStatementCache convention, and the builder validation (chunkCacheCapacity <= 0, maxDeferredIncomingEdges < 0) is sensible.
• Lazily re-allocating the deferred incoming-edge arrays in accumulateIncomingEdges() after an early drain nulls them out is a correct, minimal fix for the NPE that the periodic-drain change would otherwise introduce.
• close()'s if (bidirectional && inEdgeCount > 0) guard correctly accounts for the early-drain having already zeroed inEdgeCount.
• Good faith effort at test coverage for the two behaviors the PR intends to add (bounded cache size, early drain, 0-disables-early-drain opt-out) - it's just missing the interaction case above.

Given #1 is a silent-data-loss bug in the exact large-scale-import scenario this PR targets, I'd hold off merging until it's addressed (or the PR is scoped down to just the deferred-incoming-edge early drain, which doesn't have this problem, while the RID cache bounding gets revisited).

[claude]

Review

I read through the full diff (GraphBatch.java + the new GraphBatchBoundedStateTest) and traced the call graph for the sequential and parallel flush paths, close(), and batchUpdateVertexHeadChunks(). Overall this is a careful, well-tested piece of work, and the correctness fix for the LRU-eviction data-loss hole is sound. One design point below is worth resolving before merge (or at minimum tracking explicitly), since it affects whether the PR actually delivers the memory win it's named for.

Correctness of the eviction fix - looks right

I verified by hand that:

• deferredOutHead/deferredInHead are only cleared in batchUpdateVertexHeadChunks(), which runs exclusively from close() (line ~995), never from the new early-drain path in flush(). So they really are authoritative for the vertex's current head for the whole batch lifetime, which is the invariant the fix depends on.
• The unconditional deferredOutHead.get(vertexKey) / deferredInHead.get(vertexKey) check is placed before both the knownNewVertexKeys branch and the on-disk read in all four call sites (getOrCreateOutSegmentDeferred, getOrCreateInSegmentDeferred, connectOutEdgesRangeLocal, connectIncomingEdgesRangeLocal), so both sub-cases (known-new vertex and pre-existing vertex) are covered consistently.
• The parallel path's read of the shared (non-thread-safe) deferredOutHead/deferredInHead from connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal is safe: writes to those maps only happen in the single-threaded merge step after async.waitCompletion(), so within any given parallel round only concurrent reads occur against state from prior (already-merged) rounds.
• accumulateIncomingEdges()'s lazy re-allocation uses the same batchSize initial capacity as the constructor, so the mid-batch drain-then-refill doesn't change buffer growth behavior.

The new regression tests are good: they reproduce the exact silent-loss symptom (edge count short by one, TestHelper's integrity checker flagging the orphaned edge) for both the known-new-vertex and pre-existing-vertex sub-cases, on both flush paths. That's the right shape of test for a silent-data-loss bug - "run it and count" wouldn't have caught this without the integrity check.

Design concern: the memory-reduction goal is only partially met for the PR's own flagship scenario

outChunkRIDCache/inChunkRIDCache are now correctly bounded, but the fix requires every miss to fall back to deferredOutHead/deferredInHead - and those two maps are unbounded for the batch's lifetime (cleared only at close()), populated on every new-or-overflowed segment via persistNewSegment() and the overflow branches.

For the workload this PR is written for - bulk-loading a brand-new graph via createVertices() + edges (the 663M-vertex/14B-edge gRPC stream from discussion #5597) - essentially every touched vertex's first edge creates a brand-new segment, so deferredOutHead/deferredInHead will grow to roughly one entry per distinct vertex touched, i.e. close to the same O(V) growth the old unbounded outChunkRIDCache/inChunkRIDCache had. Since deferredOutHead/deferredInHead already existed pre-PR with this same growth pattern (untouched by this diff), the realistic before/after memory picture for the flagship scenario is closer to "roughly halved" than the "16-18 GB -> 100-150 MB" figure quoted in the class javadoc and PR description - which describes only the two RID caches, not the two maps that are now (as of this PR) load-bearing for correctness on every cache miss.

This isn't a correctness bug and the PR body's "Impact / follow-up" section does mention deferredOutHead/deferredInHead staying unbounded, but it doesn't connect that to the headline memory numbers, and the growth pattern is worse than "bounded by distinct-vertex count" makes it sound for a mostly-new-vertices stream - it's essentially the whole stream's distinct vertex count. Suggestions:

• Correct the "16-18 GB -> 100-150 MB" framing in the class javadoc / PR description to make clear it applies to the RID caches only, not overall batch memory.
• Consider exposing a size getter for deferredOutHead/deferredInHead (mirroring getOutChunkRIDCacheSize()/getInChunkRIDCacheSize()) so operators/tests can actually observe the dominant remaining growth source, rather than it being invisible.
• File the "bound deferredOutHead/deferredInHead via early batchUpdateVertexHeadChunks()" idea as its own tracked issue rather than prose in this PR's description, since as written it's the actual remaining fix for the original discussion Ingesting Massive DB with 600 Million Vertices and 14 Billion Edges #5597 report.

Minor / already acknowledged

• Collections.synchronizedMap(LRUCache) serializing outChunkRIDCache/inChunkRIDCache across parallel-flush async slots (losing the ConcurrentHashMap lock-free access the old code had) is a real throughput concern for parallelFlush=true on a multi-core box. The PR already flags this as a deferred, non-blocking follow-up with a sensible mitigation sketch (sharded LRU); agree it's fine to land without it, but it'd be good to actually get a before/after benchmark rather than let it sit as a TODO indefinitely, since this is a hot path used by every GraphBatch caller.
• outChunkRIDCache.put(vertexKey, deferredRID) on every eviction-miss re-inserts into the bounded LRU, so a workload that alternates between more than chunkCacheCapacity "hot" vertices will thrash (evict-then-immediately-reinsert) rather than settle. Not a bug given the fallback is correct either way, just worth knowing if someone tunes chunkCacheCapacity down aggressively and sees no throughput improvement.

Test coverage

Good on the correctness fix specifically. Nothing exercises the deferredOutHead/deferredInHead growth pattern discussed above (e.g. asserting it does not stay bounded, to make the design tradeoff visible in the test suite rather than just in prose) - not required for this PR, but would pair well with adding the size getters suggested above.

Nit

Builder field alignment (chunkCacheCapacity, maxDeferredIncomingEdges) and the two new DEFAULT_* constants read cleanly and match the surrounding style. No stray System.out/debug output, license headers on the new test file look correct, no new dependencies.

[claude]

Review

Nice, thorough PR: clear root-cause writeup, sensible use of the existing LRUCache + Collections.synchronizedMap pattern (correctly needed here since LRUCache extends LinkedHashMap in access-order mode, so even get() mutates the internal linked list), builder validation, and the third commit shows real rigor in catching a subtle eviction hole. That said, I think the fix in commit 3 is narrower than it needs to be and leaves a real data-loss path open. Details below.

1. Correctness: silent edge loss survives for "regular" (non-createVertices()) vertices, sequential flush path

getOrCreateOutSegmentDeferred / getOrCreateInSegmentDeferred (GraphBatch.java ~L1687-1770), used only by the sequential path (connectOutgoingEdgesSorted / connectIncomingEdgesSequential, i.e. withParallelFlush(false)):

if (!knownNewVertexKeys.contains(vertexKey)) {
  final VertexInternal vertex = (VertexInternal) database.lookupByRID(new RID(bucketId, position), true);
  final RID headChunk = vertex.getOutEdgesHeadChunk();
  if (headChunk != null) { ... return existing ... }
  // falls through here WITHOUT ever consulting deferredOutHead
} else {
  // the new fix: only reachable for knownNewVertexKeys members
  final RID deferredRID = deferredOutHead.get(vertexKey);
  if (deferredRID != null) { ... return it ... }
}
// falls through to "create brand new" in both cases

The bug that commit 3 fixed isn't actually specific to knownNewVertexKeys vertices - it's that any vertex whose OUT/IN segment was newly created earlier in the same batch has that fact recorded only in outChunkRIDCache/deferredOutHead, never persisted to the vertex record until close() → batchUpdateVertexHeadChunks(). That's true of persistNewSegment() (GraphBatch.java ~L1775-1786) regardless of knownNewVertexKeys membership - it always does deferredOutHead.put(...), never database.updateRecord(vertex).

So for a vertex created the normal way (database.newVertex() + .save(), never passed through GraphBatch.createVertices(), hence never added to knownNewVertexKeys):

1. Its first-ever OUT edge in this batch creates a new deferred segment (outChunkRIDCache/deferredOutHead only, vertex record's outEdgesHeadChunk stays null).
2. Enough other distinct vertices get touched in later flush() calls to evict this vertex's outChunkRIDCache entry (very plausible with the default chunkCacheCapacity=1_000_000 on exactly the 100M+-vertex streams this PR is written for).
3. A later flush() gives this same vertex a second OUT edge. Cache miss, not in knownNewVertexKeys → loads the vertex fresh from disk → getOutEdgesHeadChunk() is still null (never persisted) → wrongly assumes "no segment yet" → creates a second, disconnected segment, overwriting deferredOutHead[vertexKey] → the first segment (and the edge in it) is silently orphaned.

This is the exact same failure class the PR's commit 3 was written to fix, just for a wider set of vertices than the knownNewVertexKeys guard covers. The fix, I think, should drop the knownNewVertexKeys gate on the deferredOutHead/deferredInHead fallback entirely (or at least check it whenever the persisted headChunk comes back null, not only inside the else branch) - it's unconditionally safe to consult since deferredOutHead/deferredInHead are authoritative for any vertex touched so far in the batch.

Worth noting the parallel path (connectOutEdgesRangeLocal's "not known-new" fallback) doesn't have this hole, because it goes through getOrCreateOutEdgeChunk(), which does persist the vertex record immediately (database.updateRecord(vertex)) - so this is specific to withParallelFlush(false). Since parallelFlush defaults to true, the blast radius is anyone who has explicitly opted into sequential flush (a documented, supported option, and used directly in this PR's own zeroCapDisablesEarlyDrain test).

Suggested regression test: mirror knownNewVertexSurvivesCacheEvictionBetweenItsOwnEdgesSequential, but create the vertices with plain database.newVertex().save() instead of importer.createVertices(...) - that's the one scenario the existing GraphBatchBoundedStateTest doesn't cover (its bounded-cache test creates vertices the same "regular" way, but each vertex only gets one edge per direction, so it never exercises "two edges from the same regular vertex with eviction in between").

2. Performance: Collections.synchronizedMap puts a single global lock on the hottest read in the parallel-flush path

outChunkRIDCache/inChunkRIDCache used to be ConcurrentHashMap - effectively lock-free reads, fine-grained writes. They're now Collections.synchronizedMap(LRUCache), which serializes every get/put behind one monitor. connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal (the parallel-flush workers, one per async slot, parallelFlush=true is the default) call outChunkRIDCache.get(vertexKey)/inChunkRIDCache.get(vertexKey) once per distinct source/destination vertex group - i.e. very frequently, from every slot concurrently. That's a new contention point on precisely the "big batch, many parallel slots" scenario this PR is optimizing for (the 663M-vertex / 14B-edge stream from discussion #5597). Given ArcadeDB's performance mandate, it'd be worth either benchmarking parallel-flush throughput before/after this change, or considering a sharded/striped LRU (e.g. N LRUCache instances keyed by vertexKey hash, each independently locked) instead of one global lock.

3. Minor: stale concurrency comment

The class-level comment on deferredOutHead/deferredInHead (GraphBatch.java ~L216-224) says:

The PARALLEL paths (connectOutEdgesRangeLocal, connectIncomingEdgesRangeLocal) do NOT touch these fields directly - they write to per-flush local ConcurrentHashMap parameters ... LongObjectHashMap ... is safe because no concurrent writes ever hit these fields.

Commit 3 added direct reads of deferredOutHead/deferredInHead from inside both of those parallel methods (L1525/L2278 in the merged diff), which contradicts "do NOT touch these fields directly." I believe the reads are actually safe (no concurrent writer touches these fields during a parallel round - writes only land via the sharedDeferred*Head merge after waitCompletion() - and LongObjectHashMap.get() is a pure read with no internal mutation, unlike the LRU cache), but the comment should be updated to say so explicitly, since it's a subtle invariant that a future change could easily violate by adding a write into the parallel path without noticing this doc no longer matches reality.

Nits

• outChunkRIDCache/inChunkRIDCache in getOrCreateOutSegmentDeferred's else branch also do outChunkRIDCache.put(vertexKey, deferredRID) to repopulate the cache on the fallback hit - good, this avoids repeatedly missing on the same evicted key. Same in the IN counterpart and in both connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal. Nice touch.
• Test coverage for the two bugs actually fixed here (unbounded cache growth, unbounded deferred-buffer growth, and the known-new eviction hole) is solid and reproduces the failure pre-fix per the PR description.

Given point 1, I'd hold off merging until the knownNewVertexKeys gate on the deferredOutHead/deferredInHead fallback is either removed or the fallback is reachable independent of that flag, with a regression test for the "regular vertex, sequential flush, eviction between two of its own edges" case.

[codecov]

Codecov Report

❌ Patch coverage is 93.51145% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.22%. Comparing base (f2754ae) to head (c31233a).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...e/src/main/java/com/arcadedb/graph/GraphBatch.java	92.82%	8 Missing and 8 partials ⚠️
...n/java/com/arcadedb/utility/LongObjectHashMap.java	97.43%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5950      +/-   ##
==========================================
+ Coverage   68.14%   68.22%   +0.07%     
==========================================
  Files        1797     1797              
  Lines      153216   153381     +165     
  Branches    32457    32487      +30     
==========================================
+ Hits       104411   104639     +228     
+ Misses      35404    35326      -78     
- Partials    13401    13416      +15     

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

[robfrank]

Thanks for the thorough re-review. Checking the timeline: this comment landed 23:17 UTC, but only discusses "commit 3" (1dedd8161) - it doesn't reflect commit 4 (529fcd1d6), which was already pushed at 23:05:57 UTC, 11 minutes earlier. That commit already does exactly what point 1 asks for: it drops the knownNewVertexKeys gate entirely and checks deferredOutHead/deferredInHead unconditionally, before any branching, in both getOrCreateOutSegmentDeferred/getOrCreateInSegmentDeferred (the sequential path this comment calls out) and the parallel connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal. Two new regression tests (preExistingVertexSurvivesCacheEvictionBetweenItsOwnEdgesSequential/...Parallel) cover exactly the "regular vertex, sequential flush, eviction between two of its own edges" scenario this comment asks for, using plain database.newVertex() (not createVertices()) - verified RED against 1dedd8161, GREEN against 529fcd1d6. See the PR description's "Review cycle 1" section for the full account.

Point 2 (the synchronizedMap contention concern) is unchanged from the prior review - still deferred, not fixed, with rationale in the PR body (correctness took priority; happy to follow up with a sharded LRU if a benchmark shows a real parallelFlush=true regression).

Point 3 (the stale "do NOT touch these fields directly" comment) was a fair catch and is fixed in 0e7f4b13c - updated to explain that the parallel paths now read deferredOutHead/deferredInHead directly (added by the point-1 fix) and why that's safe (pure read, no concurrent writer during a parallel round).

[claude]

Review of #5950

Solid piece of work overall - the PR description's self-documented review cycle (catching and fixing the knownNewVertexKeys LRU-eviction data-loss bug across two commits) reflects real rigor, and I independently traced all four call sites (getOrCreateOutSegmentDeferred/getOrCreateInSegmentDeferred, connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal) - the "check deferredOutHead/deferredInHead unconditionally before knownNewVertexKeys" fix is correct and consistent between the sequential and parallel paths. LongObjectHashMap.get() is confirmed allocation/mutation-free, so the newly-documented concurrent-read claim in the class comment holds.

One new issue I found that isn't covered by the existing tests:

Correctness: early in-flush draining can duplicate incoming edges if connectDeferredIncomingEdges() fails partway through

flush() now calls connectDeferredIncomingEdges() directly (PHASE 4, GraphBatch.java:677-678) once inEdgeCount crosses maxDeferredIncomingEdges (default 5,000,000). That method does incremental database.commit() calls every commitEvery edges (default 50,000) inside connectIncomingEdgesSequential/connectIncomingEdgesParallel, and only resets inEdgeCount = 0 / nulls the buffer arrays after that call returns successfully (GraphBatch.java:1450-1457).

If an exception is thrown partway through that pass (a NeedRetryException/ConcurrentModificationException that isn't retried here - unlike vertex creation, there's no commitRetries wrapping around these commits - a unique-index violation, disk-full, etc.), some sub-batches have already been durably committed with correct back-pointers, but inEdgeCount and the deferred buffer arrays are left untouched at their pre-call values. flush()'s catch block resets edgeCount = 0 (outgoing buffer) but does not touch inEdgeCount/the incoming arrays (GraphBatch.java:689-696).

The exception propagates out of flush() (and, since flush() is invoked automatically from newEdge() once batchSize is reached, out of newEdge() too) - which, per the class's own documented usage pattern (try (GraphBatch batch = ...) { ... }), triggers close() via try-with-resources auto-close. close() unconditionally re-runs connectDeferredIncomingEdges() over the same stale inEdgeCount buffer (GraphBatch.java:1051-1052) - this is deliberate, existing behavior for issue #4113 (avoid orphaned back-pointers after a failed flush), but it now reprocesses edges that were already committed in the failed early-drain attempt, producing duplicate incoming-edge entries for those vertices instead of just the orphaned-back-pointer situation #4113 was designed to guard against.

This requires no unusual caller behavior - just the textbook usage shown in the class javadoc, plus one failure inside a large early drain (a real possibility at the scale this PR targets: 5M-edge drains, commitEvery-driven commits, no retry wrapper, and - per this repo's own engine notes - CME/NeedRetryException is the designed response to concurrent page conflicts under replication).

None of the new GraphBatchBoundedStateTest cases exercise a failure during a drain (early or at close), so this wouldn't have been caught by the added coverage. Worth a regression test that forces a RuntimeException mid-connectDeferredIncomingEdges() (a NeedRetryException test hook similar to the existing vertex-commit fault-injection hook at GraphBatch.java:166, or a UNIQUE-constraint violation timed to land after a couple of commitEvery boundaries) and asserts no duplicate incoming edges after close().

Minor: parallel path doesn't repopulate the RID cache on a deferredOutHead/deferredInHead fallback hit

In connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal, the new deferredChunkRID != null branch (GraphBatch.java:2450-2455, 1633-1638) reads the fallback but doesn't write it back into sharedOutChunkCache/sharedInChunkCache - only the isNew/overflow branches do. The sequential counterpart (getOrCreateOutSegmentDeferred/getOrCreateInSegmentDeferred) does re-populate outChunkRIDCache/inChunkRIDCache on the same fallback (GraphBatch.java:1835, 1886). Not a correctness issue (functionally still correct, since deferredOutHead/deferredInHead remain authoritative), just a small, easily-fixed inconsistency that means a vertex touched again in the parallel path keeps paying for the fallback lookup instead of getting cached after the first recovery.

Other notes

• The deferred performance follow-up (Collections.synchronizedMap(LRUCache) serializing the parallel-flush hot path) is reasonably deferred - it's explicitly non-blocking and the PR is honest about not having benchmarked it. Agreed this shouldn't hold up the fix.
• LRUCache's removeEldestEntry / access-order semantics were double-checked and are correct for this use (evict-on-exceed, not evict-on-equal).
• Builder validation (withChunkCacheCapacity, withMaxDeferredIncomingEdges) and defaults look reasonable and are tested.
• Nice attention to detail keeping GRAPH_EDGE_APPEND_MERGE/bulk-write invariants and the super-node routing (GraphBatch never promotes super-nodes, and hard-fails on an already-promoted vertex #5667) untouched by this change.

Nothing here should block review cycle 2 from focusing on the reentrancy issue above, since that's the one with a real data-corruption path; the cache-refresh note is a nice-to-have.

[robfrank]

Review cycle 3 - fixed

Confirmed the reentrancy bug: flush()'s early incoming-edge drain (issue #5664) can fail partway through connectDeferredIncomingEdges() after some slices already committed durably, and close()'s unconditional retry then reprocessed the whole stale buffer, duplicating the already-committed work.

Fixed in 6d296b284:

• Sequential path: a resume cursor (inEdgesResumeSortIndex) tracks how far a fully-committed prefix reaches, advanced only after each successful commit, so a retry skips already-committed groups.
• Parallel path: a completedIncomingBuckets set tracks which destination-bucket tasks already committed, marked from the transaction's post-commit OkCallback (never from inside the bucket lambda, which runs before the commit and could still fail).

Building the reproduction test surfaced a second, deeper bug in the same family: deferredInHead/inChunkRIDCache get mutated before the containing transaction commits, so a rolled-back slice left phantom RIDs behind - the retry's resume logic correctly skipped the committed groups, but the very next group crashed with RecordNotFoundException reading a rolled-back record. Fixed with an undo log on the sequential path (needed LongObjectHashMap.remove(), added via tombstone deletion, with 17 new unit tests) and per-bucket local maps merged only on success for the parallel path.

New TEST_BEFORE_INCOMING_EDGE_COMMIT_HOOK fault-injection hook plus Issue5950IncomingEdgeDrainReentrancyTest (both flush paths) - verified RED pre-fix (reproduced the exact duplicate-edge and then the RecordNotFoundException symptoms), GREEN post-fix. Full details, including a related-but-out-of-scope finding on the outgoing-edge side, are in the PR body under "Review cycle 3".

Verification: com.arcadedb.graph.*Test sweep (excluding slow/benchmark) 206/206 passed; targeted GraphBatch/LongObjectHashMap sweep 52/52 passed.

[claude]

Review

I read through the current GraphBatch.java, the LongObjectHashMap changes, and the new tests independently (not just the PR narrative), focusing on the concurrency/correctness surface given engine/CLAUDE.md's note that page-level ConcurrentModificationException is common on this exact code path (supernode/edge-chunk appends touching the same page).

🔴 The "deliberately out-of-scope" outgoing-side gap is reachable within a single flush(), not just across attempts

The PR body reasons that the eager-write-before-commit pattern on the outgoing side (connectOutEdgesRangeLocal → getOrCreateOutEdgeChunk, which writes straight into the class-level outChunkRIDCache and calls database.updateRecord(vertex) before the surrounding transaction commits) is safe to leave unfixed because "outgoing edges have no retry-across-attempts mechanism today - a failed flush() unconditionally drops the buffered outgoing edges."

That's true for retries across flush()/close() calls, but connectOutgoingEdgesParallel schedules each bucket via async.transaction(..., 3, ...), and DatabaseAsyncTransaction.execute() retries the same lambda up to 3 times on ConcurrentModificationException, rolling back and re-invoking connectOutEdgesRangeLocal from scratch after each failed attempt:

} catch (final ConcurrentModificationException e) {
  lastException = e;
  if (database.isTransactionActive())
    database.rollback();

If attempt 1 reaches getOrCreateOutEdgeChunk for some vertex (creating a new segment + writing outChunkRIDCache+ vertex.setOutEdgesHeadChunk/updateRecord immediately), then hits a CME later in the same bucket range and rolls back, attempt 2 re-enters connectOutEdgesRangeLocal for the same [from,to). It will hit the now-stale outChunkRIDCache entry first (line ~2598) and database.lookupByRID(cachedChunkRID, true) a RID that belongs to a rolled-back transaction - i.e. a record that was never durably created. That should surface as RecordNotFoundException, turning what should be a transparently-retried transient conflict into a hard batch failure (or, if the position happens to get reused, silent misattachment).

This is the exact same bug class review cycle 3 fixed for the incoming side (local per-bucket maps, merged only after the OkCallback fires post-commit) - connectIncomingEdgesRangeLocal correctly defers via sharedInChunkCache/sharedDeferredInHead now, but connectOutEdgesRangeLocal's fallback branch (getOrCreateOutEdgeChunk) still writes directly to the shared, class-level outChunkRIDCache before commit. Given this is triggered by ordinary page-conflict CMEs under parallelFlush=true (which engine/CLAUDE.md describes as a normal, expected occurrence on this exact code path - two edges into the same chunk), this seems more reachable than "pre-existing latent issue, distinct piece of work" suggests, and arguably belongs in this PR's fix rather than deferred, since the PR already built the local-map-merge machinery for the IN side and the OUT side needs the structurally identical treatment.

🟡 LongObjectHashMap.remove()'s tombstones aren't counted toward the resize threshold

resize() triggers off size >= threshold, and size only counts live entries (remove() decrements it). Tombstones left behind by remove() are never counted against threshold and are only reclaimed opportunistically (by a later put() landing on that exact slot, or by a resize() triggered by live-entry growth). If a workload does many more remove()s than put()s that grow size past the next threshold, the table can end up saturated with tombstones + live entries and zero remaining EMPTY_KEY slots. At that point get(), put(), remove(), and containsKey() all loop forever, since their linear probes only terminate on EMPTY_KEY.

For this PR's actual usage (inHeadUndoLog restore path in connectIncomingEdgesSequential's catch block), removes are rare relative to puts, so it's unlikely to bite in practice today. But LongObjectHashMap is a general-purpose utility (not private to GraphBatch), and remove() is a new public capability being added here - worth guarding against, e.g. by tracking tombstone count and resizing (or rehashing in place) when size + tombstones >= threshold, not just size.

🟡 The incoming-edge drain resume state assumes "no intervening newEdge() calls" - an assumption that isn't enforced

The inEdgesResumeSortIndex / completedIncomingBuckets javadoc explicitly says this mechanism is "valid only because... close() retries immediately with no intervening newEdge() calls." Nothing in the code actually enforces that. If a caller catches the exception from a failed flush() (early drain crossed maxDeferredIncomingEdges and failed) and keeps calling newEdge() - which is exactly the kind of "catch a transient failure and keep streaming" pattern this same class already supports elsewhere (createVerticesWithRetry, built for issue #4724's Raft-hiccup case) - then a later flush() calls accumulateIncomingEdges() again, appending new rows onto the still-undrained buffer. The next connectDeferredIncomingEdges() call rebuilds inSortIndex from scratch via partitionIncomingByDestBucket over the now-larger inEdgeCount. Since that's a counting sort, adding rows changes bucketCounts/bucketOffsets for every bucket at or after the affected one(s), which shifts what a given raw index into inSortIndex means. inEdgesResumeSortIndex (a raw array index) and completedIncomingBuckets (bucket ids, safer than the index but still assumes the same bucket→range mapping) are then used against a differently-shaped index, which can skip groups (data loss) or reprocess them (duplication) - silently.

GraphBatch has no "poisoned" state after a drain failure - nothing stops newEdge() from being called again. Given three review cycles already went into making the retry path idempotent, it'd be worth either (a) hard-documenting + enforcing (e.g. throw IllegalStateException from newEdge()/flush() if a previous incoming-edge drain failed and hasn't been retried yet), or (b) making the resume mechanism robust to buffer growth (e.g. snapshot/freeze the prefix that existed at drain-start rather than reusing a raw index across a rebuilt sort).

Nits

• TEST_BEFORE_INCOMING_EDGE_COMMIT_HOOK is another public static volatile global test hook (mirrors the existing TEST_BEFORE_VERTEX_COMMIT_HOOK pattern, so at least it's consistent) - just flagging that this doubles the surface area for cross-test pollution if a test forgets the @AfterEach reset or tests ever run in parallel within one JVM.
• Issue5950IncomingEdgeDrainReentrancyTest.parallelRetryDoesNotDuplicateAlreadyCommittedBuckets calls database.async().setParallelLevel(1) without restoring it; probably fine given TestHelper's per-test database lifecycle, but worth double-checking it can't leak into a later test sharing the executor.

What looks solid

• The core LRU-bound + deferredOutHead/deferredInHead fallback fix for the known-new-vertex and pre-existing-vertex silent-data-loss bug (review cycles 1-2) is correct as far as I can tell, and well covered by GraphBatchBoundedStateTest's eviction-between-two-edges regression tests for both flush paths.
• LongObjectHashMap.remove()'s tombstone/probe-chain logic itself (aside from the resize-threshold gap above) looks correct: put() reclaims the first tombstone seen while probing, get()/containsKey() correctly skip over tombstones without stopping, and resize() correctly drops tombstones during rehash.
• Good discipline throughout on documenting why (not just what) for every non-obvious concurrency decision - genuinely made this easier to review.
• Test coverage for the specific failure modes that were found is thorough and uses fault-injection hooks appropriately rather than trying to force real races.

Given the size and concurrency depth of this change, I'd suggest at minimum resolving the outgoing-side gap (finding #1) before merge, since it's the same bug class the whole PR is about, just on the other edge direction - and it's a normal-conditions repro path (CME + parallelFlush=true), not an exotic one.

[robfrank]

Review cycle 4 - all three findings fixed in d7cf46879

🔴 Outgoing-side gap: you're right, and my cycle-3 deferral was wrong

I had only considered retries across flush()/close() calls and missed that async.transaction(..., 3, ...) retries the same bucket lambda on a CME within a single flush(). Verified by reproducing it: RecordNotFoundException: Record #2:0 not found, the retry dereferencing a segment RID from the rolled-back attempt, exactly as you described.

I also found a second sub-case while fixing it: parallelDeferredOutHead/parallelOutChunkCache were merged into the class-level maps unconditionally after waitCompletion(), including buckets that exhausted all 3 retries - poisoning deferredOutHead (the authoritative fallback the cycle-1/2 fix depends on) for the rest of the batch.

Fixed by giving the OUT path the identical treatment the IN path got in cycle 3: per-bucket local maps merged only for buckets whose commit succeeded, plus new-segment creation deferred out of getOrCreateOutEdgeChunk(). Both directions now also clear their local maps per attempt so a CME retry can't inherit rolled-back entries. New Issue5950OutgoingEdgeRetryCacheTest + an OUT-side commit hook; RED→GREEN.

🟡 Tombstone/threshold: confirmed as a real hang

Reproduced: with tombstones uncounted, the test class ran for 400+ seconds without completing a single test (it normally takes 0.08s). Tombstones now count toward the threshold (size + tombstones >= threshold, i.e. the non-empty slot count), and the rehash sizes from the live count so churn reclaims slots instead of doubling capacity forever.

One note worth flagging: my first version of the guard used a plain @Timeout, which does not work for this - JUnit's default same-thread timeout can only report a breach after the method returns, which an infinite loop never does, so it would hang the build rather than fail it. The committed tests use threadMode = SEPARATE_THREAD.

🟡 Resume state vs. intervening newEdge(): fixed with (b), not (a)

Confirmed reachable - nothing enforced the assumption my own javadoc stated. I went with your option (b) rather than fail-fast: for a bulk loader, turning a survivable transient failure into a dead batch is the worse outcome. Each drain pass now pins the row count it covers and a retry reuses that exact pin, so the counting-sort index has an identical shape across attempts; rows appended past the pin drain as a separate pass. On the normal path the prefix equals the full buffer, so it's a counter reset with no copying.

Building this also exposed that my first test design couldn't fail (the appended rows happened to sort after the prefix). The committed version perturbs the index both ways and reproduces two distinct symptoms: sequential loses a row in a lower-sorting bucket, parallel loses a row added to an already-completed bucket.

Nits

• Third static hook: kept. It matches the existing TEST_BEFORE_VERTEX_COMMIT_HOOK convention, each is @AfterEach-cleared, and forcing a real CME on a chosen bucket isn't deterministic. On parallel execution specifically: forkCount=1 and no junit.jupiter.execution.parallel.enabled anywhere, so this is currently hypothetical - and the pre-existing vertex hook has identical exposure, so they'd want addressing together rather than diverging one of them now.
• setParallelLevel(1): checked rather than assumed - it can't leak. parallelLevel is per-DatabaseAsyncExecutor, and TestHelper builds a fresh database (fresh executor) per test method in its constructor and drops it in @AfterEach. A restore would only churn the pool of a database about to be dropped, so I documented the reasoning in place instead.

Full sweep: com.arcadedb.graph.*Test + LongObjectHashMapTest, 232/232 passing. Each fix independently verified RED before / GREEN after by reverting only that fix. Details in the PR body under "Review cycle 4"; the stale out-of-scope paragraph from cycle 3 is corrected there too.

[claude]

Reviewed the current state of the diff (post review-cycle-4). This PR has already been through an unusually thorough internal review loop (the knownNewVertexKeys/LRU-eviction bug, the incoming-drain reentrancy bug, the outgoing-side CME/cache-poisoning bug, and the LongObjectHashMap tombstone-starvation hang are all real, well-diagnosed, and the fixes look correct to me on a full read of GraphBatch.java and LongObjectHashMap.java). I won't re-litigate those; two things stood out that aren't mentioned in any of the four review cycles.

Correctness / design

deferredOutHead/deferredInHead are still unbounded for the full batch lifetime, which quietly reopens the exact problem issue #5664 set out to fix.

The whole point of bounding outChunkRIDCache/inChunkRIDCache with an LRU was "a long-lived stream's cache memory does not grow with distinct vertex count." But the fix for the LRU-eviction correctness bug makes deferredOutHead/deferredInHead the authoritative fallback, and those two maps are explicitly never bounded or drained early -- batchUpdateVertexHeadChunks() (the only thing that clears them) is called exclusively from close(), never from flush():

grep -n "batchUpdateVertexHeadChunks" engine/src/main/java/com/arcadedb/graph/GraphBatch.java
1125:          batchUpdateVertexHeadChunks();   // only call site, inside close()
1284:  private void batchUpdateVertexHeadChunks() {

So for a genuinely long-lived stream that touches many millions of distinct vertices (i.e. exactly the scenario #5664 was written for), deferredOutHead + deferredInHead will still grow to O(distinct vertices touched) for the entire batch duration -- smaller per-entry footprint than before thanks to LongObjectHashMap, but not bounded. The incoming-edge buffer got an early-drain mechanism to solve this same shape of problem (maxDeferredIncomingEdges triggers connectDeferredIncomingEdges() from flush()); the head-pointer maps got no equivalent (e.g. periodically calling batchUpdateVertexHeadChunks() + clearing once memory/entry-count crosses a cap). Worth either doing the same early-drain treatment for these two maps, or explicitly documenting in the class javadoc that the RID-cache/incoming-buffer bounds don't actually cap total batch memory for a very-high-cardinality stream, so it doesn't read as fully solved.

Performance

Collections.synchronizedMap(new LRUCache<>(...)) puts a single global lock on the hottest path of the parallel flush.

outChunkRIDCache/inChunkRIDCache replace a lock-free ConcurrentHashMap with a synchronizedMap-wrapped LinkedHashMap (accessOrder=true). Two compounding costs on the parallel path (connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal, one call per edge group, across every async slot):

• LinkedHashMap with accessOrder=true mutates its internal doubly-linked list on every get(), not just put() -- so even a cache hit needs exclusive access, not just a read.
• synchronizedMap uses one mutex for the whole map, and it's genuinely hit from multiple threads concurrently: besides the post-waitCompletion() merge, connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal call outChunkRIDCache.put(...)/inChunkRIDCache.put(...) directly and immediately (for the "existing, durable head chunk" branch), from whichever async thread is running that bucket.

Given GraphBatch's whole reason for existing is high-throughput parallel bulk loading, serializing every edge's cache lookup/insert behind one lock could meaningfully cap parallel scalability at higher core counts/parallelLevel, in a way a single-threaded correctness test won't surface. Might be worth a quick throughput comparison against the old ConcurrentHashMap on a multi-core parallelFlush=true run, or considering a sharded/striped LRU (e.g. N independent LRUs keyed by vertexKey % N, each with capacity/N) to keep the memory bound without the single choke point.

Everything else

• LongObjectHashMap's tombstone remove() + size + tombstones >= threshold resize trigger looks correct to me (probe termination is preserved, resize() correctly drops tombstones and sizes off live size), and the new @Timeout(..., SEPARATE_THREAD) regression tests for the starvation hang are a good touch -- a same-thread @Timeout genuinely wouldn't have caught it.
• Test coverage across the four bug classes is thorough and each test's docstring clearly states what pre-fix RED behavior it reproduces -- good regression-test hygiene.
• No security concerns (no external/untrusted input on this path).
• No new dependencies, so no ATTRIBUTIONS.md/NOTICE changes needed -- none required here anyway since LRUCache already existed in com.arcadedb.utility.
• Minor/non-blocking: GraphBatch.chunkCacheCapacity is stored as an instance field but only read once, in the constructor, to build the two caches -- could just be a local variable unless something else is meant to consult it later.

[codacy-production]

Not up to standards ⛔

🔴 Issues 2 medium

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

Results:
2 new issues

Category	Results
BestPractice	2 medium

View in Codacy

🟢 Metrics 37 complexity

Metric	Results
Complexity	37

View in Codacy

🟢 Coverage 96.95% diff coverage · -6.36% coverage variation

Metric	Results
Coverage variation	✅ -6.36% coverage variation
Diff coverage	✅ 96.95% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (f2754ae)	153216	117852	76.92%
Head commit (c31233a)	185724 (+32508)	131052 (+13200)	70.56% (-6.36%)

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 (#5950)	262	254	96.95%

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

I read through the full diff (GraphBatch.java, LongObjectHashMap.java, and the new/updated tests) independently of the PR description's own review-cycle writeups, focusing on the concurrency-sensitive parts: the LRU-bounded RID caches, the early incoming-edge drain/resume machinery, the per-bucket local-map merge pattern for both OUT and IN directions, and the new tombstone-based remove() in LongObjectHashMap.

Correctness

The fixes described across review cycles 1-4 hold up under a manual trace:

• getOrCreateOutSegmentDeferred/getOrCreateInSegmentDeferred now check the unbounded deferredOutHead/deferredInHead unconditionally before falling back to knownNewVertexKeys/on-disk reads. Traced through both the known-new-vertex and pre-existing-vertex cases; the eviction-mid-batch scenario is correctly handled.
• Parallel-path retry safety (cycle 4): confirmed connectOutEdgesRangeLocal/connectIncomingEdgesRangeLocal no longer write new-segment RIDs directly into the shared outChunkRIDCache/inChunkRIDCache/deferredOutHead/deferredInHead before the surrounding async.transaction commits. The per-bucket local maps are correctly scoped (created fresh per attempt, cleared at the top of the lambda so a CME retry doesn't inherit a rolled-back attempt's entries) and merged into shared state only from the post-commit OkCallback, gated on completedIncomingBuckets/local completedOutgoingBuckets. Caching an existing, already-durable head chunk immediately (the srcHeadChunk != null branch) is correctly distinguished from caching a newly created segment - only the latter needs to be deferred.
• Resume/pin mechanism (inEdgesDrainPrefix, inEdgesResumeSortIndex, completedIncomingBuckets): traced the interaction between discardDrainedIncomingPrefix and the pin - a partially-failed drain retains its pin across flush()'s catch block (which only clears the outgoing buffer, by design), so a retry from close() rebuilds an identically-shaped sort index. The "keep streaming after a caught failure" edge case (new rows appended past the pin) is handled correctly since rows past the pin are drained in a separate loop iteration.
• LongObjectHashMap.remove()/tombstones: the size + tombstones >= threshold accounting is correct (occupancy = non-EMPTY slots, and probes only terminate on EMPTY_KEY, so this is exactly the right invariant to protect). resize() correctly sizes from live size rather than always doubling, so a remove-heavy workload reclaims tombstones in place. Reused-tombstone-slot bookkeeping in put() (tombstones--, size++, net occupancy unchanged) is correct.

Minor nit (non-blocking)

LongObjectHashMap.get() and containsKey() don't guard against key == TOMBSTONE_KEY the way put()/remove() do. If TOMBSTONE_KEY (Long.MIN_VALUE + 2) were ever passed as a lookup key, the probe would spuriously match the first tombstone slot encountered and return null "by luck" rather than by an explicit early-return. Not reachable today since GraphBatch only ever looks up real vertex keys, but for a general-purpose reusable utility class it'd be more defensive to short-circuit on both sentinels in get/containsKey as well, for consistency with put/remove.

Style/process

• Follows repo conventions: reuses the existing LRUCache (already used by CypherStatementCache) instead of introducing a new dependency; uses the existing async executor rather than any new thread pool, consistent with the engine's concurrency rules; benchmark tests are correctly @Tag("benchmark")-annotated to stay out of regular CI; no stray System.out debug prints.
• Test coverage is thorough and well-targeted: each fix has a test that was verified RED pre-fix / GREEN post-fix (per the PR description), and the new Issue5950*Test classes use fault-injection hooks that mirror the existing TEST_BEFORE_VERTEX_COMMIT_HOOK pattern rather than inventing a new mechanism.
• The deferred/not-fixed item (Collections.synchronizedMap(LRUCache) serializing the parallel-flush hot path) is honestly flagged rather than silently dropped, and the PR backs the "no regression" claim with an actual before/after benchmark rather than just asserting it.

Overall

This is a large, concurrency-heavy change to a hot data-loading path, but the final state is well-reasoned and the regression tests target the actual failure mechanisms (rolled-back RIDs leaking into shared caches, partial-drain reprocessing) rather than just re-asserting happy-path behavior. I didn't find any correctness issues beyond the tombstone-guard nit above. Nice work tracking down the two "eager write before commit" hazards (IN in cycle 3, OUT in cycle 4) - that's a subtle class of bug that's easy to miss on a first pass since it only manifests under CME retry / partial-commit-failure, not on the happy path exercised by most tests.

[robfrank]

Review cycle 5 - addressed, plus two corrections to the record

Fixed in c31233a02:

• get()/containsKey() now short-circuit on TOMBSTONE_KEY, matching put()/remove(). Worth noting the regression test needed care to be able to fail at all: a tombstone placed anywhere isn't enough, because the probe stops at the first EMPTY slot and returns false for the wrong reason - the test would pass with the fix reverted. It now replicates the map's MurmurHash3 finalizer to plant the tombstone at exactly the slot the sentinel's own probe starts on. Verified RED (expected false but was true) before the fix, GREEN after.
• The GraphBatch class javadoc now states the memory bound is partial: deferredOutHead/deferredInHead/knownNewVertexKeys still grow with distinct-vertex count, and since this PR they are load-bearing rather than an optimization (they are the authoritative fallback that stops a cache eviction from orphaning an earlier segment). Agreed with the framing that leaving this implicit would let GraphBatch retains per-vertex state for the whole batch lifetime, so a long-lived bulk stream grows without bound #5664 read as more fully solved than it is.

The synchronizedMap(LRUCache) question is now measured, not deferred. Benchmarked on 12 cores / 11 async worker threads, 100k vertices x 1M edges, median of 5:

	pre-PR baseline (f2754aee3)	this PR	delta
parallelFlush=true	1754 ms · 570k edges/s	1734 ms · 577k edges/s	~1% faster
parallelFlush=false	1612 ms · 620k edges/s	1586 ms · 631k edges/s	~1.5% faster

No measurable regression; both within noise and both trending faster. Harness committed as performance/GraphBatchDrainPerfBenchmark.java, deliberately written to use only builder methods present in both trees so the identical source runs against the pre-PR baseline. Full details and the eviction-cost measurement are in the PR description.

Two corrections:

1. An earlier review stated this PR "backs the no-regression claim with an actual before/after benchmark." That was not true when written - no benchmark existed at that point. It is true now, as of 8790f89ac. Flagging so the earlier sentence isn't read as independent confirmation.
2. The red integration-tests check is a pre-existing main breakage, not this PR. It fails identically on f2754aee3 (this branch's own merge-base), a049d9325, b380e9dca and 81362c771. The job log ends mid-HTTPDocumentIT with no assertion failure, which looks like a timeout/kill rather than a test failure. Not caused by, and not fixable in, this PR - but worth someone opening an issue for separately.

[claude]

Review

I read through the actual diff and the current state of GraphBatch.java, LongObjectHashMap.java, LRUCache.java, and DatabaseAsyncTransaction.java directly (not just the PR description), since this touches concurrency-sensitive, correctness-critical code and the PR body documents five rounds of self-review already. Independent spot-checks below.

Verified independently

• DatabaseAsyncTransaction.execute(): confirmed onOkCallback.call() only fires after database.commit() succeeds, and only ConcurrentModificationException triggers the same-lambda retry (other exceptions roll back and rethrow without retry). This backs the completedIncomingBuckets/completedOutgoingBuckets "mark-only-from-OkCallback" design in connectIncomingEdgesParallel/connectOutgoingEdgesParallel — membership really does imply a durable commit.
• beginTx() is a no-op when a transaction is already active, which is what makes the sequential resume (inEdgesResumeSortIndex) safe to reuse across a flush()-catch rollback + close() retry.
• LongObjectHashMap: walked the tombstone/resize logic by hand. size + tombstones >= threshold correctly bounds the non-empty slot count, guaranteeing an empty slot always remains for probe termination — this genuinely fixes the infinite-loop hazard the PR describes (open addressing with only an EMPTY_KEY probe-stop condition is the classic way to introduce this bug once remove() is added). resize()'s "size from live count" loop correctly avoids unbounded doubling on remove-heavy churn. The get()/containsKey() tombstone-sentinel guard is exercised by a genuinely nasty test (tombstoneSentinelIsInertOnEveryReadPath) that plants a tombstone at the exact slot the sentinel's own probe starts on — good test construction, this is the one case a naive "put a tombstone anywhere" test would miss.
• LRUCache: pre-existing class (already used by CypherStatementCache), confirmed Collections.synchronizedMap(new LRUCache<>(...)) is the same pattern, not a new dependency.
• Bucket disjointness: confirmed vertex keys for a given async bucket task are disjoint from every other concurrently-running bucket (grouping is by srcBucket/dstBucket, and same-vertex edges always land in one contiguous group within one partition pass), so the shared, thread-safe outChunkRIDCache/inChunkRIDCache genuinely never sees concurrent writers for the same key — the per-bucket local-map-then-merge-on-success pattern is sound.
• No System.out left in production code (only in the two @Tag("benchmark")-annotated benchmark harnesses, which is their whole purpose and correctly tagged so they're excluded from normal CI per this repo's convention).
• No fully-qualified names introduced; no new external dependency (only JDK Collections/Set and the existing internal LRUCache).

Residual observations (non-blocking)

1. Collections.synchronizedMap(LRUCache) contention was benchmarked only at 11-way async parallelism (12-core box) with 100k distinct vertices, showing no regression. That's a reasonable proof point, but it doesn't rule out contention on much higher core-count deployments — worth keeping an eye on if this is ever run on a 64+ core box with parallelFlush=true, per the PR's own flagged follow-up.
2. deferredOutHead/deferredInHead/knownNewVertexKeys remain unbounded by distinct-vertex count and are now load-bearing (not just an optimization) for the eviction-fallback correctness fix — this is clearly documented in the class Javadoc as of the latest commit, good.
3. connectIncomingEdgesSequential's undo log (inHeadUndoLog) uses a boxed HashMap<Long, RID> rather than the zero-boxing LongObjectHashMap used elsewhere in this class. That's fine given it's scoped to one commit slice (bounded by commitEvery) on a failure/retry path, not the hot path, but it's a slight inconsistency with the file's stated "prefer arrays of primitives" performance mantra worth a one-line comment if it wasn't intentional.
4. I could not execute mvn compile/mvn test in this review environment (sandboxed, no command approval available), so I verified logic by reading rather than by running the suite. The PR body's claimed 10,680 tests, 0 failures full-module run should be confirmed via the actual CI status before merge rather than taken on faith from the description.

Overall

The core bug (LRU eviction silently orphaning a segment and losing edges) and the two more subtle ones found in later review cycles (partial-drain reprocessing/duplication, and pre-commit cache writes surviving a CME rollback) are real, well-understood, and the fixes match the documented reasoning when checked against the actual DatabaseAsyncTransaction retry semantics. Test coverage is thorough and the regression tests are constructed to actually fail pre-fix rather than being tautological (e.g. the createVertices() vs. plain newVertex() distinction to hit both the known-new and pre-existing branches, and the tombstone-slot-targeting test). Good work tracking down a genuinely subtle set of interacting hazards.

[robfrank]

Retraction: my "correction" about the benchmark claim was itself wrong

In my previous comment I wrote that a review's statement - "the PR backs the 'no regression' claim with an actual before/after benchmark" - was false because no benchmark existed when it was written. That is incorrect, and I'm withdrawing it.

Checked against actual UTC timestamps rather than assumed:

• benchmark commit 8790f89ac pushed 12:26:44Z
• the review making that statement: 12:32:44Z, six minutes later

The review was describing the committed GraphBatchDrainPerfBenchmark / GraphBatchEvictionCostBenchmark accurately - the same review separately notes the benchmark tests "are correctly @Tag(\"benchmark\")-annotated," which only makes sense in reference to those files. So the reviewer was right and I was wrong; nothing about that review needs discounting.

The earlier reviews at 11:56:54Z and 09:11:11Z make no benchmark claim at all, so there was never a false statement to correct in the first place.

Cause, for the record: I relayed that assertion without independently checking the timestamps - the same class of error I flagged two comments earlier when a timezone comparison produced a wrong "no new reviews" conclusion. Apologies for the noise on the thread.

Everything else in that comment stands: the cycle-5 fixes in c31233a02, the benchmark numbers, and the finding that the red integration-tests check is a pre-existing main breakage (still reproducible on f2754aee3, a049d9325, b380e9dca, 81362c771).