fix(engine) #5615: keep vectors the graph build leaves unreachable searchable via the delta scan

[robfrank]

Closes #5615

Root cause

A graph rebuild occasionally emits an orphan node: present in the graph and carrying a full set of outgoing edges, but with zero incoming edges. Beam search starts at the entry node and only ever follows edges forward, so an orphan can never be visited - at any efSearch, however good its score. The vector, the location index, the ordinal map and the scoring path are all correct, which is why the symptom looked like a delta-retention or publication race for so long and why nine earlier theories all failed.

Instrumenting the reproducer to walk the graph at the moment of a miss:

batch 15: #1:8382 -> #1:6156
  retryHit=false wideEfHit=false
  rankProbe: expectedOrdinal=1500 expectedScore=1.0 betterScoring=0 bestOrdinal=1500 bestScore=1.0
             graphSize=1600 mapLen=1600
  reach:     entryNode=1050 visited=1599 ofUpperBound=1600
             targetReachable=false targetInGraph=true targetOutDegree=32 targetInEdges=0

The missing vector is the globally best-scoring node (betterScoring=0), graph and ordinal map agree on size, and a BFS from the entry node reaches 1599 of 1600 nodes - every one except this. retryHit=false and wideEfHit=false (efSearch=2000 against a 1600-node graph) show the miss is stable rather than a transient read-path race.

A second probe placed directly after builder.build(vectors) closed the chain: on a failing run the build reported ORPHANS count=3 sample=252(deg=32) 257(deg=32) 258(deg=32) and the search then missed ordinal 258, one of those three. Passing runs reported zero orphan builds.

GraphIndexBuilder.build() does call cleanup() (verified in the 4.0.0-rc.7 bytecode: submit(addNodes) -> join -> cleanup), and cleanup has connectivity machinery. It nonetheless leaves these nodes unreferenced under concurrent rebuild pressure.

The change

findUnreachableOrdinals walks every edge from the graph entry node after a build and returns the ordinals nothing reaches. buildGraphFromScratchExclusively re-queues those vectors into the delta buffer, where mergeWithDeltaScan - an exhaustive linear scan - keeps them searchable until the next rebuild wires them into the graph.

• The walk and the vector reads run before the write lock is reacquired, so an O(V+E) traversal never stalls concurrent searches.
• Vectors at or past deltaSnapshotId are skipped: the existing trim already carries them over, and re-adding them would score the same vector twice in the delta scan.
• Reachability is walked from the entry node rather than counted as in-degree, because a disconnected cycle gives every node an in-edge while remaining unreachable.

This is a safety net at the ArcadeDB layer that restores the "every committed vector is findable" guarantee. It does not address why the builder produces the orphan, which is worth reporting upstream.

Test plan

• LSMVectorIndexGraphConnectivityTest - 5 deterministic cases against graphs whose shape is fixed by the test: a connected chain, a node with out-edges and no in-edges, several orphans at once, a disconnected cycle, and an empty graph. Stubbing findUnreachableOrdinals to a no-op fails 3 of the 5, confirming they bind.
• LSMVectorIndexConcurrentRebuildVisibilityTest - the end-to-end reproducer, which on a miss now reports whether the vector is reachable from the entry node at all.
• mvn -pl engine test -Dtest='com.arcadedb.index.vector.*Test' - 245 tests, 0 failures, 0 errors.

Note the concurrency reproducer is inherently probabilistic: its flake rate on unfixed code swung between 1 run in 1 and 1 run in 10, so a single clean run of it proves little on its own. The deterministic test is what pins the fix.

[mergify]

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

• Queue this pull request

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

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: fix(engine) #5615 - keep graph-unreachable vectors searchable via the delta scan

Reviewed LSMVectorIndex.java, the two new tests, and the delta/search interaction (mergeWithDeltaScan, rebuildGraphBeforeSearch, ArcadePageVectorValues.getVector). Well-reasoned, well-documented change; the design is sound and the "theories eliminated" writeup is excellent.

A few points worth considering, none blocking.

Correctness / design - looks right

