[nonirosenfeldredis]

Summary

• protect every tiered backend deletion with mainIndexGuard shared
• keep creation of a tombstone and its counter increment in the same GC epoch
• add a deterministic tiered-index regression test for the post-compact deletion window

GC can finish compacting the SVS index, then a concurrent tiered deletion can increment num_marked_deleted before GC resets the counter to zero. The atomic counter prevents a C++ data race but the unconditional store still loses the logical update because the deletion path previously did not acquire either GC lock. The fix routes all tiered backend deletions through a helper that holds mainIndexGuard shared, while GC already holds that guard exclusively through compaction and the counter reset.

Testing

• regression test with the original unlocked implementation: fails because the post-compact deletion is not blocked
• regression test with the fix: passes
• existing markedDeleted, runGCAPI, and runGCInPlaceMode tests pass
• clang-format dry run and git diff --check pass

The before/after tests were run in an isolated build using the repository-pinned SVS revision because the working tree's pre-existing SVS submodule checkout differs from the pinned commit.

Supersedes #1081, which GitHub closed when its head branch was renamed.

---

Note

Medium Risk
Changes concurrency around tiered SVS deletions and GC counter semantics; scope is localized but affects correctness of when reindex/GC is triggered.

Overview
Fixes a logical race between tiered SVS garbage collection and backend deletions on num_marked_deleted: after compact(), GC could reset the counter to zero while a concurrent deletion (previously unlocked) still incremented it, so the new tombstone was effectively dropped.

Tiered SVS now routes every backend delete (deleteVector, in-place/async overwrites) through deleteVectorFromBackend, which takes a shared mainIndexGuard so deletions cannot interleave with GC’s exclusive hold through compact() and the counter reset. GC wrapper docs are updated to reflect that the guard also blocks backend deletions.

Test-only: SVSIndex::setAfterCompactCallbackForTest runs a hook after compact() and before the counter reset; a new unit test drives a concurrent deleteVector in that window and asserts it blocks until GC finishes, preserving the marked-deletion count across epochs.

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

[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 6892476. Configure here.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.53%. Comparing base (a59dd72) to head (7692f8e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1082      +/-   ##
==========================================
+ Coverage   97.52%   97.53%   +0.01%     
==========================================
  Files         143      143              
  Lines        9697     9704       +7     
==========================================
+ Hits         9457     9465       +8     
+ Misses        240      239       -1     

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

[razdoburdin]

Looks good,
but wouldn't it better to just fix the initial snapshot logic? No need in shared mutex for deletions in this case.

in svs.h runGC() body:

const size_t before = num_marked_deleted.load(std::memory_order_relaxed);
if (auto impl = getImpl(); impl && impl->view_data().size() != 0) {
   impl->compact();
}
num_marked_deleted.fetch_sub(before, std::memory_order_relaxed);

[nonirosenfeldredis]

I initially used that snapshot/fetch_sub approach, but it has a second race because the snapshot is not synchronized with SVS's compaction cutoff. A deletion can complete after the load and before compact() acquires SVS's internal exclusive lock; compact() then removes that tombstone, but fetch_sub(before) leaves its increment in num_marked_deleted. If it was the last node, the tiered empty-index fast path can leave that stale count indefinitely.

The view_data().size() check does not close that window. Taking mainIndexGuard shared around the complete backend deletion (tombstone plus counter increment) aligns it with GC's existing exclusive mainIndexGuard: the deletion either finishes before compaction and is cleared by that GC, or starts after the reset and remains counted for the next GC.

[razdoburdin]

got it. Thanks!