[MOD-18863] Reuse hnsw internal id in updateVector - write-in-place mode

[nonirosenfeldredis]

Summary

• updateVector/updateVectors in write-in-place mode used to delete the label's existing vector(s) and re-append fresh ones - unnecessarily shuffling ids via the swap-to-last compaction a real removal uses, even though the label's vectors are being replaced 1:1 (or close to it) anyway.
• storeNewElement gained an optional elementId to overwrite an existing slot in place instead of appending. The detach half of removeVectorInPlace splits into repairConnectionsInPlace (repair every affected neighbor's connections) and repairEntryPoint (replace the entry point if needed), called together with disposeElementData (free the element's own graph data, no compaction) by both removeVectorInPlace and a new overwriteVectorInPlace, which composes that detach + reuse-store + link under one continuous indexDataGuard lock.
• Single-value: HNSWIndex_Single::addVector now overwrites an existing label in place via overwriteVectorInPlace instead of delete-then-append. The tiered in-place path required splitting deleteVector's flat-side cleanup into deleteFromFlatAndInsertJobs (so the backend copy is left alone for the reuse to find) and invalidating any repair job still pending against the reused id (found via a real crash under stress testing - a stale job from an earlier, unrelated async deletion would otherwise run against the new element in that slot).
• Multi-value: updateMultiValueInPlace pairs the label's existing ids with the new blobs one for one and reuses each pair in place. Extra old ids (label shrinking, including all the way to zero - which is functionally a delete) are removed for real, with the same bookkeeping deleteLabelFromHNSWInplace already uses; extra new blobs (label growing) are freshly appended. Each reused/appended blob is preprocessed the same way any other direct-to-backend write is, so quantized/cosine backends store correct values.
• reuseOnUpdate toggle: setReuseOnUpdate/isReuseOnUpdate let a caller fall back to the original delete-then-append behavior (default is reuse-on, matching the description above).
• SQ8 accumulation: the in-place reuse fast path is skipped while accumulating, since training writes must stay in FLAT in both write modes - it now falls back to the ordinary delete-then-add path, which already routes through the accumulation-aware addVector/deleteVector.
• Scope is deliberately write-in-place only; async is unaffected (still deletes + re-inserts as a fresh id, unchanged).
• Added getLabelSize(label) to the label-lookup interface so updateMultiValueInPlace can read a label's current id count in O(1) instead of building and discarding the whole id vector via getElementIds(label).size().
• Dropped the unused removeIdFromLabel from the label-lookup interface once popLastIdFromLabel covered every remaining id-removal call site.

Test plan

• overwriteReusesInternalId (single-value): id stability, other labels' ids untouched, graph integrity, search correctness after overwrite.
• updateVectorsMultiInPlaceReusesIds (multi-value): same-count (full reuse), shrinking (partial reuse + real removal), growing (partial reuse + fresh append), id/size/integrity/search checks throughout.
• updateVectorsMultiInPlaceShrinkToZeroRemovesLabel: shrinking a multi-value label to zero vectors removes every id and the label itself.
• updateVectorsMultiInPlaceInvalidatesPendingRepairJob: a repair job left pending against a reused id from an earlier, unrelated async deletion is invalidated before the id is overwritten, instead of later running against the new vector.
• updateVectorsMultiInPlaceNormalizesCosine: reused and freshly-appended blobs are both preprocessed correctly for a cosine backend.
• reuseOnUpdateToggleDisablesReuse: toggling reuse off restores the old delete-then-append id behavior, for both single- and multi-value.
• updateVectorsMultiInPlaceDuringAccumulationStaysInFlat: an in-place multi-value update during SQ8 accumulation stays entirely in FLAT and keeps the running-sum bookkeeping correct.
• Updated hnsw_blob_sanity_test, which explicitly encoded the old swap-to-last relocation behavior as its contract.
• Full test_hnsw suite (374 tests) passes, stress-run 8x back-to-back with no flakes/crashes under real concurrent execution.

🤖 Generated with Claude Code