• Walking reachability from the entry node (rather than counting in-degree) is the correct call, and the aDisconnectedCycleIsUnreachableEvenThoughEveryNodeHasIncomingEdges test pins exactly why.
• Running the O(V+E) walk and vector reads before reacquiring the write lock keeps concurrent searches unblocked.
• Skipping vectorId >= deltaSnapshotId to avoid double-scoring the trim carryover is correct, and the fix is self-cleaning: on the next rebuild these old vectorIds fall below the new snapshot and are trimmed out of the delta, so no duplication accumulates.
• graphState goes MUTABLE when only unreachable entries are re-queued, and since mergeWithDeltaScan gates on delta emptiness (not on state) and the search path always calls it after the graph pass, the vectors stay findable.

Points to consider

1. vector != null does not exclude the deleted sentinel. ArcadePageVectorValues.getVector() returns deletedSentinelVector (non-null) on ~8 paths; only loc == null || loc.deleted is pre-filtered here. On a non-deleted read-error / out-of-bounds path getVector(ordinal) can hand back the sentinel while loc looks healthy, so a delta entry pairing a real RID with a sentinel vector could be queued and later scored (right RID, bogus distance). It mirrors the build loop at ~line 1965 so it is parity, not a regression, but a vector != deletedSentinelVector guard would make the safety net strictly safe. Minor.

2. The walk runs on every rebuild, including the common no-orphan case. findUnreachableOrdinals allocates boolean[upper] + int[upper] and does a full BFS each rebuild. For a large, frequently-rebuilt index that is a couple of transient MB and an O(V+E) pass paid unconditionally. Off the write lock and bounded, so likely fine - worth a note in the PR; the arrays could be pooled/reused if it ever matters.

3. An orphan on the last rebuild of an otherwise-idle index lingers in the delta indefinitely. The re-queue does not bump mutationsSinceSerialize, so rebuildGraphBeforeSearch (line 2525: mutationsSinceSerialize.get() <= 0 -> return) will not trigger a follow-up rebuild to wire it back in. This is the intended "no rebuild spin" behavior and the safe choice, but searches carry the extra delta entry until the next real mutation. A one-line comment noting this is deliberate would help the next reader.

4. Scope: only the from-scratch rebuild path is covered. If the live/incremental builder path can also emit an orphan it is not caught here. Probably out of scope, but worth confirming the reproducer only exercises buildGraphFromScratchExclusively.

Tests

• @Tag("slow") on the reproducer and leaving the deterministic connectivity test untagged both match the CLAUDE.md guidance.
• FixedGraph stubbing only the members the walk touches (throwing on the rest) keeps the test honest.
• Stubbing findUnreachableOrdinals to a no-op and confirming 3/5 cases fail is a solid way to prove the assertions bind.
• Nit: the connectivity test reuses one Probe index across all cases, so the real index is just a vehicle for reaching the package-private method - fine, the DB setup is incidental to what is tested.

Overall: solid fix, strong diagnostics, good coverage. My only genuine ask is a quick look at point (1); the rest are notes.

[claude]

Review: fix(engine) issue 5615 - keep graph-unreachable vectors searchable via the delta scan

Overall this is a strong, well-reasoned PR. The root-cause writeup is excellent, the fix sits at the right layer (an ArcadeDB-side safety net that restores the "every committed vector is findable" guarantee without pretending to fix the upstream JVector builder), and the design choices are sound:

• The walk and vector reads run off the write lock, so the O(V+E) traversal never stalls concurrent searches. Good.
• Not bumping mutationsSinceSerialize for re-queued vectors is the right call: counting them would let an orphaned index rebuild forever, and the next rebuild trim (vectorId < deltaSnapshotId drops it, then re-detects if still orphaned) means no unbounded delta growth.
• Walking reachability from the entry node rather than counting in-degree is the correct primitive; the disconnected-cycle test case pins exactly why.
• Rejecting the sentinel before re-queueing avoids pairing a real RID with a meaningless distance in the delta scan. mergeWithDeltaScan also re-checks loc.deleted and dedupes by RID, so a re-queued-but-actually-reachable vector is harmless.

