Skip to content

[MOD-17578] Isolate a deleted element from the graph once its repairs are done - #1012

Open
nonirosenfeldredis wants to merge 3 commits into
mainfrom
sharon-17578-isolate-swap
Open

[MOD-17578] Isolate a deleted element from the graph once its repairs are done#1012
nonirosenfeldredis wants to merge 3 commits into
mainfrom
sharon-17578-isolate-swap

Conversation

@nonirosenfeldredis

@nonirosenfeldredis nonirosenfeldredis commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Describe the changes in the pull request

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.

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:

  1. 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.
  2. 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.
  3. repairConnectionsForDeletion no 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. swapJobBasic2 was retraced accordingly (one repair job instead of two, and the two "invalid repair job" steps it used to produce no longer occur), and a new invalidRepairJobOnSwap test 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

  1. MOD-17578

Main objects this PR modified

  1. HNSWIndex::isolateDeletedElement (new), removeFromGraph, mutuallyRemoveNeighborAtPos, repairConnectionsForDeletion, removeLinkIfExists (new helper)
  2. TieredHNSWIndex::executeRepairJob / deleteLabelFromHNSW - isolation call sites and the moved counter decrement
  3. tests/unit/test_hnsw_tiered.cpp - swapJobBasic2 retraced, invalidRepairJobOnSwap added

Mark if applicable

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

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 into removeFromGraph + swapWithLast (formerly one removeAndSwap path).

Tiered async delete now isolates after the last repair job completes, decrements swap-job pending counters only after repairNodeConnections runs (avoiding premature “ready” swaps), and splits swap bookkeeping into invalidateRepairJobs vs fixJobsAfterSwap. Edge repair helpers tolerate already-isolated nodes (removeLinkIfExists, link scan in mutuallyRemoveNeighborAtPos).

Unit tests swapJobBasic2 and new invalidRepairJobOnSwap reflect 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.

nonirosenfeldredis and others added 2 commits August 12, 2026 13:45
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>

@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 f25f84c. Configure here.

Comment thread src/VecSim/algorithms/hnsw/hnsw_tiered.h
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.17%. Comparing base (efd63da) to head (25a744b).

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

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant