fix(vector): memory-aware WARM offload with a real, reloadable ceiling - #252
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
TinDang97
force-pushed
the
feat/vector-offload-reload-pool
branch
2 times, most recently
from
July 9, 2026 18:46
4c318ca to
1a8820f
Compare
pilotspacex-byte
changed the base branch from
feat/vector-offload-reload-pool
to
main
July 9, 2026 19:08
MmapBudget::enforce_budget (the per-shard --vec-warm-mmap-budget byte cap, default 2gb) evicted a WARM segment by dropping its Arc outright with no COLD stub — so once the byte cap evicted a segment it silently vanished from the searchable set until process restart (recall loss), despite the module doc claiming "the next search reloads it transparently." The reload path did not exist for this eviction. Fix: convert each evicted WARM segment to a reloadable UnloadedSegment stub pushed into SegmentList.unloaded, mirroring the idle-unload HOT/WARM->COLD path. Every WARM segment is durably disk-backed — transition_to_warm writes its .mpf files before WarmSearchSegment::from_files ever loads it, and every warm-creation path (HOT->WARM transition, boot recovery, COLD reload) goes through from_files — so from_warm captures a stub (segment_id + cloned SegmentHandle keeping the dir alive + tombstones) that reloads byte- identically on next touch via promote_unloaded/submit_unloaded_reloads. No data loss: the heap copy is freed when the old Arc drops, the disk files stay. enforce_budget and the reload/install path both run on the shard event-loop thread, so the read-modify-swap stays race-free. This turns --vec-warm-mmap-budget into a real, reloadable memory ceiling on the WARM vector tier (prerequisite for the reload-admission gate and memory-triggered offload that follow). Test: test_eviction_demotes_to_reloadable_stub — after enforce_budget the evicted segments are present in `unloaded` (count == segments_evicted), the MRU survives in `warm`, and a stub reloads Ok from its on-disk files. author: Tin Dang
… cap The COLD->WARM reload/install path (promote_unloaded + the #18 off-loop submit_unloaded_reloads) grew the WARM tier with no memory bound: a query burst first-touching many COLD segments re-materialized all of them into resident heap, and nothing capped the aggregate until the 10s MmapBudget tick — an unbounded transient overshoot that could OOM before the tick ran. Vector WARM memory is outside --maxmemory/the elastic budget/the diskfull guard, so no existing ceiling saw this growth. Add an install-time gate: after a reload installs freshly-hot segments, evict least-recently-accessed idle WARM segments — converting each to a reloadable COLD stub (fix A) — until the tier's resident bytes fit the per-shard cap. The segments this query just reloaded are protected (never evicted; a single query's working set is the full-recall floor). Result: live WARM stays <= --vec-warm-mmap-budget between ticks, idle segments make room for active ones (proper LRU cache), and everything stays reloadable. The per-shard cap is mirrored into a process-global atomic (reload_pool::set_warm_budget_per_shard, set at startup from --vec-warm-mmap-budget) so the holder path needs no plumbing of the shard's !Send MmapBudget; the tick remains the authoritative per-shard enforcer, this is the immediate bound. cap==0 disables the gate. With multiple vector indexes on one shard each holder gates to the full per-shard cap (loose upper bound), reconciled by the tick. Tests: evict_warm_lru_to_budget demotes idle warm to reloadable stubs to fit the cap; never evicts the protected (just-reloaded) segment even when it alone exceeds the cap; cap==0 no-ops. author: Tin Dang
The RSS-freeing vector offload (HOT/WARM->COLD) was purely time-based (--engine-offload-idle-secs, default 3600s) with zero memory input, and the one memory-pressure mechanism that existed had two holes: 1. Its trigger (should_run_pressure_cascade) measured KV memory only — vector segment memory was invisible to it, so a vector-heavy workload (the primary disk-offload use case, where KV is light) never fired the cascade regardless of how much RAM the vector tier held. 2. Under pressure, cascade step 2 demoted HOT->WARM, which frees NO resident bytes (a WarmSearchSegment is a same-size heap copy of the HOT segment). Fix both: - Trigger (C2): thread the shard's vector resident bytes (HOT immutable + WARM, already computed this same tick for INFO/Prometheus) into should_run_pressure_cascade, added to the KV total before the disk-offload-threshold comparison. Vector-driven pressure now fires the cascade. - Response (C1): cascade step 2 now offloads idle vector segments straight to COLD (UnloadedSegment stub — actually returns RAM, stays reloadable) via try_warm_transitions_all_idle with warm_after = u64::MAX (no HOT->WARM) and an aggressive idle floor PRESSURE_OFFLOAD_IDLE_SECS = 60s. Once a shard is over budget, a segment untouched for a minute is shed to reclaim RAM instead of waiting out the full idle timeout; actively-queried segments (idle < 60s) stay resident and anything shed reloads on next touch. Together with the reloadable byte-cap eviction (A) and the reload-admission gate (B), the vector WARM/HOT tier now has a real memory ceiling that responds to pressure, not only to the wall-clock idle timer. Known follow-up: HOT-immutable memory has no standalone byte budget (only the idle/pressure demotion bounds it); the pressure signal remains KV+vector RSS vs the per-shard maxmemory budget, not a dedicated vector cap. Test: test_pressure_cascade_triggered_by_vector_memory_alone — a shard with trivial KV but ~93% of budget in resident vector segments fires the cascade; KV-only stays below threshold. Existing threshold tests updated for the new parameter (pass 0 vector bytes). author: Tin Dang
author: Tin Dang
…nting Adversarial perf + security review of the memory-aware offload branch found one HIGH-severity correctness bug and one accounting bug; both fixed here. - DROP workstream B (reload-admission gate). The synchronous LRU eviction in promote_unloaded/submit_unloaded_reloads could demote a pre-existing WARM segment to a COLD stub while only protecting the just-reloaded segments — and the triggering query then does a fresh load() and scans snapshot.warm for full-index KNN, silently OMITTING the evicted segment from its own top-k (deterministic recall loss, no warning). The root cause is fundamental: a full-recall KNN query needs its entire working set resident, so no WARM segment is safe to evict during that query. WARM is now bounded BETWEEN queries by the byte-cap tick (A) and memory-pressure offload (C), both recall-safe (in-flight queries hold Arc snapshots; future queries reload at capture). A single query's working set is the resident floor — consistent with #18's "await full recall". Removes evict_warm_lru_to_budget, the per-shard budget atomic, and its main.rs wiring. - FIX WARM memory accounting (both reviews' finding 2). SegmentHolder:: resident_bytes() hardcoded the WARM/COLD tiers to 0, so a shard whose HOT segments had aged into WARM (default --segment-warm-after 3600s) reported ~0 vector memory — blinding INFO/Prometheus AND the memory-pressure trigger (C) that this branch adds. It now sums the WARM tier (the dominant term for a long-lived shard) plus COLD stubs. IVF/DiskANN-cold still lack a resident accessor (noted). - HARDEN the byte-cap tick (perf finding 2). enforce_segment_holder_budget now takes the holder reload_lock around its load-mutate-swap, so A, the reload/install path, and C's pressure offload serialize on the lock rather than solely on shard-thread affinity (defense-in-depth against a future off-thread sweep or worker-pool install). Verified safe by review: the WARM->stub conversion (A) is durable on every warm-creation path (no in-memory-only WARM), tombstones survive the demote/reload cycle, and no A/B/C lost-update race exists on either runtime. Test: test_resident_bytes_includes_warm_tier asserts a resident WARM segment contributes to resident_bytes(). Gate tests removed with the gate. author: Tin Dang
TinDang97
force-pushed
the
feat/vector-memory-aware-offload
branch
from
July 9, 2026 19:10
d78dc6c to
f258407
Compare
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 14, 2026
…sed] + resolve PR #TBD placeholders (#330) The v0.6.0 tag (355f68d, 2026-07-10) absorbed several PRs that merged after the v0.6.0 release PR itself but before the tag was cut. Their changelog entries were left under [Unreleased], understating what shipped in the tagged v0.6.0 binary. Audit (task #65, v0.7.0 roll-up prep) identified 23 sections whose PR merge commits are verified ancestors of v0.6.0 via `git merge-base --is-ancestor`: PR #248 (FastScan SIMD, SQ8 default, TQ ADC L2 fix, EF_RUNTIME FT.CONFIG, RERANK_MULT+EXACT_BEAM, CLI/moon.conf tuning defaults), PR #250 (13 production-hardening sections), PR #251 (COLD-segment reload off event loop), PR #254 (roadmap docs suite), PR #255 (CI macOS 30m), PR #256 (v0.6.0 ledger closure). Moved verbatim to a new subsection at the top of [0.6.0], with a note explaining the absorption. Sections from PR #252, #253, and #257-#263 were verified NOT ancestors of v0.6.0 and correctly remain in [Unreleased]. Also resolves the "PR #TBD" placeholders left by those sections (now #248, #257, #261) now that the real PR numbers are known. Section-count invariant holds (306 before, 306 after); content unchanged except for the PR-number substitutions. author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Memory-aware vector WARM offload with a real, reloadable ceiling
Stacked on #251 (base =
feat/vector-offload-reload-pool) — shows only this delta.Problem
Investigating the #251 off-loop reload path surfaced that Moon's vector WARM/reload memory has no real ceiling and is invisible to every memory mechanism:
--engine-offload-idle-secs, default 3600s) — no memory-pressure input.--maxmemory, the elastic budget, and the diskfull guard.--vec-warm-mmap-budget) dropped evicted segments non-reloadably — a documented bug: an evicted WARM segment silently vanished from search until restart.WarmSearchSegment::from_filesfully materializes the segment on the heap (not mmap-evictable), so a query burst re-materializing many COLD segments grows resident heap toward OOM.Fix (two recall-safe, tick-driven bounds)
A — Reloadable byte-cap eviction.
MmapBudget::enforce_budgetnow demotes each evicted WARM segment to a reloadableUnloadedSegmentstub (inunloaded) instead of dropping theArc. Every WARM segment is durably disk-backed (transition_to_warmwrites its.mpffiles beforefrom_filesloads it — verified across all four warm-creation paths), so the stub reloads byte-identically.--vec-warm-mmap-budgetbecomes a real, reloadable per-shard ceiling. The eviction tick also takes the holderreload_lock(serializes with the reload/install path).C — Memory-triggered early offload. Two fixes:
SegmentHolder::resident_bytes()hardcoded WARM/COLD to 0, so a shard whose HOT segments had aged into WARM reported ~0 vector memory — blinding INFO/Prometheus and the pressure trigger. It now sums the WARM tier (the dominant term) + COLD stubs.should_run_pressure_cascade) now factors in the shard's vector resident bytes, and cascade step 2 offloads idle vector segments straight to COLD (real RAM reclaim, reloadable) at an aggressive 60s idle floor — previously it demoted HOT→WARM, which frees nothing.Together A and C bound WARM memory between queries (by byte-cap and by memory pressure), both recall-safe: in-flight queries hold
Arcsnapshots, future queries reload what they need at capture.Dropped after review: workstream B (synchronous reload-admission gate)
An adversarial security review caught that a synchronous per-query eviction gate is a deterministic recall bug: a full-recall KNN query needs its entire working set resident, so evicting any WARM segment during that query silently drops it from the query's own scan. A single query's working set is therefore the resident floor — consistent with #251's "await full recall". B was removed; A+C are the bound.
Review outcome
Perf + security reviews (adversarial): data-loss invariant verified safe (no in-memory-only WARM; every stub is disk-backed), tombstones survive demote→reload, no A/C lost-update race on either runtime. Findings fixed: the recall bug (B dropped), WARM accounting, and the tick
reload_lock.Known follow-ups
Verification
Dual-runtime (monoio + tokio):
clippy -D warningsclean both,fmtclean; new tests —test_eviction_demotes_to_reloadable_stub(A),test_resident_bytes_includes_warm_tier+test_pressure_cascade_triggered_by_vector_memory_alone(C) — plus full mmap_budget/holder/store/persistence_tick/reload_pool suites green on both runtimes.