A few things worth considering:

1. Exception-driven control flow in the level loop (performance) - findUnreachableOrdinals, ~line 1427. The BFS iterates every level 0..maxLevel for every visited node and relies on catch (Exception) for the "node absent at this level" case, which the comment itself calls "normal in a hierarchical graph." In HNSW almost every node lives only at level 0, so on a large hierarchical graph this throws and catches on the order of V * maxLevel exceptions per rebuild, each paying JVM stack-trace fill-in - exactly the kind of hot-path cost the CLAUDE.md performance mantra warns against. The View interface exposes contains(int level, int node) (your own FixedGraph test mock implements it), so guarding with if (!view.contains(level, node)) continue; before the iterator call avoids the exceptions entirely. Worth confirming getNeighborsIterator actually throws (vs. returning empty) for an absent node; either way the guard is cheaper and clearer than try/catch in the inner loop.

2. Transient boolean[upper] + int[upper] on every rebuild (performance). Both arrays are allocated on every rebuild including the common zero-orphan case. For a multi-million-vector index rebuilt frequently, that is tens of MB of transient allocation per rebuild. The doc acknowledges this and argues the build dominates, which is fair; just flagging it since GC pressure is a stated project concern. A pooled/reused buffer is a possible future optimization if profiling ever points here.

3. view.entryNode() null-safety (robustness) - line 1418. view.entryNode().node will NPE if entryNode() returns null for a non-empty graph. That NPE is caught by the outer catch (Exception) and degrades to "log a WARNING, skip orphan detection this rebuild," which is acceptable, but an explicit null check would distinguish "graph has no entry" from a genuine failure and keep the warning meaningful.

4. Reachability model note (minor). The walk unions edges across all levels, whereas beam search descends top-to-0 then beam-searches level 0. Union-reachability is a superset of beam-reachable, so this can never produce a false orphan (safe direction), but in principle a node reachable only via a higher-level in-edge could still be beam-unreachable and go undetected. Given the observed defect is in-degree-0 at all levels this is fine; a one-line comment that this detects graph-unreachable rather than strictly beam-unreachable would set expectations.

Tests. Good coverage. The deterministic LSMVectorIndexGraphConnectivityTest cases (chain, out-no-in, multiple orphans, disconnected cycle, empty, sentinel) are the right shapes, and the FixedGraph mock that throws on untouched members is a nice guard. @Tag("slow") on the probabilistic reproducer follows the repo convention, and the honest note that a single clean reproducer run proves little is appreciated. The reproducer leans heavily on reflection into private fields, which is brittle to refactors, but it only runs on a miss as a diagnostic, so that is a reasonable trade.

Nits. The vectors instanceof ArcadePageVectorValues check at the re-queue site is always true here (vectors = pageVectors), so it is purely defensive - fine, just noting. Also, docs/5615-...md largely duplicates the PR description; if the repo does not usually keep per-issue design docs in docs/, consider whether it belongs in the tree.

None of these are blockers. Item 1 is the one I would most want addressed before merge, since it runs on every rebuild on the hot path.

[robfrank]

Thanks - addressed 3 of the 4 in b1eb6e4. Pushing back on item 1, which rests on an incorrect premise.

1. Exception-driven control flow - the premise does not hold, so there is no exception storm.

getNeighborsIterator does not throw for an absent node. From the 4.0.0-rc.7 bytecode of OnHeapGraphIndex.getNeighborsIterator(int, int):

 0: iload_1
 2: getfield      layers:Ljava/util/List;
 5: invokeinterface List.size:()I
10: if_icmplt     17
13: getstatic     NodesIterator.EMPTY_NODE_ITERATOR    <-- level out of range
16: areturn
...
31: invokevirtual ConcurrentNeighborMap.get:(I)...Neighbors;
35: aload_3
36: ifnonnull     43
39: getstatic     NodesIterator.EMPTY_NODE_ITERATOR    <-- node absent from this level
42: areturn

Both the out-of-range level and the unmapped node return the shared EMPTY_NODE_ITERATOR. The catch never fires on this path, so no stack traces are filled and there is no V * maxLevel cost. The graph a rebuild publishes is exactly what builder.build() returns, i.e. an OnHeapGraphIndex.

Guarding with view.contains(level, node) would also not be cheaper: contains performs the same layer lookup plus neighbor-map lookup that getNeighborsIterator already does, so it would add a call rather than remove work.

What was genuinely wrong was my comment, which asserted absence was signalled by an exception and is what pointed you at this. Corrected to state that absence yields an empty iterator and that the catch is purely defensive for View implementations that choose to throw.

3. entryNode() null-safety - fixed. Explicit null check with its own WARNING, so "graph has no entry node" is distinguishable from a genuine failure.

4. Reachability model - documented on the method. The walk unions all levels, which is a superset of beam-reachable, so it can never invent a false orphan; a node reachable only via a higher-level in-edge is not reported, which suits the observed defect (no in-edges at any level). Noted that conservative is the right direction here, since a false orphan costs a duplicate delta entry on every search.

2. Transient arrays - agreed and left as-is; noted in the tracking doc as a future optimization if profiling points here.

Nits: the instanceof is indeed always true today and deliberately defensive. On docs/ - per-issue docs are the convention in this repo, so it stays.

mvn -pl engine test -Dtest='com.arcadedb.index.vector.*Test' - 246 tests, 0 failures.

[claude]

Review of PR 5633 - keep unreachable vectors searchable via the delta scan

Reviewed the engine change (LSMVectorIndex, ArcadePageVectorValues), both tests, and the surrounding rebuild/delta machinery. This is a high-quality, unusually well-diagnosed PR. The root-cause writeup and the theories-eliminated section are exemplary, and the design choices are sound.

What is good

• Correct core insight. Walking reachability from the entry node (rather than counting in-degree) is the right primitive - the disconnected-cycle test case proves an in-degree count would miss it. The union-over-levels walk being a superset of what beam search can reach is the right conservative bias: it can never invent a false orphan, only miss one, and a false orphan would only cost a duplicate delta entry.
• Off-lock traversal. Doing the O(V+E) walk and the vector reads before reacquiring the write lock keeps concurrent searches unaffected. Good instinct.
• Self-healing and bounded. A re-queued orphan (vectorId < snapshot) is trimmed from remaining on the next rebuild but picked up fresh from the pages, re-detected if still orphaned, so there is no unbounded delta growth and no leak.
• Sentinel handling. Rejecting deletedSentinelVector (reference equality against a per-instance sentinel) is correct, since getVector() never returns null. The live loc.deleted check ahead of it also correctly filters a vector superseded by a concurrent update during the build, which incidentally guards against pairing a stale vector with a live RID in the delta scan.
• Tests bind the behavior. Deterministic FixedGraph cases (chain, orphan, multi-orphan, disconnected cycle, empty) plus the sentinel-distinguishability case, and the mutation-testing note (stubbing to no-op fails 3 of 6) is exactly the right way to demonstrate the assertions actually pin the fix. The @tag(slow) on the probabilistic reproducer matches the CLAUDE.md convention.

Points worth considering (none blocking)

1. Restart gap. The graph persisted after this block still physically contains the orphan node (unreachable), and deltaVectors is volatile and lost on restart. So immediately after a restart, before the next rebuild fires, the orphan is once again unsearchable - the safety net only covers the live session. Given the builder bug is rare and any rebuild re-detects it, this is acceptable, but it is a residual hole in the every-committed-vector-is-findable guarantee that is worth a sentence in the doc.

2. graphState stays MUTABLE with the inactivity timer possibly cancelled. When only unreachable entries remain, remaining is non-empty so state stays MUTABLE, but mutationsSinceSerialize was just decremented and may be <= 0, cancelling the inactivity rebuild timer. On an otherwise idle index that means the delta scan runs on every search with no scheduled rebuild to clear it until the next real mutation. The inline comment acknowledges the scan cost, but not that there is no self-scheduled path out of it for an idle index. Worth confirming that is the intended tradeoff (it appears to be, to avoid the rebuild-forever loop).

