feat(diskann): delete-time in-neighbor repair with in-degree tracking and garden refresh - #104
feat(diskann): delete-time in-neighbor repair with in-degree tracking and garden refresh#104huanglune wants to merge 3 commits into
Conversation
…rden refresh Delete-time repair (update_repair): tombstone the batch, discover each deleted node's approximate in-neighbors (out-neighbor symmetry plus an optional repair_search_l candidate search), rebuild their lists while the slots are still dark, then release the slots for reuse. SlotAllocator grows mark_removed()/release() to split tombstoning from free-list entry. In-degree tracking (track_in_degree): live in-degree counters maintained across all three neighbor-list write paths, recounted at load. Garden refresh (garden_refresh): re-links the lowest in-degree live nodes with an insert-style search under a caller budget. Bench grows --update_repair / --track_indegree / --garden_* knobs, an --oracle_ids batch-rebuild reference mode, and a --garden_pass coverage probe. E2E tests cover the repair contract, churn recall floor, the in-degree counter invariant, and garden behavior.
…repair robustness Review fixes on the delete-repair feature: - Skip the lazy path's safety-net reconnect on the repair branch. Repair's discovery set (out-neighbors + optional search) is a superset of the set the net scans, so under default arming (5% tombstones / 16 deletes) every armed batch paid a full-history scan that could never find an edge to fix. Pinned by RepairSkipsSafetyNetUnderDefaultArming, which fails on the previous behavior. - Replace worker-count wave chunks with kUpdateWaveChunk=1024 across repair discovery, the repair wave, and garden_refresh: chunking at the worker count re-created the intra-batch barrier convoy batch_insert removed and never filled the 4x AsyncGate; unbounded when_all would churn the page LRU. Bound read_delete_nodes chunks by half the page cache (new DiskPageIO::page_cache_capacity()) instead of the worker count. - Give the repair-mode local pool the same worker count with or without the reactor (was 1 with it, serializing discovery searches and RMWs). - Short-circuit symmetry-only discovery (repair_search_l=0) to a sync loop over the in-memory out-neighbor lists. - Garden refresh now shares the insert path's selection-coords wave (extracted wave_selection_coords) and warms the node + pruned pages in one wave before its RMW loop, mirroring insert_one. - Release the batch's slots when repair throws (degrades to lazy-delete semantics) instead of leaking them tombstoned off the free list. - Warn once when fresh slots outrun the in-degree tracking headroom. - Sort once for in-degree percentiles instead of three nth_element copies.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in delete-time in-neighbor repair mode to the updatable DiskANN index, along with live in-degree tracking and a budgeted garden_refresh maintenance pass, and extends benchmarks/e2e coverage to exercise the new behaviors.
Changes:
- Introduces delete-time repair flow (tombstone-first, repair in-neighbors, then release slots) and new load params to control it.
- Adds optional in-degree tracking plus
garden_refresh(budget)and related stats plumbing. - Expands benchmark CLI flags and adds multiple e2e tests covering repair, churn, in-degree invariants, and garden behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/diskann/test_diskann_update_e2e.cpp | Adds e2e coverage for repair delete semantics, churn recall floors, in-degree invariants, and garden refresh contracts. |
| tests/diskann/bench_diskann_sift_update.cpp | Extends benchmark options/CLI behavior for update repair, in-degree tracking, garden passes, and oracle rebuild evaluation. |
| include/index/graph/diskann/slot_allocator.hpp | Splits delete semantics into tombstone-only (mark_removed) and delayed freelist release (release). |
| include/index/graph/diskann/diskann_index.hpp | Implements repair-mode delete batching, in-degree tracking/recounting, and garden_refresh, plus new instrumentation fields. |
| include/index/graph/diskann/disk_page_io.hpp | Exposes effective page-cache capacity for wave sizing and chunking decisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void mark_removed(uint32_t id) { tombstone_.set(id); } | ||
|
|
||
| /// Make a mark_removed() slot reusable after its in-neighbor repair window. | ||
| void release(uint32_t id) { free_list_.push_back(id); } |
There was a problem hiding this comment.
Adopted in d9a6261: release() now re-asserts the tombstone before enqueuing (a no-op in the intended mark_removed() -> release() sequence, which runs entirely under the update serial mutex with no publish possible in between — but the hardening makes the invariant local instead of structural).
| void print_usage(const char *argv0) { | ||
| std::cerr << "Usage:\n" | ||
| << " " << argv0 << " [data_dir] [trace_dir] [index_dir] [out_csv] [flags]\n\n" | ||
| << "Flags:\n" | ||
| << " --rebuild force a fresh index build\n" | ||
| << " --deterministic deterministic search barriers\n" | ||
| << " --flush_rounds flush dirty pages after each round\n" | ||
| << " --no_flush_rounds skip per-round dirty-page flush\n" | ||
| << " --no_update_rerank disable update rerank\n" | ||
| << " --update_insert_prune alpha-prune insert candidates\n" | ||
| << " --update_repair enable delete-time in-neighbor repair\n" | ||
| << " --no_update_repair disable delete-time in-neighbor repair\n" | ||
| << " --repair_search_l N repair search list (0 => deleted-node out-neighbors)\n" | ||
| << " --track_indegree track live-slot in-degree percentiles\n" | ||
| << " --garden_budget N refresh up to N low-in-degree live nodes per round\n" | ||
| << " --garden_l N garden search list (0 => use --update_L)\n" | ||
| << " --build_only build the initial index and exit\n" | ||
| << " --eval_only replay live mask and run eval only\n" | ||
| << " --single_updates issue single remove/insert calls\n" | ||
| << " --mixed run mixed update/search workload\n" | ||
| << " --mixed_mode MODE background or shared_queue\n" | ||
| << " --rounds N cap update rounds\n" | ||
| << " --nq N cap eval queries (0 => all)\n"; | ||
| } |
There was a problem hiding this comment.
Adopted in d9a6261: --garden_pass, --garden_pass_iters and --oracle_ids are now listed in print_usage().
| std::vector<uint64_t> read_ids_file(const std::string &path) { | ||
| std::ifstream in(path, std::ios::binary); | ||
| if (!in) { | ||
| throw std::runtime_error("cannot open oracle ids file: " + path); | ||
| } | ||
| uint64_t count = 0; | ||
| in.read(reinterpret_cast<char *>(&count), sizeof(count)); | ||
| if (!in) { | ||
| throw std::runtime_error("oracle ids file truncated header: " + path); | ||
| } | ||
| std::vector<uint64_t> labels(count); | ||
| in.read(reinterpret_cast<char *>(labels.data()), | ||
| static_cast<std::streamsize>(count * sizeof(uint64_t))); | ||
| if (!in) { | ||
| throw std::runtime_error("oracle ids file truncated body: " + path); | ||
| } | ||
| return labels; | ||
| } |
There was a problem hiding this comment.
Adopted in d9a6261: the header count is validated against std::filesystem::file_size before the vector is allocated, so a corrupt ids.bin fails with a clear error instead of a giant allocation.
| spdlog::info("DiskANNIndex::load: in-degree recount scanned {} slots in {} us", | ||
| counts.size(), | ||
| scan_us); |
There was a problem hiding this comment.
Keeping this at info deliberately: the recount is opt-in (track_in_degree) and is a full neighbor-list scan that can take seconds to minutes on a large index — without the line, load() looks mysteriously slow with no explanation. It fires once per load, and test/embedding contexts already control verbosity via the spdlog level (the e2e fixture runs at warn). Happy to revisit if the project prefers the LOG_* macros from utils/log.hpp for consistency.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces delete-time in-neighbor graph repair and live in-degree tracking/refresh (gardening) for the DiskANN index. It adds configuration parameters, tracking arrays, and methods to split slot deletion into tombstoning and releasing phases, allowing in-neighbors to be rebuilt before slots are reused. Additionally, it updates benchmarking tools and adds extensive E2E tests to validate these new features. Feedback is provided regarding the kInDegreeRecountChunk constant, which is set to a very high value (65536) and could trigger massive concurrent I/O waves, potentially exhausting kernel resources or causing memory allocation failures; reducing this chunk size to a safer limit is recommended.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // untracked by the guarded in-degree helpers; brand-new slots are not garden | ||
| // targets. | ||
| static constexpr uint64_t kInDegreeHeadroomSlots = 1ULL << 20; | ||
| static constexpr uint32_t kInDegreeRecountChunk = 65536; |
There was a problem hiding this comment.
The kInDegreeRecountChunk constant is set to 65536. When recount_in_degree is called during index load, this large chunk size is passed to read_delete_neighbors, which in turn calls read_neighbors_batch_async (when the reactor is enabled). This triggers a massive concurrent I/O wave via wave_load_pages for up to 65,536 pages, allocating a huge contiguous buffer (up to 256 MB) and submitting an excessive number of concurrent requests to io_uring. This can easily exhaust kernel resources, exceed the io_uring submission queue depth, or cause severe memory allocation failures.
To prevent LRU cache thrashing and I/O queue exhaustion, consider reducing kInDegreeRecountChunk to a much safer size (e.g., 1024 to match kUpdateWaveChunk or 4096).
| static constexpr uint32_t kInDegreeRecountChunk = 65536; | |
| static constexpr uint32_t kInDegreeRecountChunk = 1024; |
There was a problem hiding this comment.
Partially adopted in d9a6261 — the constant is now 8192, though not for the SQ-depth reason: UringReactor::read_batch submits in ring-depth chunks by contract (see the reactor header notes), so a large wave cannot exceed the submission queue, and the recount reads land in a private wave buffer (reconciled afterwards), not the prefetch LRU. The real bound is the ONE contiguous aligned buffer backing each wave's unique pages plus the in-flight count: at 65536 consecutive ids that is ~13k pages (~51 MiB) for SIFT-128 and up to 256 MiB when a node fills a page (high-dim). 8192 caps the worst case at 32 MiB while keeping waves far above the ring depth, which is what actually saturates the one-time load scan — 1024 would give up scan throughput without buying extra safety.
…(), bench arg validation - kInDegreeRecountChunk 65536 -> 8192: each recount wave backs its unique pages with one contiguous aligned buffer and keeps them all in flight (the reactor already submits in ring-depth chunks), so the real bound is wave memory — 8192 ids cap the worst case (one node per page) at 32 MiB while staying far above the ring depth that saturates the one-time scan. - SlotAllocator::release() re-asserts the tombstone (no-op in the intended sequence) so misuse on a live slot cannot leave it searchable + allocatable. - bench: validate the ids.bin header count against the file size before allocating; document --garden_pass/--garden_pass_iters/--oracle_ids in print_usage.
Description
Adds an opt-in delete-time in-neighbor repair mode to the updatable DiskANN index, plus live in-degree tracking and a budgeted
garden_refreshmaintenance pass, with follow-up scheduling/robustness fixes from review.Today's lazy delete tombstones a slot and puts it straight on the free list; in-neighbors keep dangling edges until an insert's reconnect or the safety net touches them, and once the slot is reused those edges silently point at an unrelated new vector. With
update_repairon, a delete batch is tombstoned first, each deleted node's approximate in-neighbors (out-neighbor symmetry, optionally merged with an insert-style candidate search viarepair_search_l) are rebuilt while the slots are still dark — dead edges dropped, two-hop spliced — and only then do the slots become allocatable, so a reused slot can no longer inherit stale in-edges from its previous owner.Related Issue
N/A
Type of Change
Changes Made
SlotAllocator: splitfree()intomark_removed()(tombstone, not reusable) +release()(enter the free list after the repair window).DiskANNLoadParams.update_repair/repair_search_l: delete-time discovery + repair wave shared byremove/batch_remove/batch_remove_with_pool; the lazy path is unchanged and remains the default. Repair supersedes the lazy path's safety-net reconnect (its affected set is a subset of the discovery set), so the net is skipped on the repair branch.DiskANNLoadParams.track_in_degree: live in-degree counters maintained symmetrically across all neighbor-list write paths, recounted from disk at load;garden_refresh(budget)re-links the lowest-in-degree live nodes (search → prune → forward + reverse edges) and reports before/after percentiles.kUpdateWaveChunk = 1024chunks instead of worker-count chunks (which re-created the intra-batch barrier convoybatch_inserthad removed);read_delete_nodeschunks by half the page cache via the newDiskPageIO::page_cache_capacity(); symmetry-only discovery (repair_search_l = 0) short-circuits to a sync loop; the repair-mode local pool uses the same worker count with or without the io_uring reactor.--update_repair,--repair_search_l,--track_indegree,--garden_budget/--garden_l, an--oracle_idsbatch-rebuild reference mode, and a--garden_passcoverage probe.Testing
Seven new e2e tests in
test_diskann_update_e2e: the repair remove contract (hidden → repaired → slots reused at same capacity), churn recall floor vs lazy mode, search-assisted discovery, the in-degree counter == full-recount invariant across insert/delete/garden, garden lifting the starved in-degree tail, garden API contracts, andRepairSkipsSafetyNetUnderDefaultArming(fails without the safety-net skip).Also run under ThreadSanitizer (
-fsanitize=thread -O1): the repair/in-degree/garden tests pass, and every reported race classifies to the pre-existing OpenMPVamanaBuilderbuild phase (vamana_builder.hpp,robust_prune.hpp) — none in the update paths this PR touches. CI is green on the fork branch.Checklist
make lintand fixed any issuesmake test)Additional Notes
--oracle_idsmode exists to quantify that residual against a batch rebuild.VamanaBuilder(OpenMP build phase) data races observed under TSan are present onmainand may deserve a separate issue.