From 6892476aa96410cca93047b049d2ba4f36b1cfc5 Mon Sep 17 00:00:00 2001
From: sharonrosenfeld <sharon.rosenfeld@redis.com>
Date: Tue, 20 Oct 2026 08:57:55 +0300
Subject: [PATCH] [MOD-18471] Fix SVS marked-deletion GC race

---
 src/VecSim/algorithms/svs/svs.h | 16 +++++++++++-
 tests/unit/test_svs_tiered.cpp  | 45 +++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/src/VecSim/algorithms/svs/svs.h b/src/VecSim/algorithms/svs/svs.h
index 2b998dc11..0b9ecc794 100644
--- a/src/VecSim/algorithms/svs/svs.h
+++ b/src/VecSim/algorithms/svs/svs.h
@@ -79,6 +79,7 @@ struct SVSIndexBase
     // the checks preceding those calls rule out, and it grows its dataset rather than refusing a
     // write, so without this the code that reports a failed write would go untested.
     virtual void throwOnNextWriteForTest() = 0;
+    virtual void setAfterCompactCallbackForTest(std::function<void()> callback) = 0;
 #endif
 protected:
     // Index marked deleted vectors counter to initiate reindexing if it exceeds threshold
@@ -305,9 +306,13 @@ class SVSIndex : public VecSimIndexAbstract<svs_details::vecsim_dt<DataType>, fl
 #ifdef BUILD_TESTS
 public:
     void throwOnNextWriteForTest() override { throw_on_next_write_ = true; }
+    void setAfterCompactCallbackForTest(std::function<void()> callback) override {
+        after_compact_callback_ = std::move(callback);
+    }
 
 private:
     bool throw_on_next_write_ = false;
+    std::function<void()> after_compact_callback_;
     void maybeThrowForTest() {
         if (throw_on_next_write_) {
             throw_on_next_write_ = false;
@@ -896,12 +901,21 @@ class SVSIndex : public VecSimIndexAbstract<svs_details::vecsim_dt<DataType>, fl
     }
 
     void runGC() override {
+        // Only deletions already reflected in the counter when this GC starts belong to this
+        // run. A concurrent deletion may land after compact() has finished, so resetting the
+        // whole counter after compact() would lose that update.
+        const size_t marked_deleted_before_gc = num_marked_deleted.load(std::memory_order_relaxed);
         if (auto impl = getImpl()) {
             // concurrent compact() consolidates every soft-deleted points first
             // pending per-label consolidate jobs need not be drained - they become no-ops
             impl->compact();
         }
-        num_marked_deleted.store(0, std::memory_order_relaxed);
+#ifdef BUILD_TESTS
+        if (after_compact_callback_) {
+            after_compact_callback_();
+        }
+#endif
+        num_marked_deleted.fetch_sub(marked_deleted_before_gc, std::memory_order_relaxed);
     }
 
 private:
diff --git a/tests/unit/test_svs_tiered.cpp b/tests/unit/test_svs_tiered.cpp
index 6fabc1cf5..049ebaa8a 100644
--- a/tests/unit/test_svs_tiered.cpp
+++ b/tests/unit/test_svs_tiered.cpp
@@ -2814,6 +2814,51 @@ TYPED_TEST(SVSTieredIndexTestBasic, markedDeleted) {
     ASSERT_EQ(tiered_index->indexLabelCount(), 0);
 }
 
+TYPED_TEST(SVSTieredIndexTestBasic, deletionAfterCompactIsKeptInMarkedDeletedCount) {
+    constexpr size_t dim = 4;
+    constexpr size_t n = 2;
+    SVSParams params = {.type = TypeParam::get_index_type(), .dim = dim, .metric = VecSimMetric_L2};
+    VecSimParams svs_params = CreateParams(params);
+    auto mock_thread_pool = tieredIndexMock(1);
+    auto *tiered_index = this->CreateTieredSVSIndex(svs_params, mock_thread_pool, n, n);
+    ASSERT_INDEX(tiered_index);
+
+    for (labelType label = 0; label < n; ++label) {
+        GenerateAndAddVector<TEST_DATA_T>(tiered_index, dim, label);
+    }
+    while (!mock_thread_pool.jobQ.empty()) {
+        mock_thread_pool.thread_iteration();
+    }
+    ASSERT_EQ(tiered_index->GetBackendIndex()->indexSize(), n);
+
+    ASSERT_EQ(tiered_index->deleteVector(0), 1);
+    ASSERT_EQ(tiered_index->GetSVSIndex()->getNumMarkedDeleted(), 1);
+
+    bool deleted_after_compact = false;
+    tiered_index->GetSVSIndex()->setAfterCompactCallbackForTest([&] {
+        // The tiered deletion path does not take updateJobMutex or mainIndexGuard, so it can
+        // reach the backend in the window between compact() and the GC counter adjustment.
+        ASSERT_EQ(tiered_index->deleteVector(1), 1);
+        deleted_after_compact = true;
+    });
+
+    VecSimTieredIndex_GC(tiered_index);
+    while (!mock_thread_pool.jobQ.empty()) {
+        mock_thread_pool.thread_iteration();
+    }
+
+    ASSERT_TRUE(deleted_after_compact);
+    ASSERT_EQ(tiered_index->GetSVSIndex()->getNumMarkedDeleted(), 1);
+    ASSERT_FALSE(tiered_index->GetSVSIndex()->isLabelExists(1));
+
+    tiered_index->GetSVSIndex()->setAfterCompactCallbackForTest({});
+    VecSimTieredIndex_GC(tiered_index);
+    while (!mock_thread_pool.jobQ.empty()) {
+        mock_thread_pool.thread_iteration();
+    }
+    ASSERT_EQ(tiered_index->GetSVSIndex()->getNumMarkedDeleted(), 0);
+}
+
 TYPED_TEST(SVSTieredIndexTestBasic, deleteVectorMulti) {
     // Create TieredSVS index instance with a mock queue.
     size_t dim = 4;