3. getNeighborsIterator returning null vs throwing. The inner try only guards the call; if some View implementation returned null (rather than the documented empty iterator), the neighbors.hasNext() NPE would escape to the outer catch and abort the entire walk, treating the whole graph as fully reachable (silent no-op). For OnHeapGraphIndex this cannot happen, and it is documented, so low risk - just flagging that the failure mode is silently-skip-the-check rather than skip-one-node.

4. Micro: the count-then-fill loop calls graph.containsNode(node) twice per node across two passes. Trivial, and it keeps the primitive-array allocation exact, so fine as-is.

Verification

Followed the reasoning through mergeWithDeltaScan (RID dedup against graph results + loc.deleted recheck), the deltaSnapshotId skip (prevents double-scoring against the trim), and the ordinal-to-finalActiveVectorIds mapping (bounds-guarded). All consistent. The provided/test scope rules and the no-new-dependency rule are not touched. LGTM once the restart-gap limitation is noted somewhere durable.

[robfrank]

Addressed in HEAD. Confirmed item 1 against the code before writing it up.

1. Restart gap - real, and narrower than "the delta is lost". Verified the exact mechanism: on load, LSMVectorIndex treats a persisted graph as stale only when graphSize < ordinalMap.length. An orphan is physically in the graph, so the node count matches the ordinal map, the graph reads as up to date, and no rebuild is triggered. Combined with deltaVectors being in-memory, the vector is unsearchable again after a restart until some mutation causes a rebuild. Documented in both the method comment and a new "Known limitations" section. Closing it would mean persisting the orphan set alongside the graph, which is out of scope here given any rebuild re-detects it.

2. Idle index / inactivity timer - confirmed intended. You are right that the decrement can take mutationsSinceSerialize to zero and cancel the inactivity timer, leaving no self-scheduled path out of the delta scan. That is the accepted price of not bumping the counter, which would let an orphaned index rebuild itself forever. The comment now says both halves rather than only the scan cost.

3. Null iterator - guarded. Added an explicit if (neighbors == null) continue;. You correctly identified the asymmetry: an NPE there would unwind to the outer catch and abandon the whole walk, treating every remaining node as reachable. Now it skips one node instead.

4. Double containsNode - left as-is; it keeps the array allocation exact.

mvn -pl engine test -Dtest='com.arcadedb.index.vector.*Test' - 246 tests, 0 failures.

[claude]

Review: keep graph-build orphans searchable via the delta scan (issue 5615)

Thorough, well-diagnosed fix. The root-cause writeup and the "theories eliminated" section make this reviewable in a way most concurrency bugs never are, and the mechanism (walk-from-entry, re-queue into deltaVectors, served by mergeWithDeltaScan) is a clean layer-appropriate safety net that does not touch the JVector builder itself. I traced the search path and the delta-scan semantics; the fix is sound. Notes below, mostly minor.

Correctness - verified good

• No double-counting / no resurrection. An orphan is unreachable in the graph, so graph search never returns it; the delta scan is the only source of the hit. mergeWithDeltaScan also dedupes by RID (seenRIDs) and re-checks loc.deleted at scan time (LSMVectorIndex.java:3354), so the off-lock loc.deleted check in the collection loop has no TOCTOU exposure - a vector deleted after collection is dropped at scan time.
• No snapshot double-scoring. Skipping vectorId >= deltaSnapshotId is correct: the trim keeps those entries anyway, and there is no overlap with remaining (which keeps only >= deltaSnapshotId), so a re-queued orphan is never present twice.
• State transition. Adding unreachableEntries to remaining forces graphState = MUTABLE, and mergeWithDeltaScan runs unconditionally after graph search (:3621), so the re-queued entries are actually reached by searches. Good.
• Null entry node, empty graph, out-of-range entry, and the sentinel path are all handled and pinned by tests.

Points worth surfacing (mostly already documented)

1. The "every committed vector is findable" guarantee is restored only until restart. deltaVectors is in-memory; after a restart the persisted graph still physically contains the orphan and passes the graphSize == ordinalMap.length staleness check, so the vector is unsearchable again until some mutation triggers a rebuild. This is documented as a known limitation, which is the right call, but it means the guarantee is slightly stronger than what is delivered. Consider softening the framing to "until restart or the next rebuild."

