[MOD-17578] Isolate a deleted element from the graph once its repairs are done - #1012
Open
nonirosenfeldredis wants to merge 3 commits into
Open
[MOD-17578] Isolate a deleted element from the graph once its repairs are done#1012nonirosenfeldredis wants to merge 3 commits into
nonirosenfeldredis wants to merge 3 commits into
Conversation
A marked-deleted element used to keep its edges until its swap job disposed of it: `removeFromGraph` was the one to take it out of its neighbours' incoming edges sets, under the exclusive main index guard. Instead, take the element out of the graph in the repair context, as soon as the last repair job created for its deletion completes - dropping every edge in and out of it, so that by the time the swap job runs there is nothing left to disconnect. An element that never had repair jobs is isolated right away, upon marking it deleted. Three adjustments this required: - The pending repair jobs counter is now decreased *after* the repair has been performed, not before. Otherwise a swap job could be seen as ready - and its element isolated - while an element still pointed at it from a repair that had not run yet. - `mutuallyRemoveNeighborAtPos` inferred the edge direction from the absence of a record on the other side. Isolation clears those records, so it now checks the links directly. - `repairConnectionsForDeletion` no longer assumes a deleted neighbour still holds the link it is about to remove, as an isolated one holds no links. Note that a deleted element now holds no edges while it waits for its swap job, so it no longer generates repair jobs for subsequent deletions - `swapJobBasic2` was retraced accordingly, and `invalidRepairJobOnSwap` covers the invalidation of a pending repair job whose node is disposed of. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
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 f25f84c. Configure here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1012 +/- ##
==========================================
- Coverage 97.17% 97.17% -0.01%
==========================================
Files 141 141
Lines 8328 8382 +54
==========================================
+ Hits 8093 8145 +52
- Misses 235 237 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nonirosenfeldredis
requested review from
GuyAv46,
alonre24 and
dor-forer
and removed request for
GuyAv46
August 13, 2026 16:16
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
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.

Describe the changes in the pull request
A marked-deleted element used to keep its edges until its swap job disposed of it:
removeFromGraphwas the one to take it out of its neighbours' incoming edges sets, under the exclusive main index guard.This PR takes the element out of the graph in the repair context instead, as soon as the last repair job created for its deletion completes - dropping every edge in and out of it (
isolateDeletedElement), so that by the time the swap job runs there is nothing left to disconnect. An element that never had any repair job is isolated right away, upon marking it deleted.Three adjustments this required:
mutuallyRemoveNeighborAtPosinferred the edge direction from the absence of a record on the other side. Isolation clears those records, so it now checks the links directly.repairConnectionsForDeletionno longer assumes a deleted neighbour still holds the link it is about to remove, as an isolated one holds no links.Behaviour worth noting for reviewers: a deleted element now holds no edges while it waits for its swap job, so it no longer generates repair jobs for subsequent deletions.
swapJobBasic2was retraced accordingly (one repair job instead of two, and the two "invalid repair job" steps it used to produce no longer occur), and a newinvalidRepairJobOnSwaptest covers the invalidation of a pending repair job whose node is disposed of - reached by deleting a second element while the first is still connected.The only edges that reach the isolation fallbacks are edges between two deleted elements (neither gets a repair job for the other); this is asserted rather than silently handled. Verified by counting the firings across the suite - 9 of each, all with the other side marked deleted, none live.
Which issues this PR fixes
Main objects this PR modified
HNSWIndex::isolateDeletedElement(new),removeFromGraph,mutuallyRemoveNeighborAtPos,repairConnectionsForDeletion,removeLinkIfExists(new helper)TieredHNSWIndex::executeRepairJob/deleteLabelFromHNSW- isolation call sites and the moved counter decrementtests/unit/test_hnsw_tiered.cpp-swapJobBasic2retraced,invalidRepairJobOnSwapaddedMark if applicable
Note on the unchecked boxes: no API or serialization format change, but the graph content does change - a marked-deleted element is serialized with no edges. Anything comparing dumped graphs across versions will see that.
🤖 Generated with Claude Code
Note
High Risk
Changes core tiered HNSW async deletion, graph connectivity, and lock ordering; serialized graphs may show deleted nodes with no edges.
Overview
Marked-deleted HNSW nodes are removed from the graph as soon as their deletion repairs finish (or immediately when no repair jobs were created), via new
isolateDeletedElement, instead of keeping edges until the swap job runs. Physical slot reclaim is split intoremoveFromGraph+swapWithLast(formerly oneremoveAndSwappath).Tiered async delete now isolates after the last repair job completes, decrements swap-job pending counters only after
repairNodeConnectionsruns (avoiding premature “ready” swaps), and splits swap bookkeeping intoinvalidateRepairJobsvsfixJobsAfterSwap. Edge repair helpers tolerate already-isolated nodes (removeLinkIfExists, link scan inmutuallyRemoveNeighborAtPos).Unit tests
swapJobBasic2and newinvalidRepairJobOnSwapreflect fewer spurious repair jobs and repair invalidation on swap.Reviewed by Cursor Bugbot for commit 25a744b. Bugbot is set up for automated code reviews on this repo. Configure here.