---

Note

High Risk
Changes core HNSW mutation, graph repair, and tiered async repair invalidation; incorrect reuse could corrupt the graph or apply stale repair jobs to wrong slots.

Overview
Write-in-place updates no longer delete a label’s vector(s) and re-append (which shuffled unrelated internal ids via swap-to-last). The HNSW layer now detaches an element from the graph without compaction (repairConnectionsAndDetach + disposeElementData), reuses the same slot via optional elementId in storeNewElement, and re-links through overwriteVectorInPlace.

Single-value addVector overwrites in place. Tiered write-in-place routes through updateVectorInPlace / updateMultiValueInPlace (pair ids with new blobs, shrink/grow, invalidate stale async repair jobs before reuse). setReuseOnUpdate(false) restores delete-then-append; SQ8 accumulation still uses the old path.

Async write mode is unchanged.

^{Reviewed by Cursor Bugbot for commit d5415d7. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Stale Bugbot comment from a previous run.

[codecov]

Codecov Report

❌ Patch coverage is 93.98496% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.49%. Comparing base (fcdeb37) to head (dda24b4).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/VecSim/algorithms/hnsw/hnsw_single.h	38.46%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1059      +/-   ##
==========================================
- Coverage   97.55%   97.49%   -0.06%     
==========================================
  Files         142      142              
  Lines        9095     9199     +104     
==========================================
+ Hits         8873     8969      +96     
- Misses        222      230       +8     

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

[cursor]

Stale Bugbot comment from a previous run.

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit cd9bb91. Configure here.}

[cursor]

Failed overwrite leaves dangling graph slot

Medium Severity

overwriteVectorInPlace detaches and destroys the slot before storeNewElement allocates replacement graph data. That constructor throws on allocator failure, so the label still points at a slot whose others and incoming-edge lists are already freed. A caught low-memory error then leaves the index corrupted: later search, repair, or index teardown can use or double-free that memory. The previous delete-then-append path removed the label and compacted the slot first, so a failed re-insert did not leave a live dangling occupant.

Additional Locations (2)

• src/VecSim/algorithms/hnsw/hnsw.h#L2089-L2096
• src/VecSim/algorithms/hnsw/hnsw.h#L2020-L2025

 

^{Reviewed by Cursor Bugbot for commit cd9bb91. Configure here.}

[dor-forer]

Very good job!
Just a few minor comments

[dor-forer]

We can replace these two calls with a popLastIdFromLabel(label) method on the HNSW backend interface. The multi-value implementation could use back() and pop_back() directly on its private ID list, removing the label if it becomes empty. This avoids copying the entire list in getElementIds() and then searching it again in removeIdFromLabel(), while preserving encapsulation and reading the current mapping after each compaction.
Something like:

idType popLastIdFromLabel(labelType label) override {
    auto it = labelLookup.find(label);
    assert(it != labelLookup.end());

    auto &ids = it->second;
    assert(!ids.empty());

    const idType id = ids.back();
    ids.pop_back();
    if (ids.empty()) {
        labelLookup.erase(it);
    }
    return id;
}

If it is too much of a hustle you can leave it

[nonirosenfeldredis]

its defintly better!

[dor-forer]

Should this be directHNSWInsertions += n? The counter tracks vectors written directly to HNSW, while this helper processes n vectors. Incrementing once undercounts multi-vector updates and counts an insertion when n == 0 only deletes the label. Please cover both cases in the statistics assertions.

[dor-forer]

Could we also assert ID reuse during shrink and grow? After shrinking, check that the surviving ID belongs to the original set, then save it and verify that it remains after growing. Currently these sections would also pass with delete-and-append behavior.

[dor-forer]

Could we add a regression test that queues a repair job for an existing multi-value ID, overwrites that ID in write-in-place mode before the job runs, then drains the queue and runs GC? Please verify that the old job is invalidated, swap bookkeeping completes, and the replacement vectors and graph remain valid.

[nonirosenfeldredis]

@dor-forer very good comments!