2. Idle orphaned index never self-heals. Because re-queueing deliberately does not bump mutationsSinceSerialize, and rebuildGraphBeforeSearch early-returns when mutationsSinceSerialize <= 0 (:2565), an idle index pays the delta-scan cost on every search indefinitely, with no self-scheduled path out. Documented and accepted. For a build that orphans many nodes this is a persistent per-query latency add, not a one-off; if orphan counts are ever observed to be large in practice, a bounded self-heal (e.g. one deferred rebuild scheduled when unreachableEntries is non-empty) would be worth revisiting.

3. Union-reachability is conservative in one direction only. Walking the union of all levels edges guarantees no false orphans (union-reachable is a superset of search-reachable, so anything reported is genuinely search-unreachable). But it can miss a true orphan whose only in-edge exists at a higher level - such a node is union-reachable yet may not be search-reachable. That matches the observed defect (zero in-edges at any level), and the doc says so, so this is acceptable; just flagging that the detector is not complete for all orphan shapes.

4. Per-rebuild cost. findUnreachableOrdinals runs on every rebuild with two O(upper) allocations and an O(V*L + E) walk (the per-node level loop calls getNeighborsIterator for every level up to maxLevel, even for nodes absent above level 0). Off the write lock and dominated by the build itself, so fine.

Nits

• findUnreachableOrdinals scans [0, upper) twice (count, then fill). Minor; avoids a growable buffer, so arguably the right tradeoff for GC given the CLAUDE.md primitive-array preference.
• The broad catch (Exception) returning EMPTY_ORDINALS is fail-open (never breaks a rebuild), which is the correct posture for a safety net; good that it logs at WARNING.

Tests
Good coverage. LSMVectorIndexGraphConnectivityTest pins the detection primitive deterministically (chain, out-but-no-in, multiple orphans, disconnected cycle proving reachability beats in-degree counting, empty graph, sentinel distinguishability), and the FixedGraph record throwing UnsupportedOperationException on untouched methods is a nice way to keep the stub honest. The concurrency reproducer is correctly tagged slow per CLAUDE.md, and the PR is honest that it proves little on its own - the deterministic test is what binds the fix. Style matches the repo (final params, assertThat(...).isTrue()).

Overall: solid, safe, and unusually well-documented. My only substantive ask is to make sure the restart gap and the idle-scan cost are visible to users of the guarantee (they are in the doc/comments; consider echoing in the issue/upstream report). Not blocking.

[claude]

Review: PR #5633 - keep graph-orphaned vectors searchable via the delta scan

Reviewed the full diff, the surrounding buildGraphFromScratchExclusively / mergeWithDeltaScan code, and the two new tests. This is a high-quality, well-reasoned fix. The root-cause writeup, the "theories eliminated" section, and the review-cycle log make it easy to follow. Findings below, most substantive first; none are blockers.

Correctness - the fix hangs together. Traced the integration and it is internally consistent:

• finalActiveVectorIds is the exact ordinal->vectorId map the build ran over (pageVectors is constructed with it), so finalActiveVectorIds[ordinal] and vectors.getVector(ordinal) both key off the same ordinal space, so there is no mismatch.
• findUnreachableOrdinals runs on the freshly built, not-yet-published builtGraph before the write lock, so the O(V+E) walk touches no shared state and cannot stall searches. Good.
• Setting graphState = MUTABLE when remaining is non-empty, plus the fact that mergeWithDeltaScan snapshots deltaVectors unconditionally, means the re-queued orphans stay searchable even though mutationsSinceSerialize is at 0 and no rebuild fires. The < deltaSnapshotId filter cleanly avoids double-counting against the concurrent-insert trim. Nicely threaded.
• mergeWithDeltaScan dedups orphans against graph results by RID and re-checks loc.deleted live, so a stale re-queued entry for a since-deleted vector is dropped at search time. This also mitigates the TOCTOU window between the pre-lock loc.deleted check and the write-lock reacquire.

