Skip to content

[MOD-20706] Reuse HNSW ID holes after shared_ptr finalization - #1087

Open
nonirosenfeldredis wants to merge 14 commits into
mainfrom
reuse-from-cache-stacked
Open

nonirosenfeldredis wants to merge 14 commits into
mainfrom
reuse-from-cache-stacked

Conversation

@nonirosenfeldredis

@nonirosenfeldredis nonirosenfeldredis commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Describe the changes in the pull request

Allow tiered HNSW inserts to fill reusable ID holes published by a shared_ptr<idType> finalizer. Insert-time reuse remains disabled by default through setReuseOnInsert.

After a deleted node's last repair completes, detach it from the graph under shared main-index access. Active query, insert, repair, and delete markers retain references to its ID so a reader with a saved candidate or graph pointer can finish safely. The last reference's deleter clears the slot and publishes it to the FIFO reusableIdHoles cache, using shared main-index access. Batch iterators retain their markers until depletion, reset, or destruction. Finalization does not acquire the main index guard exclusively.

Inserts fill the oldest reusable hole under shared main-index access, or append when the cache is empty. GC only consumes finalized cache entries for compaction, up to maxJobsToRun, regardless of whether insert-time reuse is enabled. It drains the remaining cache when the limit permits, including a single remaining entry. TIERED_HNSW_NUM_ID_HOLES reports the current number of reusable ID holes in the cache. It increases when finalizers publish slots and decreases when inserts reuse them or GC compacts them; deleted nodes still held by readers are excluded.

RuntimeMarker usage

RuntimeMarker tracks an operation that may retain an internal ID or graph pointer. attach() registers it in the index's currentlyRunning list under runningGuard, following the operation-marker approach used by the disk index.

  • Top-K and range queries: a scoped marker covers the query.
  • Inserts, repairs, and deletions: scoped markers cover the backend operations that can retain graph references. For in-place deletion, the marker outlives the existing exclusive main guard.
  • Batch iterators: the marker becomes active before creating the backend iterator and remains active across batches until depletion, reset, or destruction.

Once a repaired node is isolated, its shared_ptr<idType> is added to every currently active marker's pendingIds. Operations starting afterward cannot reach the isolated node and do not retain that reference.

detach() (also called by the destructor) unregisters the marker under runningGuard, then releases its ID references after releasing both that mutex and its operation's main guard. The last reference invokes the custom deleter, which takes shared main-index access, clears the slot, and publishes it to the cache. This protects saved candidates and graph pointers without an exclusive finalization lock.

Which issues this PR fixes

  1. MOD-19327

Main objects this PR modified

  1. Tiered HNSW repair accounting, node isolation, operation markers, and ID finalization. popPendingFinalizer() centralizes the pending queue pop under its mutex.
  2. Insertion into reusable ID holes, bounded cache draining through takeReusableIdHoles(), GC compaction, and cache size statistics.
  3. Tests covering hook-controlled query/delete interleavings, last-reader release, concurrent queries and overwrites, deletion without repairs, invalidation, relocated cached holes, and GC limits.

The deterministic query/delete test runs for both top-K and range queries. One hook pauses after the query saves a candidate's graph pointer; another pauses after scanning, before the query releases its marker. A separate thread deletes and repairs the node while the query is paused. The test verifies that deletion completes without an exclusive finalization lock, the hole remains unavailable at both checkpoints, and the original ID becomes reusable when the completed query releases its marker.

Marker lifetime coverage

Operation Deterministic lifetime check
Top-K and range queries Pause with a saved graph pointer; finish deletion and repairs; resume the query before releasing its marker.
Query timeout Both query types retain the hole until their timed-out return path releases the marker.
Batch iterators Reset, destruction, and depletion release the marker; depletion is checked while the iterator object remains alive.
Background and direct inserts Append and hole-reuse paths retain a newly retired ID until the insert marker detaches.
Repair jobs The last repair has completed and isolated the node, but its hole waits for the repair marker to detach.
Async deletion Repairs complete while the delete operation is paused before marker detach.
In-place deletion Invalidating a pending repair completes an earlier deletion; its hole waits for the in-place delete marker to detach.
Explicit GC with a pending finalizer GC compacts ready holes and can relocate the waiting ID. After the query marker detaches, another GC empties the cache and all finalizer tracking.

Validation

  • 303 tiered HNSW tests and 209 HNSW SQ8 tests passed locally.
  • All 69 tiered cache tests passed again after extracting the cache-draining helper; both HNSW test targets rebuilt successfully.
  • The seven new operation-marker, timeout, depletion, and explicit-GC tests passed ten repetitions normally and ten under AddressSanitizer.
  • The saved-pointer top-K/range query/delete test also passed ten normal and AddressSanitizer repetitions during query validation.
  • Formatting and diff checks passed.

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

Note

High Risk
Changes core tiered HNSW deletion, compaction, and concurrent graph traversal lifetimes via new marker/reference counting; correctness bugs could cause use-after-free or graph corruption under parallel queries and async repairs.

Overview
Tiered HNSW defers clearing deleted node slots until no in-flight operation can still hold stale internal IDs or graph pointers. When repairs finish, nodes are isolated under shared locking; RuntimeMarker tracks queries, inserts, repairs, and deletes (including batch iterators) and holds shared_ptr references so the last reader triggers clearDeletedSlot and publishes the slot into a FIFO reusableIdHoles cache—without taking the main index lock exclusively for finalization.

Optional insert-time reuse (setReuseOnInsert, off by default) fills the oldest cleared hole via storeNewElementInClearedSlot instead of always growing curElementCount. compact() (renamed from defrag()) still runs swap compaction but only drains already finalized cache entries, with bounded batches unchanged in spirit. Repair scheduling skips neighbors already isolated, and debug info adds TIERED_HNSW_NUM_ID_HOLES. Test-only hooks on candidate graph reads support concurrency tests for query/delete interleaving.

Reviewed by Cursor Bugbot for commit 313c775. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.42%. Comparing base (eb02555) to head (313c775).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1087      +/-   ##
==========================================
+ Coverage   97.37%   97.42%   +0.04%     
==========================================
  Files         145      145              
  Lines        9560     9740     +180     
==========================================
+ Hits         9309     9489     +180     
  Misses        251      251              

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

Base automatically changed from reuse-from-cache to main October 29, 2026 13:59
nonirosenfeldredis and others added 2 commits November 1, 2026 22:20
Re-add the cache of cleared slots: the GC clears the slots of ready swap jobs
into a FIFO cache and compacts half of it, and an insert takes the oldest slot
instead of appending a fresh id. Reuse is off by default (`setReuseOnInsert`).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@nonirosenfeldredis nonirosenfeldredis changed the title [MOD-19327] Reuse ids on insert from a cache filled by the GC [MOD-19327] Reuse ids on insert from a cache filled by deletion finalization Nov 1, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix All in Cursor

❌ 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 404f52a. Configure here.

Keep detached IDs alive through active operation markers. Clear and cache each slot from the last reference deleter using shared main-index access, and let GC consume all cached entries up to its limit.
@nonirosenfeldredis nonirosenfeldredis changed the title [MOD-19327] Reuse ids on insert from a cache filled by deletion finalization [MOD-19327] Reuse deleted HNSW slots after shared_ptr finalization Nov 1, 2026
@nonirosenfeldredis nonirosenfeldredis changed the title [MOD-19327] Reuse deleted HNSW slots after shared_ptr finalization [MOD-19327] Reuse HNSW ID holes after shared_ptr finalization Nov 1, 2026
@nonirosenfeldredis nonirosenfeldredis changed the title [MOD-19327] Reuse HNSW ID holes after shared_ptr finalization [MOD-20706] Reuse HNSW ID holes after shared_ptr finalization Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant