Skip to content

[MOD-18683] Invalidate the pending repair jobs of an isolated element - #1053

Open
nonirosenfeldredis wants to merge 1 commit into
mainfrom
sharon-invalidate-repairs-after-isolate
Open

nonirosenfeldredis wants to merge 1 commit into
mainfrom
sharon-invalidate-repairs-after-isolate

Conversation

@nonirosenfeldredis

@nonirosenfeldredis nonirosenfeldredis commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Motivation

part of the optimization of reuse deleted ids.
first pr was: #1012

What

Isolating a deleted element takes every edge in and out of it, including the very edges that a
pending repair job on that element was created to remove. Those jobs were left for the swap job to
invalidate, so the swap job of another deleted element kept waiting for a repair that the
isolation had already made unnecessary. They are now invalidated at isolation time, so a waiting
swap job is released as soon as the element leaves the graph rather than when its slot is reclaimed.

Locking

In short: idToRepairJobsGuard becomes the one lock that guards access to the repair jobs map,
taken explicitly by every path that reads or writes it, instead of some paths relying on the main
index guard being held exclusively.

Invalidating a repair job from the isolation path means doing it while the main index guard is held
for shared ownership only, which the previous locking could not take:

  • executeRepairJob tested job->isValid outside idToRepairJobsGuard and only then claimed the
    job from idToRepairJobs. An invalidation landing in between rewrites node_id with the
    invalid-job key (setAndSaveInvalidJob stores it there), so the claim would miss. Both steps are
    now decided under that one guard.
  • invalidateRepairJobs takes idToRepairJobsGuard itself instead of relying on the main guard
    being held exclusively. A job still listed in the lookup has not been claimed by a worker yet -
    executeRepairJob de-registers it under the same guard before touching it - so it is alive for as
    long as the guard is held.
  • Its two existing callers (executeReadySwapJobs, deleteLabelFromHNSWInplace) are main-exclusive
    and do not hold the guard, so main -> idToRepairJobsGuard is unchanged and the guard is not
    taken recursively. The only new nesting is idToRepairJobsGuard -> invalidJobsLookupGuard
    (through setAndSaveInvalidJob), and nothing takes those two in the opposite order.

readySwapJobs is mutated inside invalidateRepairJobs, so it is now under the guard there as well

  • matching the other two mutations that run under the main guard held shared. The remaining ones
    (executeReadySwapJobs, deleteLabelFromHNSWInplace) are main-exclusive, which excludes every
    shared holder.

Tests

Three existing tests encoded the old timing and were updated, since the new timing is the point:

  • invalidRepairJobOnSwap - the 0->1 job is invalidated when 0 is isolated, before any GC round,
    and 1's swap job stops waiting for it there. With both swap jobs ready, disposing of them takes two
    GC rounds at a threshold of 1.
  • deleteVectorMulti - the second repair job is invalidated rather than executed, so its node_id
    now holds its invalid-job key.
  • swapJobBasic - this flow now reaches an invalidation, so the memory accounting reserves 0 buckets
    in the invalid jobs lookup like it does for the other two lookups.

ASAN, locally: test_hnsw 343/343 (261 tiered), test_hnsw_parallel 4/4.

Follow-up (commented in the code)

A swap job released here is not isolated in turn - it is only marked ready, and
executeReadySwapJobs removes its element by walking its edges, as for any element whose repairs
completed without this shortcut. Cascading the isolation would make that removal the cheap one too.

🤖 Generated with Claude Code

Isolating a deleted element removes every edge in and out of it, including the
edges that a pending repair job *on* that element was created to remove. Those
jobs were left for the swap job to invalidate, so a swap job of another deleted
element kept waiting for a repair that the isolation had already made
unnecessary. Invalidate them at isolation time instead: the waiting swap jobs
are released as soon as the element leaves the graph, rather than when its slot
is reclaimed.

Doing so means a repair job can now be invalidated while the main index guard is
only held for shared ownership, which the previous locking could not take:
`executeRepairJob` tested `isValid` outside `idToRepairJobsGuard` and only then
claimed the job from the lookup, so an invalidation landing in between would
rewrite `node_id` with the invalid-job key and the claim would miss. Both steps
are now decided under that guard, and `invalidateRepairJobs` takes it itself
instead of relying on the main guard being held exclusively. Its existing
callers are main-exclusive, so the main -> repair-jobs order is unchanged; the
only new nesting is repair-jobs -> invalid-jobs (through
`setAndSaveInvalidJob`), which nothing takes in the opposite order.

`invalidRepairJobOnSwap` covers the new timing: the 0->1 job is invalidated when
0 is isolated, not when it is swapped out. `swapJobBasic` now reaches an
invalidation, so it has to account for the invalid jobs lookup's buckets like
the other lookups, and `deleteVectorMulti`'s second repair job is invalidated
rather than executed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.54%. Comparing base (b1f01bf) to head (57890e6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1053   +/-   ##
=======================================
  Coverage   97.54%   97.54%           
=======================================
  Files         141      141           
  Lines        9011     9014    +3     
=======================================
+ Hits         8790     8793    +3     
  Misses        221      221           

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

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.

2 participants