Test coverage - one gap worth noting. The deterministic LSMVectorIndexGraphConnectivityTest pins findUnreachableOrdinals well (chain, orphan, multi-orphan, disconnected cycle, empty, sentinel), and the FixedGraph "throw on anything unexpected" stub is a nice touch. But the re-queue block itself (the < deltaSnapshotId skip, the loc == null || loc.deleted skip, the sentinel rejection, and remaining.addAll(unreachableEntries)) is only exercised by the probabilistic @Tag("slow") concurrency reproducer, which, as the PR itself notes, proves little on a single clean run. Consider a small deterministic test that injects a known-orphan graph into buildGraphFromScratchExclusively (or factors the collect-unreachable-entries loop into a testable helper) and asserts the orphan is served from the delta scan afterward. That would pin the half of the fix the deterministic suite currently cannot reach.

Performance - transient allocation on every rebuild. findUnreachableOrdinals allocates boolean[upper] + int[upper] and does a full V+E walk on every rebuild, including the common zero-orphan case. On a multi-million-vector index that is tens of MB of transient garbage per rebuild (the int[upper] queue dominates). It is off the hot path and dwarfed by the build it follows, and the PR documents the tradeoff, so this is acceptable, but given the repo GC-pressure mantra it is worth a follow-up thought: the queue could be bounded/reused, or the whole walk gated behind a cheap size heuristic if profiling ever flags it. Minor: the reachable-node set is scanned twice (count, then fill); a single pass into an IntArrayList-style buffer would halve it, though at upper scale it is negligible.

Minor notes.

• findUnreachableOrdinals fails open: any exception logs a WARNING and returns EMPTY_ORDINALS, so a check failure silently forgoes orphan recovery rather than breaking the rebuild. That is the right call for a safety net, just flagging it explicitly.
• Same-RID duplication in deltaVectors: if a doc is re-inserted during the build, the orphan (old vectorId, < snapshot) and the new insert (>= snapshot) can both live in deltaVectors. mergeWithDeltaScan does not dedup delta entries against each other (only against graph results), so this relies entirely on the old vectorId location being marked deleted to suppress the stale one. That holds today, but it is a pre-existing sharp edge this fix makes marginally easier to hit, worth keeping in mind.
• The vectors instanceof ArcadePageVectorValues guard is effectively always true here (it is the concrete type built above), but the defensive fallback is harmless.

Docs. docs/5615-...md is excellent: the eliminated-theories list, the known-limitations (restart gap, idle-index scan), and the scoped guarantee are exactly the context a future maintainer needs. The separately-noted GraphSearcherPool.borrow drain-before-publish race and the upstream GraphIndexBuilder.cleanup() question are good follow-ups to file.

Overall: sound fix, correctly scoped as an ArcadeDB-layer safety net, thoroughly documented. Main ask is a deterministic test over the re-queue path; the allocation note is a nice-to-have.

[codecov]

Codecov Report

❌ Patch coverage is 64.28571% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.49%. Comparing base (8a4cf19) to head (84fde7a).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...java/com/arcadedb/index/vector/LSMVectorIndex.java	63.76%	13 Missing and 12 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5633      +/-   ##
============================================
- Coverage     67.20%   66.49%   -0.71%     
- Complexity        0     1115    +1115     
============================================
  Files          1771     1771              
  Lines        149642   149745     +103     
  Branches      31738    31764      +26     
============================================
- Hits         100560    99576     -984     
- Misses        35942    37172    +1230     
+ Partials      13140    12997     -143     

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

Filed the searcher-pool race found during this investigation as #5648 - GraphSearcherPool.borrow drains the idle queue before publishing pooledGraph/pooledEpoch, so a concurrent release can re-pool a searcher bound to the replaced graph. Separate mechanism from this PR (it needs two searching threads on one index, which the #5615 reproducer does not have), but it produces the same silent wrong-result symptom. Linked from the tracking doc.

[claude]

Review of #5633 - keep graph-build-orphaned vectors searchable via the delta scan

