fix(vector): rebuild after an admitted one is no longer deferred forever at 10M (#7260) - #9289

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Review

Overall a well-reasoned fix: the swap only happens once the rebuild is admitted, the identity check via `persistedGraphSource` is a safe certification, and `persistedGraphSource` is cleared when a build publishes. Comments below, roughly by importance.

Correctness / risk

1. Twin equivalence rests on ordinals only. The check is `persistedGraphSource == resident` plus `getIdUpperBound()` equality. If the on-heap graph can be mutated in place after the persist (incremental adds, or deletes tracked via the orphan set from LSM_VECTOR: a vector the build left unreachable is served from the delta scan until close(); after reopen it is gone from every search while totalVectors still counts it #7190), the twin is no longer the same graph, and `ordinalToVectorId` and the manifest orphan set could disagree with it. Please confirm nothing mutates `graphIndex` after publish, or clear `persistedGraphSource` on any such mutation path. A test that inserts/deletes after the build and then demotes would pin this.
2. Admitted but never completed. After demotion, if the rebuild then fails, is cancelled, or loses the `REBUILD_SEMAPHORE` race, the index stays on the on-disk graph with `persistedGraphSource == null`, so searches pay page reads indefinitely and nothing promotes it back. Consider restoring the on-heap graph on a failed or aborted rebuild if it is still referenced, or at least logging that the demotion is now sticky.
3. Lifecycle of the replaced on-heap graph and pooled searchers. `releasePooledSearchers()` is called under the write lock; please verify that searchers already checked out against the old `resident` stay valid and are not returned to the pool afterwards (stale searcher on the old graph).
4. Post-swap credit. `creditedHeap = availableHeap + onHeapBytes` assumes the heap reading still counts the on-heap graph. If the reading was taken after a GC that already dropped it (e.g. a previous demotion attempt, or a concurrent publish), the heap is double counted and a build that does not fit gets admitted, which is the OOM case this gate exists to avoid. A comment on why this cannot happen would help.

Minor

• The INFO log's "available heap that X allows" prints `availableHeap / 100 * min(percent, 90)`, but the decision used `availableHeap + onHeapBytes` (credited). The number shown is not the one compared, which will confuse operators. Also `availableHeap / 100 * p` loses precision versus `* p / 100` (overflow is not a realistic concern here).
• `rebuildDeferralStreak++` on a volatile is non-atomic. You note the race is benign, which is fine, but an `AtomicInteger`/`getAndIncrement` costs nothing and removes the comment.
• `forgetPersistedGraphSourceForTest()` adds a test hook to production code. Acceptable given the package-private style used here, just flagging it.
• The `catch (Exception)` path logs at WARNING on every attempt with no streak suppression, so a persistent loadGraph failure would spam the log at every trigger.
• Per CLAUDE.md, prefer a single `@Tag`-free test is fine here, but `withIndex(Consumer)` swallows nothing; nice.

Tests

• Good coverage of the 4 main branches with pinned heap figures. Missing: (a) the race where `graphIndex != resident` at lock time, (b) reclaim of page cache failing (`reclaimReadCacheRAM < needed`) returning false without changing state, (c) the streak logging (WARNING once, FINE after, reset after a successful build), (d) `twin.getIdUpperBound() != resident.getIdUpperBound()` mismatch, (e) an end-to-end check that after demotion the actual rebuild completes and the index is back on an on-heap graph with `residentGraphDemotions == 1`.

Style follows the surrounding code and the `assertThat(...).isTrue()` convention. No new dependencies, nothing security-sensitive.

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

No actionable comments were generated in the recent review. 🎉

sequenceDiagram
  participant LSMVectorIndex
  participant VectorHeapBudget
  participant PersistedGraph
  participant LSMVectorIndexMetrics
  LSMVectorIndex->>VectorHeapBudget: Estimate rebuild with the resident graph
  alt Ordinary estimate does not fit
    LSMVectorIndex->>PersistedGraph: Load the certified on-disk twin
    PersistedGraph-->>LSMVectorIndex: Return the twin and node upper bound
    LSMVectorIndex->>VectorHeapBudget: Estimate rebuild with the twin retained
    alt Revised estimate fits and graph identity is unchanged
      LSMVectorIndex->>LSMVectorIndex: Swap the resident graph under the write lock
      LSMVectorIndex->>LSMVectorIndexMetrics: Increment resident graph demotions
    else Revised estimate does not fit or eligibility fails
      LSMVectorIndex->>LSMVectorIndex: Record a memory deferral
    end
  end

Merge Risk: ⚪ Minimal · up to bedfa

No actionable issue remains from this review; the change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to bedfa

The change includes identity checks and failure cleanup, but a new reference can keep an old graph alive during a rebuild intended to release it first. This can weaken protection against memory exhaustion. No new access or privilege boundary was established.

Retained concerns

• Medium · reliability · inferred: The new persistedGraphSource reference can defeat release-first graph reclamation. Successful persistence stores the built graph in this field, but the releaseResidentGraphFirst branch does not clear it when dropping the resident graph, cache, and pooled searchers. For a certified on-heap generation, the old graph can therefore remain reachable while its replacement is constructed. Clearing certification at later publication is too late to prevent that overlap, weakening the low-memory rebuild path's failure containment and potentially causing memory exhaustion in the shared JVM.

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Review

Overall a well-reasoned fix with good comments and a pinned-heap test overload. A few points:

Correctness / risk

1. Heap credit is optimistic. creditedHeap = min(availableHeap + onHeapBytes, maxHeap) assumes the post-GC reading still counts the on-heap graph. If no GC ran since publish, the credit double-counts and the gate admits a build that may OOM. The comment acknowledges this and relies on the OOM handler plus cooldown as backstop. Consider only crediting when the last GC timestamp is after the graph's publish time, or crediting a fraction.
2. Race window after demotion. After graphIndex = twin, if the admitted rebuild is then not started or fails early (permit or other gate), searches stay on the slower page-read graph until the next trigger. Please confirm that every return true path is followed by an actual build attempt, and that a failed build leaves persistedGraphSource == null while a later re-promotion is not needed.
3. Twin lifecycle. The OnDiskGraphIndex now published as graphIndex must be closed when the build replaces it (and on index close/drop). Please verify the existing publish path closes a previous on-disk graph, since before this PR graphIndex was only an on-disk graph in the storeVectorsInGraph path.
4. twin.getIdUpperBound() != resident.getIdUpperBound() is a weak equivalence check, but persistedGraphSource identity covers the real guarantee. Fine, but worth a comment.
5. reclaimReadCacheRAM is called before the write lock and the identity re-check. If the re-check fails, the cache was reclaimed for nothing. Minor.

Quality

• The INFO log prints creditedHeap * min(percent,90) / 100 as "available heap that X allows", which is misleading since it includes the credit. Same for the "did not fit" wording.
• demotionFailureLogged is never reset, so a failure after a long healthy period is silent (FINE). Consider resetting on a successful demotion.
• forgetPersistedGraphSourceForTest is a test hook in production code; acceptable given the package-private convention here.
• rebuildDeferralStreak is incremented in the deferral path and reset only on a successful build; a demotion-admitted rebuild that fails keeps the streak, which seems intended.
• Per CLAUDE.md, prefer final on locals/params; OnDiskGraphIndex twin is intentionally non-final, fine.

Tests
Issue7260ResidentGraphDemotionTest covers the main path. Please also confirm there are cases for: persist-failed (no certified twin => defer), a build publishing between decision and swap (identity re-check), and a failing loadGraph() (WARNING once, graph untouched). No wall-clock assertions seen, good.

Security / performance: no security concerns. Search latency during demotion is the documented trade-off; the new residentGraphDemotions stat makes it observable.

Nice work; mainly worth checking items 1 and 3.

Review

The approach is sound. Demoting to the certified persisted twin, with identity tracked by persistedGraphSource, is a clean way to break the unbounded-deferral loop. The design is also safe: nothing changes unless the rebuild is admitted, the decision is rechecked under the write lock (graphIndex != resident), and searches keep working. Tests pin the heap figures through a package-private overload, which is a good pattern.

Potential issues

1. lastCollectionEndMillis() may over-credit the resident graph (VectorHeapBudget).

   • It takes the latest getLastGcInfo() end time across all collectors. liveHeapBytes() is built from per-pool post-collection usage.
   • With G1, a young collection after the publish updates the collector's GcInfo, but the old-gen CollectionUsage is only refreshed by a mixed or full or concurrent cycle. A large graph normally lives in old gen.
   • So heapReadingCountsResident can be true while the old-gen reading still predates the graph. The code then adds onHeapBytes on top of a reading that never counted it, and the build is over-admitted. That is the OOM risk this gate exists to prevent.
   • Consider tying the check to the same pool(s) that liveHeapBytes() sums, for example the old-gen/tenured pool's collector. Alternatively, drop the credit unless a full or old-gen collection is known to have ended after the publish. Being conservative costs one more deferral. Over-admitting costs an OOM.

2. twin.close() on the decline path.

   • The comment says published twins are never closed because they hold no resource. If that is true, closing here is harmless.
   • Please verify that OnDiskGraphIndex.close() and gf.loadGraph() do not share a reader or supplier with the graph file, or with an already-published graph. Closing a shared reader would break live searches on a declined path. A test that declines and then searches would pin this.

3. Metadata mismatch check. The only check is getIdUpperBound() equality, and the real guarantee is persistedGraphSource identity. That is fine. But persistedGraphSource = builtGraph is set after writeGraphManifest outside the visible lock scope, while graphIndex could have been replaced by then. The identity comparison in admitByDemotingResidentGraph covers this, so it is OK. A short comment would help.

4. Log text. The existing INFO message was reworded from "deferred" to "not admitted as it stands". Check that no test or operator doc greps for the old wording. The new INFO line also has a very long single string segment, so please wrap it per the surrounding style.

5. Streak counter. rebuildDeferralStreak only resets when a build publishes. If a rebuild is admitted and then fails or aborts, the next deferral logs at FINE even though it is a new problem. This is minor, but resetting on admission would match the "per streak" intent more closely.

Style / minor

• com.sun.management.GarbageCollectorMXBean and GcInfo are used fully qualified, against the CLAUDE.md guideline. The name clash with the java.lang.management import makes this understandable, but add a comment saying so. Also note this is a JDK-specific API, though the LinkageError catch handles it.
• forgetPersistedGraphSourceForTest and demotionFailureLogged are fine. The heapReadingCountsResident javadoc @param alignment is slightly off.

Tests

Coverage of the happy path, the uncertified-twin path and the swap-not-applied path looks good. I would add:

• a test for the stale-reading case (heapReadingCountsResident=false) asserting that deferral still happens when the estimate does not fit without the credit;
• a decline-then-search test for item 2;
• a concurrent-publish race test (graphIndex != resident returns false and leaves state untouched).

Overall: good fix. Item 1 is the one I would resolve before merging.

Review

The approach is sound: demoting to the certified persisted twin, tracked by reference identity, and changing nothing unless the rebuild is admitted. Most of the concerns below are about the heap-credit logic.

Potential bugs

1. VectorHeapBudget.managesTenuredPool can credit the graph when only a young GC ran. With G1, the G1 Young Generation bean lists G1 Old Gen among its memory pools, so managesTenuredPool returns true for it. After any young collection, lastCollectionEndMillis() would then be later than graphPublishedAtMs, and heapReadingCountsResident would be true. That contradicts the Javadoc, which says a young collection does not refresh the old-gen reading. The graph's bytes could then be credited twice, and a build that does not fit could be admitted, which is the OOM case the gate exists to prevent. Consider keying on the collector name (Old, MarkSweep, Concurrent, Mixed) or on whether the old pool's CollectionUsage actually changed. Pool-name substring matching is also fragile across ZGC, Shenandoah, Parallel and Serial. Please add a unit test for it.
2. A demoted index stays on the on-disk graph if the rebuild never runs. After admitOnlineRebuild returns true, the rebuild can still fail to get the JVM-wide permit, or be cancelled or fail. Searches then pay page reads indefinitely, and persistedGraphSource is already null. The log says the next trigger retries, which is fine. Please confirm that a failed or cancelled rebuild after demotion leaves a retryable state, and add a test for that path.
3. The stale-decision check graphIndex != resident is done under the write lock, but loadGraph() and cache reclamation happen before it. If the check fails, the page cache has already been shrunk for nothing. This is minor.

Code quality

• forgetPersistedGraphSourceForTest() is a test hook in production code. Setting the field via reflection or placing the test in the same package would avoid it. It is a style point only.
• The demotion INFO message does a second Math.min(percent, 90) computation that duplicates the logic inside VectorHeapBudget. This can drift, so expose it from there. The format string also has an overlong line.
• In the one-argument admitOnlineRebuild(), getPageManager() is fetched twice (once in the wrapper, once in the body, where it is still used for reclaim). Reuse one reference.
• deferralLevel uses getAndIncrement() == 0, and the streak resets only on a build or a demotion. A streak that spans a long quiet period will stay at FINE. Consider also resetting on a time gap.
• CLAUDE.md prefers imports over fully qualified names. The com.sun.management.GarbageCollectorMXBean one is justified by the name clash, but a comment is already there, so it is fine.

Tests

• The new test class is good, and pinning the heap figures through the overload is a clean idea.
• theRebuildAfterADemotionCompletes... busy-waits with Thread.onSpinWait() against a wall-clock deadline for up to 30s. Use Awaitility or a short sleep to avoid burning a core, and keep the deadline only as a hang detector.
• It also invokes startAsyncGraphRebuild by reflection. A package-private accessor would be sturdier.
• Missing: a test for the young-GC false positive (item 1), for a persist failure leaving no twin (the forgetPersistedGraphSourceForTest path is presumably covered), and for concurrent publish during demotion (the graphIndex != resident branch).
• Per CLAUDE.md, consider whether this vector test belongs in the vector lane, since it waits on a background rebuild.

Performance and security

No security concerns. The page-read search latency during demotion is documented and exposed via the new residentGraphDemotions stat, which is good. The lastCollectionEndMillis() MXBean walk runs on every admission attempt. That is cheap, but it could be computed lazily, only when the first check fails.

Overall this looks good. I would fix item 1 before merging.

Review

The approach is sound. The demotion swaps the resident graph only once the rebuild is certain to be admitted. persistedGraphSource is a reference-identity check, so a stale or failed persist can never vouch for a graph. The write-lock re-check (graphIndex != resident) closes the race with a concurrent publish. The OnDiskGraphIndex is closed on every decline path, and the test seam (pinned heap figures) avoids a 24 GB fixture.

Points to consider

1. Page cache reclaimed before the swap can still be abandoned. In admitByDemotingResidentGraph, reclaimReadCacheRAM(reclaimNeeded) runs before the write lock is taken. If graphIndex != resident then, the method returns false after the cache was already shrunk. This is harmless but wasteful. It could be worth moving the cheap identity check before the reclaim, in addition to the one under the lock.

2. Demotion is sticky if the rebuild then fails. After the swap, persistedGraphSource is null. If the rebuild fails or is cancelled, the index serves from pages until the next successful rebuild. The log message says the next trigger retries, which is right. However, a failing rebuild now means a permanent latency regression, where previously it only meant a deferral. Consider whether residentGraphDemotions should be paired with a visible "serving from disk" indicator, or whether a failed rebuild after demotion should be logged at WARNING.

3. lastCollectionEndMillis heuristics.

   • isTenuredCollector is a name-based blacklist: anything not containing "young/scavenge/copy/parnew/minor" counts as tenured. An unknown future collector name would count as tenured and grant the heap credit, which is the unsafe direction, even though the Javadoc says it is conservative. The -1 is only returned when the info is missing. A whitelist, or at least a note on this, would match the stated intent better.
   • jvmStart + info.getEndTime() is compared to System.currentTimeMillis(). This is fine, but clock adjustments can skew it. The effect is limited to one extra deferral or credit.
   • Fully qualified com.sun.management.* is justified by the comment, but it is a CLAUDE.md deviation. An import alias is not possible in Java, so this is acceptable. Note that com.sun.management is JDK-specific and the LinkageError catch covers it.

4. Formatting. In the INFO log string in admitByDemotingResidentGraph, one line is well over the 160-column limit (... (%d MB on the heap), with about %d MB for the build. Searches read ...). The Javadoc @param alignment is also inconsistent in the new overload, and the new field declarations are misaligned (persistedGraphSource vs rebuildDeferralStreak).

5. Wording change in an existing log. "deferred" became "not admitted as it stands" in the page-cache INFO message. Check that no existing test asserts on that text.

6. Tests. There are new unit tests for the gate and for isTenuredCollector. I could not see an end-to-end test where a demoted index runs a search correctly and then gets its rebuild published. The searcher-pool release and the ordinalToVectorId validity after swap are the riskiest assumptions, so one such test would add real confidence. Also confirm the new test class carries the right @Tag if it is slow.

7. Minor. demotionFailureLogged is never reset on a successful demotion path except at the swap, so the WARNING is once per index until a swap succeeds. That matches the comment. The catch (Exception) also covers RuntimeException from loadGraph, which is desired.

Overall: logic looks correct and well documented. Items 1, 3 and 6 are the ones I would address before merge.

🤖 Generated with Claude Code

Review of #9289 (resident graph demotion for deferred rebuilds)

Solid, carefully reasoned fix. Tracking the persisted twin by reference identity (persistedGraphSource, cleared on publish, set only after the manifest certifies) is a good design. Nothing changes unless the rebuild is admitted, and the tenured-collector whitelist fails safe (one more deferral, never an OOM).

Points to consider

1. Wasted cache reclaim: in admitByDemotingResidentGraph, reclaimReadCacheRAM runs before the write lock. If the graphIndex != resident re-check then fails, the cache was evicted for nothing. Minor; re-checking right before reclaiming would narrow it.
2. Failed rebuild after admission: the index stays on the on-disk twin until some rebuild succeeds, so searches pay a page read per visited node. This is documented. A test for 'admitted, build fails, next trigger retries against the twin' would pin it, if not already covered.
3. Style: com.sun.management.GarbageCollectorMXBean/GcInfo are fully qualified in VectorHeapBudget.lastCollectionEndMillis. The name clash justifies it, so I accept the exception.
4. forgetPersistedGraphSourceForTest and the package-private overload match existing patterns. Keep them package-private.
5. The com.sun.management use is guarded by instanceof and LinkageError, so it degrades to -1 on other JVMs. A cheap test that lastCollectionEndMillis() doesn't throw would be nice.
6. The INFO text changed from 'deferred' to 'not admitted as it stands'. Check that no test or doc greps for the old wording.

Tests: isTenuredCollector is well covered, including null and unknown names. Issue7260ResidentGraphDemotionTest pins heap figures through the overload, so no big fixture is needed.

No security concerns. Performance is fine: the path only runs on an admission miss, and costs one graph load plus a lock acquisition.

Nice work, only minor items above.

Codecov Report

❌ Patch coverage is 73.40426% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.93%. Comparing base (cbf701d) to head (bedfadc).

@@            Coverage Diff            @@
##               main    #9289   +/-   ##
=========================================
  Coverage     76.93%   76.93%           
+ Complexity     3982     3981    -1     
=========================================
  Files          2128     2128           
  Lines        214016   214105   +89     
  Branches      45541    45560   +19     
=========================================
+ Hits         164656   164729   +73     
- Misses        31781    31789    +8     
- Partials      17579    17587    +8     

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