I reviewed the change (LSMVectorIndex.findUnreachableOrdinals + the re-queue path in buildGraphFromScratchExclusively, the ArcadePageVectorValues.isDeletedSentinel helper, and both new tests). This is a high-quality, unusually well-documented fix. Notes below are mostly confirmations, with a couple of small things worth tracking.

What is solid

• Correct diagnosis and a genuinely safe recovery. Walking reachability from the entry node (rather than counting in-degree) is the right call, and the disconnected-cycle test case (aDisconnectedCycleIsUnreachableEvenThoughEveryNodeHasIncomingEdges) proves exactly why in-degree would be wrong.
• No false-positive harm. Even if the walk under-reports reachability (e.g. a per-level getNeighborsIterator throws and the loop continues), a wrongly-flagged reachable vector only costs a spare delta entry - mergeWithDeltaScan dedups by RID via seenRIDs, so it can never double-score or return a wrong distance. Any error leans toward waste, not incorrectness.
• Duplicate-scoring avoided at the source too. The vectorId >= deltaSnapshotId skip plus the snapshot-trim in remaining guarantees an unreachable vector (always vectorId < snapshot) never overlaps the carried-over concurrent entries.
• Sentinel handling is right. Re-queueing goes through getVector(ordinal) (build snapshot) and rejects isDeletedSentinel, so an unreadable ordinal never pairs a real RID with a meaningless distance. Reference-equality on the per-instance sentinel is correct here, and theSentinelIsDistinguishableFromARealVector pins it.
• Bounds are safe. queue = new int[upper] with each node enqueued at most once (guarded by reached[]) means tail can never exceed upper.
• Tests are well-targeted and correctly tagged - deterministic shape-fixed cases for the primitive, the probabilistic reproducer marked @Tag("slow") so it stays out of regular CI. The note that stubbing findUnreachableOrdinals to a no-op fails 3/5 is a nice binding check.

Things worth keeping on the radar (all acknowledged in the code/PR body; flagging for visibility)

1. The guarantee does not survive a restart. As documented at LSMVectorIndex.java:2091-2094, deltaVectors is in-memory; after a restart the persisted graph still physically holds the orphan and reports a matching node count, so the staleness check sees an up-to-date graph and the vector is unsearchable again until some mutation triggers a rebuild. This is a real narrowing of the "every committed vector is findable" guarantee for an index that is orphaned and then idle across a restart. Accepted here, but it is the one spot where the safety net has a hole; persisting the orphan set (or forcing one rebuild on load when orphans were detected pre-shutdown) would close it.
2. Idle indexes pay a permanent linear-scan tax. Not bumping mutationsSinceSerialize (correctly, to avoid rebuild spin) plus the timer cancel means orphans sit in the delta buffer, and every search then pays an O(N-orphans) scan with no self-scheduled way out until the next real mutation. Fine for the observed 1-3 orphans; a metric would make a pathological build that orphans many nodes visible rather than silently slow.
3. Per-(node, level) iterator allocation on hierarchical graphs. The union-across-levels walk creates a NodesIterator for every level of every node, including levels where the node is absent (empty iterator). On a large hierarchical index that is ~V * maxLevel short-lived allocations per rebuild. Since the observed defect is a node with no in-edges at any level, a view.contains(level, node) guard before getNeighborsIterator would cut most of that with no loss of conservativeness. Minor - the build dominates - but cheap.

Upstream

Agree the root cause is in the JVector GraphIndexBuilder.cleanup() leaving nodes unreferenced under concurrent rebuild pressure, and that this is an ArcadeDB-layer safety net rather than a real fix. Worth the upstream report noted.

Overall: clear root-cause analysis, a fail-safe implementation, and tests that actually pin the behavior. Nice work.

Automated review - the PR is already merged; posting for the record.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 81.43% diff coverage · -7.51% coverage variation

Metric	Results
Coverage variation	✅ -7.51% coverage variation
Diff coverage	✅ 81.43% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (8a4cf19)	149642	113740	76.01%
Head commit (84fde7a)	181737 (+32095)	124491 (+10751)	68.50% (-7.51%)

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 (#5633)	70	57	81.43%

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