From a44a91f5bc1a0f285115355ea200ef2652c9da85 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Wed, 12 Aug 2026 13:45:44 +0300 Subject: [PATCH 1/5] 17578-isolate-swap split remove and swap --- src/VecSim/algorithms/hnsw/hnsw.h | 20 ++++++---- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 50 ++++++++++++++---------- 2 files changed, 43 insertions(+), 27 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 2c1fae87f..86015b0da 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -205,7 +205,7 @@ class HNSWIndex : public VecSimIndexAbstract, idType id) const; void emplaceToHeap(vecsim_stl::abstract_priority_queue &heap, DistType dist, idType id) const; - void removeAndSwap(idType internalId); + void swapWithLast(idType removedId); size_t getVectorRelativeIndex(idType id) const { return id % this->blockSize; } @@ -279,6 +279,7 @@ class HNSWIndex : public VecSimIndexAbstract, void unmarkInProcess(idType internalId); HNSWAddVectorState storeNewElement(labelType label, const void *vector_data); void removeAndSwapMarkDeletedElement(idType internalId); + void removeFromGraph(idType internalId); void repairNodeConnections(idType node_id, size_t level); // For prefetching only. const ElementMetaData *getMetaDataAddress(idType internal_id) const { @@ -1644,7 +1645,7 @@ HNSWIndex::~HNSWIndex() { */ template -void HNSWIndex::removeAndSwap(idType internalId) { +void HNSWIndex::removeFromGraph(idType internalId) { // Sanity check - the id to remove cannot be the entry point, as it should have been replaced // upon marking it as deleted. assert(entrypointNode != internalId); @@ -1673,16 +1674,19 @@ void HNSWIndex::removeAndSwap(idType internalId) { // We can say now that the element has removed completely from index. --curElementCount; +} +template +void HNSWIndex::swapWithLast(idType removedId) { // Get the last element's metadata and data. - // If we are deleting the last element, we already destroyed it's metadata. + // If we are deleting the last element, we already destroyed its metadata. auto *last_element_data = getDataByInternalId(curElementCount); DataBlock &last_gd_block = graphDataBlocks.back(); auto last_element = (ElementGraphData *)last_gd_block.removeAndFetchLastElement(); // Swap the last id with the deleted one, and invalidate the last id data. - if (curElementCount != internalId) { - SwapLastIdWithDeletedId(internalId, last_element, last_element_data); + if (curElementCount != removedId) { + SwapLastIdWithDeletedId(removedId, last_element, last_element_data); } // If we need to free a complete block and there is at least one block between the @@ -1693,7 +1697,8 @@ void HNSWIndex::removeAndSwap(idType internalId) { template void HNSWIndex::removeAndSwapMarkDeletedElement(idType internalId) { - removeAndSwap(internalId); + removeFromGraph(internalId); + swapWithLast(internalId); // element is permanently removed from the index, it is no longer counted as marked deleted. --numMarkedDeleted; } @@ -1756,7 +1761,8 @@ void HNSWIndex::removeVectorInPlace(const idType element_int } // Finally, remove the element from the index and make a swap with the last internal id to // avoid fragmentation and reclaim memory when needed. - removeAndSwap(element_internal_id); + removeFromGraph(element_internal_id); + swapWithLast(element_internal_id); } // Store the new element in the global data structures and keep the new state. In multithreaded diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index a4d5e08e4..21aaad9de 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -104,7 +104,8 @@ class TieredHNSWIndex : public VecSimTieredIndex { // To be executed synchronously upon deleting a vector, doesn't require a wrapper. Main HNSW // lock is assumed to be held exclusive here. - void executeSwapJob(idType deleted_id, vecsim_stl::vector &idsToRemove); + void fixJobsAfterSwap(idType deleted_id, vecsim_stl::vector &idsToRemove); + void invalidateRepairJobs(idType deleted_id); // Execute the ready swap jobs, run no more than 'maxSwapsToRun' jobs (run all of them for -1). void executeReadySwapJobs(size_t maxSwapsToRun = -1); @@ -283,23 +284,11 @@ void TieredHNSWIndex::executeRepairJobWrapper(AsyncJob *job) } template -void TieredHNSWIndex::executeSwapJob(idType deleted_id, - vecsim_stl::vector &idsToRemove) { - // Get the id that was last and was had been swapped with the job's deleted id. +void TieredHNSWIndex::fixJobsAfterSwap( + idType deleted_id, vecsim_stl::vector &idsToRemove) { + // Get the id that was last and had been swapped with the job's deleted id. idType prev_last_id = this->getHNSWIndex()->indexSize(); - // Invalidate repair jobs for the disposed id (if exist), and update the associated swap jobs. - if (idToRepairJobs.find(deleted_id) != idToRepairJobs.end()) { - for (auto &job_it : idToRepairJobs.at(deleted_id)) { - job_it->node_id = this->setAndSaveInvalidJob(job_it); - for (auto &swap_job_it : job_it->associatedSwapJobs) { - if (swap_job_it->atomicDecreasePendingJobsNum() == 0) { - readySwapJobs++; - } - } - } - idToRepairJobs.erase(deleted_id); - } // Swap the ids in the pending jobs for the current last id (if exist). if (idToRepairJobs.find(prev_last_id) != idToRepairJobs.end()) { for (auto &job_it : idToRepairJobs.at(prev_last_id)) { @@ -324,6 +313,24 @@ void TieredHNSWIndex::executeSwapJob(idType deleted_id, } } +template +void TieredHNSWIndex::invalidateRepairJobs(idType deleted_id) { + // Invalidate repair jobs for the disposed id (if exist), and update the associated swap jobs. + if (idToRepairJobs.find(deleted_id) == idToRepairJobs.end()) { + return; + } + + for (auto &job_it : idToRepairJobs.at(deleted_id)) { + job_it->node_id = this->setAndSaveInvalidJob(job_it); + for (auto &swap_job_it : job_it->associatedSwapJobs) { + if (swap_job_it->atomicDecreasePendingJobsNum() == 0) { + readySwapJobs++; + } + } + } + idToRepairJobs.erase(deleted_id); +} + template HNSWIndex *TieredHNSWIndex::getHNSWIndex() const { return dynamic_cast *>(this->backendIndex); @@ -342,10 +349,12 @@ void TieredHNSWIndex::executeReadySwapJobs(size_t maxJobsToR idsToRemove.reserve(idToSwapJob.size()); for (auto &it : idToSwapJob) { auto *swap_job = it.second; + // Swap job is ready for execution - execute and delete it. if (swap_job->pending_repair_jobs_counter.load() == 0) { - // Swap job is ready for execution - execute and delete it. - this->getHNSWIndex()->removeAndSwapMarkDeletedElement(swap_job->deleted_id); - this->executeSwapJob(swap_job->deleted_id, idsToRemove); + auto deleted_id = swap_job->deleted_id; + this->getHNSWIndex()->removeAndSwapMarkDeletedElement(deleted_id); + this->invalidateRepairJobs(deleted_id); + this->fixJobsAfterSwap(deleted_id, idsToRemove); delete swap_job; } if (maxJobsToRun > 0 && idsToRemove.size() >= maxJobsToRun) { @@ -533,7 +542,8 @@ int TieredHNSWIndex::deleteLabelFromHNSWInplace(labelType la // Get the id in every iteration, since the ids can be swapped in every iteration. idType id = hnsw_index->getElementIds(label).at(id_ind); hnsw_index->removeVectorInPlace(id); - this->executeSwapJob(id, idsToRemove); + this->invalidateRepairJobs(id); + this->fixJobsAfterSwap(id, idsToRemove); } hnsw_index->removeLabel(label); for (idType id : idsToRemove) { From f25f84c0a56536abc7919c4952c40e3944d089ab Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Thu, 13 Aug 2026 15:48:05 +0300 Subject: [PATCH 2/5] Isolate a deleted element from the graph once its repairs are done 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) --- src/VecSim/algorithms/hnsw/hnsw.h | 96 ++++++++++++++- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 35 +++++- .../hnsw/hnsw_tiered_tests_friends.h | 1 + tests/unit/test_hnsw_tiered.cpp | 110 +++++++++++++----- 4 files changed, 206 insertions(+), 36 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 86015b0da..a16c6e923 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -280,6 +280,25 @@ class HNSWIndex : public VecSimIndexAbstract, HNSWAddVectorState storeNewElement(labelType label, const void *vector_data); void removeAndSwapMarkDeletedElement(idType internalId); void removeFromGraph(idType internalId); + // Remove `id` from `level_data`'s links if it is still there. Unlike `removeLink`, tolerates its + // absence - an element that was already isolated (see `isolateDeletedElement`) holds no links at + // all, and a repair job running in parallel may have dropped the edge too. + static void removeLinkIfExists(ElementLevelData &level_data, idType id) { + for (size_t i = 0; i < level_data.getNumLinks(); i++) { + if (level_data.getLinkAtPos(i) == id) { + level_data.removeLink(id); + return; + } + } + } + // Take a marked-deleted element out of the graph entirely: drop every edge going out of it and + // every edge coming into it, at every level, keeping the bookkeeping of the other side + // consistent. To be called once all the repair jobs created for its deletion are done and + // before its swap job disposes of it, so that from that point no element refers to it and it + // refers to no element. + // Takes the per-element links locks (one at a time), so holding the main index guard for shared + // ownership is enough. + void isolateDeletedElement(idType internalId); void repairNodeConnections(idType node_id, size_t level); // For prefetching only. const ElementMetaData *getMetaDataAddress(idType internal_id) const { @@ -952,7 +971,8 @@ void HNSWIndex::repairConnectionsForDeletion( if (isMarkedDeleted(neighbour_id)) { // Just remove the deleted element from the neighbor's neighbors list. No need to repair as // this change is temporary, this neighbor is about to be removed from the graph as well. - neighbor_level.removeLink(element_internal_id); + // The link may already be gone if this neighbor was isolated upon completing its repairs. + removeLinkIfExists(neighbor_level, element_internal_id); return; } @@ -1542,7 +1562,20 @@ void HNSWIndex::mutuallyRemoveNeighborAtPos(ElementLevelData // mutually, so it should be sufficient to look at the removed node's incoming edges set // alone. if (!removed_node_level.removeIncomingUnidirectionalEdgeIfExists(node_id)) { - node_level.newIncomingUnidirectionalEdge(removed_node); + // No record of this edge on the other side. Normally that means the edge was bidirectional, + // but it also happens when the removed node was already isolated upon completing its repairs + // (`isolateDeletedElement` clears its incoming edges set together with its links), so check + // whether it actually points back before recording the remaining direction. + bool points_back = false; + for (size_t i = 0; i < removed_node_level.getNumLinks(); i++) { + if (removed_node_level.getLinkAtPos(i) == node_id) { + points_back = true; + break; + } + } + if (points_back) { + node_level.newIncomingUnidirectionalEdge(removed_node); + } } } @@ -1644,6 +1677,57 @@ HNSWIndex::~HNSWIndex() { * Index API functions */ +template +void HNSWIndex::isolateDeletedElement(idType internalId) { + assert(isMarkedDeleted(internalId) && "Only a marked-deleted element may be isolated"); + auto element = getGraphDataByInternalId(internalId); + for (size_t level = 0; level <= element->toplevel; level++) { + // Take the element's edges at this level, and drop its own side of them right away - from + // here on it points to nothing, so a repair job that runs for it later finds no deleted + // neighbour and returns without touching it. + lockNodeLinks(internalId); + ElementLevelData &level_data = getElementLevelData(element, level); + auto neighbours = level_data.copyLinks(); + std::vector incoming_edges(level_data.getIncomingEdges().begin(), + level_data.getIncomingEdges().end()); + level_data.setNumLinks(0); + for (idType incoming_id : incoming_edges) { + level_data.removeIncomingUnidirectionalEdgeIfExists(incoming_id); + } + unlockNodeLinks(internalId); + + // Drop the other side of the outgoing edges: the neighbour either recorded this element as + // an incoming unidirectional edge (the expected case, as no element points to it anymore), + // or still points back at it - an incoming edge whose repair job never ran, which is dropped + // here as well. + for (idType neighbour_id : neighbours) { + lockNodeLinks(neighbour_id); + ElementLevelData &neighbour = getElementLevelData(neighbour_id, level); + if (!neighbour.removeIncomingUnidirectionalEdgeIfExists(internalId)) { + // No record on the neighbour's side, so it still points back here. Every *live* + // element that pointed at this one had a repair job that removed its edge before + // this point, so this is an edge between two deleted elements - neither of them + // gets a repair job for the other, hence it is dropped here. + assert(isMarkedDeleted(neighbour_id) && + "a live element still points to a fully repaired deleted element"); + removeLinkIfExists(neighbour, internalId); + } + unlockNodeLinks(neighbour_id); + } + + // Same for any incoming edge that is still registered: remove it from its origin's links. + for (idType incoming_id : incoming_edges) { + // Same here: an incoming edge that survived all the repair jobs comes from another + // deleted element. + assert(isMarkedDeleted(incoming_id) && + "a live element still points to a fully repaired deleted element"); + lockNodeLinks(incoming_id); + removeLinkIfExists(getElementLevelData(incoming_id, level), internalId); + unlockNodeLinks(incoming_id); + } + } +} + template void HNSWIndex::removeFromGraph(idType internalId) { // Sanity check - the id to remove cannot be the entry point, as it should have been replaced @@ -1651,14 +1735,16 @@ void HNSWIndex::removeFromGraph(idType internalId) { assert(entrypointNode != internalId); auto element = getGraphDataByInternalId(internalId); - // Remove the deleted id form the relevant incoming edges sets in which it appears. + // Remove the deleted id form the relevant incoming edges sets in which it appears. For an + // asynchronously deleted element there is nothing to walk here: `isolateDeletedElement` already + // took all of its edges out when its last repair job completed. for (size_t level = 0; level <= element->toplevel; level++) { ElementLevelData &cur_level = getElementLevelData(element, level); for (size_t i = 0; i < cur_level.getNumLinks(); i++) { ElementLevelData &neighbour = getElementLevelData(cur_level.getLinkAtPos(i), level); - // Note that in case of in-place delete, we might have not accounted for this edge in + // Note that in case of in-place delete, we might have not accounted for this edge // in the unidirectional edges, since there is no point in keeping it there temporarily - // (we know we will get here and remove this deleted id permanently). + // . (We know we will get here and remove this deleted id permanently.) // However, upon asynchronous delete, this should always succeed since we do update // the incoming edges in the mutual update even for deleted elements. bool res = neighbour.removeIncomingUnidirectionalEdgeIfExists(internalId); diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index 21aaad9de..0fbe190ac 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -129,6 +129,12 @@ class TieredHNSWIndex : public VecSimTieredIndex { // while *HNSW shared lock is held* (shared locked). int deleteLabelFromHNSW(labelType label); + // Take a deleted element out of the graph once the last repair job created for its deletion is + // done - no edge in or out of it is left, so the swap job that disposes of it later only has to + // reclaim its slot. Called in the repair context, while the main index guard is held for shared + // ownership and `idToRepairJobsGuard` is not held. + void isolateRepairedElement(idType deleted_id); + // Insert a single vector to HNSW. This can be called in both write modes - insert async and // in-place. For the async mode, we have to release the flat index guard that is held for shared // ownership (we do it right after we update the HNSW global data and receive the new state). @@ -336,6 +342,11 @@ HNSWIndex *TieredHNSWIndex::getHNSWIndex return dynamic_cast *>(this->backendIndex); } +template +void TieredHNSWIndex::isolateRepairedElement(idType deleted_id) { + this->getHNSWIndex()->isolateDeletedElement(deleted_id); +} + template void TieredHNSWIndex::executeReadySwapJobs(size_t maxJobsToRun) { @@ -424,6 +435,13 @@ int TieredHNSWIndex::deleteLabelFromHNSW(labelType label) { } this->idToRepairJobsGuard.unlock(); + if (incomingEdges.size() == 0) { + // No repair job will ever run for this element, so this is already the point at which + // it can be taken out of the graph (outside the repair jobs guard, as isolating takes + // the per-element links locks). + this->isolateRepairedElement(id); + } + this->submitJobs(repair_jobs); // Insert the swap job into the swap jobs lookup (for fast update in case that the // node id is changed due to swap job). @@ -649,14 +667,29 @@ void TieredHNSWIndex::executeRepairJob(HNSWRepairJob *job) { *it = repair_jobs.back(); repair_jobs.pop_back(); } + this->idToRepairJobsGuard.unlock(); + + hnsw_index->repairNodeConnections(job->node_id, job->level); + + // Account for this job only now that its repair has actually been performed. Decreasing the + // counter before the repair would let a swap job be seen as ready - and an element be isolated - + // while an element still points to it from a repair that has not run yet. + vecsim_stl::vector fully_repaired_ids(this->allocator); + this->idToRepairJobsGuard.lock(); for (auto &it : job->associatedSwapJobs) { if (it->atomicDecreasePendingJobsNum() == 0) { readySwapJobs++; + fully_repaired_ids.push_back(it->deleted_id); } } this->idToRepairJobsGuard.unlock(); - hnsw_index->repairNodeConnections(job->node_id, job->level); + // These deleted elements have no pending repair job left, so no element points to them anymore - + // take them out of the graph entirely, leaving no edge in or out for the swap job to deal with. + // Done outside the repair jobs guard, as isolating takes the per-element links locks. + for (idType deleted_id : fully_repaired_ids) { + this->isolateRepairedElement(deleted_id); + } this->mainIndexGuard.unlock_shared(); } diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h b/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h index 21f99f8f5..d4d5cd999 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h @@ -24,6 +24,7 @@ INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_deleteVectorAndRepairAsync_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_alternateInsertDeleteAsync_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_swapJobBasic_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_swapJobBasic2_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_invalidRepairJobOnSwap_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_deleteVectorsAndSwapSync_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_BatchIterator_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_BatchIteratorAdvanced_Test) diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index 21f504177..d7f816c9a 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -1888,18 +1888,18 @@ TYPED_TEST(HNSWTieredIndexTest, swapJobBasic2) { ASSERT_EQ(mock_thread_pool.jobQ.front().job->jobType, HNSW_REPAIR_NODE_CONNECTIONS_JOB); mock_thread_pool.thread_iteration(); EXPECT_EQ(tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load(), 0); - // Delete 2, expect to create two repair job pending from 0 and 1. Also, expect that swap - // job for 0 will be executed, so that 2 and 0 are swapped. Then, we should have only 1 - // pending repair job for the "new" 0 - for deleting the old 1->2, while the second job for - // deleting the old 0->2 is invalid and reduced from the pending repair jobs counter. + // Delete 2. Only 1 still points to it: 0 was taken out of the graph when its own repair jobs + // completed, so it has no links left and generates no repair job here. Also, expect that swap + // job for 0 will be executed, so that 2 and 0 are swapped, and the single pending repair job + // is for the "new" 0 - for deleting the old 1->2. EXPECT_EQ(tiered_index->deleteVector(2), 1); EXPECT_EQ(tiered_index->indexSize(), 2); EXPECT_EQ(tiered_index->getHNSWIndex()->getNumMarkedDeleted(), 1); EXPECT_EQ(tiered_index->statisticInfo().numberOfMarkedDeleted, 1); EXPECT_EQ(tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load(), 1); - EXPECT_EQ(mock_thread_pool.jobQ.size(), 2); - // The first repair job should remove 1->0 (originally was 1->2). + EXPECT_EQ(mock_thread_pool.jobQ.size(), 1); + // The repair job should remove 1->0 (originally was 1->2). ASSERT_EQ(mock_thread_pool.jobQ.front().job->jobType, HNSW_REPAIR_NODE_CONNECTIONS_JOB); ASSERT_EQ(reinterpret_cast(mock_thread_pool.jobQ.front().job)->node_id, 1); ASSERT_EQ(reinterpret_cast(mock_thread_pool.jobQ.front().job) @@ -1908,34 +1908,15 @@ TYPED_TEST(HNSWTieredIndexTest, swapJobBasic2) { 0); mock_thread_pool.thread_iteration(); EXPECT_EQ(tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load(), 0); - // The second repair job is invalid due to the removal of (the original) 0. - ASSERT_EQ(mock_thread_pool.jobQ.front().job->jobType, HNSW_REPAIR_NODE_CONNECTIONS_JOB); - ASSERT_EQ(mock_thread_pool.jobQ.front().job->isValid, false); - ASSERT_EQ(reinterpret_cast(mock_thread_pool.jobQ.front().job)->node_id, - invalid_jobs_counter++); - ASSERT_EQ(reinterpret_cast(mock_thread_pool.jobQ.front().job) - ->associatedSwapJobs[0] - ->deleted_id, - 0); - mock_thread_pool.thread_iteration(); - // Delete 1, that should still have 0->1 edge that should be repaired. This should cause - // the swap and removal of 0 (that has no more pending jobs at that point) - so that 1 would - // get id 0, and then the new 0 should have no pending repair jobs. + // Delete 1. The only other element left (the "new" 0, which is the old 2) is deleted and was + // already taken out of the graph, so nothing points to 1 and no repair job is created for it. + // Its swap job is therefore ready right away, and the swap and removal of the previous 0 is + // triggered - so that 1 gets id 0. EXPECT_EQ(tiered_index->deleteVector(1), 1); - EXPECT_EQ(mock_thread_pool.jobQ.size(), 1); + EXPECT_EQ(mock_thread_pool.jobQ.size(), 0); EXPECT_EQ(tiered_index->idToSwapJob.size(), 1); EXPECT_EQ(tiered_index->idToSwapJob.at(0)->deleted_id, 0); EXPECT_EQ(tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load(), 0); - // The repair job is invalid due to the removal of (the previous) 0. - ASSERT_EQ(mock_thread_pool.jobQ.front().job->jobType, HNSW_REPAIR_NODE_CONNECTIONS_JOB); - ASSERT_EQ(mock_thread_pool.jobQ.front().job->isValid, false); - ASSERT_EQ(reinterpret_cast(mock_thread_pool.jobQ.front().job)->node_id, - invalid_jobs_counter); - ASSERT_EQ(reinterpret_cast(mock_thread_pool.jobQ.front().job) - ->associatedSwapJobs[0] - ->deleted_id, - 0); - mock_thread_pool.thread_iteration(); EXPECT_EQ(tiered_index->indexSize(), 1); EXPECT_EQ(tiered_index->getHNSWIndex()->getNumMarkedDeleted(), 1); EXPECT_EQ(tiered_index->statisticInfo().numberOfMarkedDeleted, 1); @@ -1949,6 +1930,75 @@ TYPED_TEST(HNSWTieredIndexTest, swapJobBasic2) { EXPECT_EQ(tiered_index->statisticInfo().numberOfMarkedDeleted, 0); } +// Covers the invalidation of a pending repair job whose node is disposed of by a swap job. +// A deleted element is taken out of the graph as soon as *its own* repair jobs are done, so to have +// a pending repair job at the time it is disposed of, the job has to belong to *another* element's +// deletion: element 0 is deleted first, then element 1 is deleted while 0 still points to it (which +// registers a repair job for node 0), and only then 0 completes its own repairs and is swapped out. +TYPED_TEST(HNSWTieredIndexTest, invalidRepairJobOnSwap) { + size_t dim = 4; + HNSWParams params = {.type = TypeParam::get_index_type(), + .dim = dim, + .metric = VecSimMetric_L2, + .multi = TypeParam::isMulti()}; + VecSimParams hnsw_params = CreateParams(params); + auto mock_thread_pool = tieredIndexMock(); + // Threshold of 1, so that a ready swap job is executed at the first opportunity. + auto *tiered_index = this->CreateTieredHNSWIndex(hnsw_params, mock_thread_pool, 1); + + // Insert 3 vectors directly into HNSW, expect to have a fully connected graph. + for (size_t i = 0; i < 3; i++) { + GenerateAndAddVector(tiered_index->backendIndex, dim, i, i); + } + + // Delete 0 - a repair job is created for every (node, level) pair that points to it. Note that + // the number of jobs depends on the levels the elements got, so the counters are compared to each + // other rather than to fixed values below. + EXPECT_EQ(tiered_index->deleteVector(0), 1); + ASSERT_GT(mock_thread_pool.jobQ.size(), 0); + ASSERT_GT(tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load(), 0); + + // Delete 1 before those jobs run. 0 is deleted but still connected (its own repairs are pending), + // so a repair job for node 0 is created here and stays pending. + EXPECT_EQ(tiered_index->deleteVector(1), 1); + ASSERT_TRUE(tiered_index->idToRepairJobs.contains(0)); + + // Execute 0's repair jobs, so that it has no pending repair job left and is taken out of the + // graph, making its swap job ready. Its own pending repair job (for deleting 1) is still queued. + while (tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load() > 0) { + ASSERT_GT(mock_thread_pool.jobQ.size(), 0); + mock_thread_pool.thread_iteration(); + } + ASSERT_TRUE(tiered_index->idToRepairJobs.contains(0)); + ASSERT_EQ(tiered_index->invalidJobs.size(), 0); + int pending_for_1 = tiered_index->idToSwapJob.at(1)->pending_repair_jobs_counter.load(); + ASSERT_GT(pending_for_1, 0); + + // Dispose of 0. The repair job that is still pending for it has to be invalidated, and 1's swap + // job should stop waiting for that job. + tiered_index->runGC(); + EXPECT_EQ(tiered_index->indexSize(), 2); + EXPECT_EQ(tiered_index->invalidJobs.size(), 1); + EXPECT_EQ(tiered_index->idToSwapJob.at(1)->pending_repair_jobs_counter.load(), + pending_for_1 - 1); + + // Drain the remaining jobs: the invalid one is disposed of without being executed, and the valid + // ones complete 1's repairs - so 1 is taken out of the graph as well. + while (!mock_thread_pool.jobQ.empty()) { + mock_thread_pool.thread_iteration(); + } + EXPECT_EQ(tiered_index->invalidJobs.size(), 0); + EXPECT_EQ(tiered_index->idToSwapJob.at(1)->pending_repair_jobs_counter.load(), 0); + + // Disposing of 1 as well leaves a single element in the index, with a valid graph. + tiered_index->runGC(); + EXPECT_EQ(tiered_index->indexSize(), 1); + EXPECT_EQ(tiered_index->getHNSWIndex()->getNumMarkedDeleted(), 0); + auto state = tiered_index->getHNSWIndex()->checkIntegrity(); + EXPECT_EQ(state.valid_state, true); + EXPECT_EQ(state.connections_to_repair, 0); +} + // A set of lambdas that determine whether a vector should be inserted to the // HNSW index (returns true) or to the flat index (returns false). inline constexpr std::array, 11> lambdas = {{ From 25a744b002ab1b67ede6679dc99615116ff18f64 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Thu, 13 Aug 2026 19:13:59 +0300 Subject: [PATCH 3/5] Fix formatting (clang-format 18) Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/algorithms/hnsw/hnsw.h | 18 +++++++++--------- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 8 ++++---- tests/unit/test_hnsw_tiered.cpp | 15 ++++++++------- 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index a16c6e923..16a267f95 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -280,9 +280,9 @@ class HNSWIndex : public VecSimIndexAbstract, HNSWAddVectorState storeNewElement(labelType label, const void *vector_data); void removeAndSwapMarkDeletedElement(idType internalId); void removeFromGraph(idType internalId); - // Remove `id` from `level_data`'s links if it is still there. Unlike `removeLink`, tolerates its - // absence - an element that was already isolated (see `isolateDeletedElement`) holds no links at - // all, and a repair job running in parallel may have dropped the edge too. + // Remove `id` from `level_data`'s links if it is still there. Unlike `removeLink`, tolerates + // its absence - an element that was already isolated (see `isolateDeletedElement`) holds no + // links at all, and a repair job running in parallel may have dropped the edge too. static void removeLinkIfExists(ElementLevelData &level_data, idType id) { for (size_t i = 0; i < level_data.getNumLinks(); i++) { if (level_data.getLinkAtPos(i) == id) { @@ -1563,9 +1563,9 @@ void HNSWIndex::mutuallyRemoveNeighborAtPos(ElementLevelData // alone. if (!removed_node_level.removeIncomingUnidirectionalEdgeIfExists(node_id)) { // No record of this edge on the other side. Normally that means the edge was bidirectional, - // but it also happens when the removed node was already isolated upon completing its repairs - // (`isolateDeletedElement` clears its incoming edges set together with its links), so check - // whether it actually points back before recording the remaining direction. + // but it also happens when the removed node was already isolated: `isolateDeletedElement` + // clears its incoming edges set together with its links. Check whether it actually points + // back before recording the remaining direction. bool points_back = false; for (size_t i = 0; i < removed_node_level.getNumLinks(); i++) { if (removed_node_level.getLinkAtPos(i) == node_id) { @@ -1689,7 +1689,7 @@ void HNSWIndex::isolateDeletedElement(idType internalId) { ElementLevelData &level_data = getElementLevelData(element, level); auto neighbours = level_data.copyLinks(); std::vector incoming_edges(level_data.getIncomingEdges().begin(), - level_data.getIncomingEdges().end()); + level_data.getIncomingEdges().end()); level_data.setNumLinks(0); for (idType incoming_id : incoming_edges) { level_data.removeIncomingUnidirectionalEdgeIfExists(incoming_id); @@ -1698,8 +1698,8 @@ void HNSWIndex::isolateDeletedElement(idType internalId) { // Drop the other side of the outgoing edges: the neighbour either recorded this element as // an incoming unidirectional edge (the expected case, as no element points to it anymore), - // or still points back at it - an incoming edge whose repair job never ran, which is dropped - // here as well. + // or still points back at it - an incoming edge whose repair job never ran, which is + // dropped here as well. for (idType neighbour_id : neighbours) { lockNodeLinks(neighbour_id); ElementLevelData &neighbour = getElementLevelData(neighbour_id, level); diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index 0fbe190ac..b90ba8e69 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -672,8 +672,8 @@ void TieredHNSWIndex::executeRepairJob(HNSWRepairJob *job) { hnsw_index->repairNodeConnections(job->node_id, job->level); // Account for this job only now that its repair has actually been performed. Decreasing the - // counter before the repair would let a swap job be seen as ready - and an element be isolated - - // while an element still points to it from a repair that has not run yet. + // counter beforehand would let a swap job be seen as ready, and its element isolated, while + // an element still points to it from a repair that has not run yet. vecsim_stl::vector fully_repaired_ids(this->allocator); this->idToRepairJobsGuard.lock(); for (auto &it : job->associatedSwapJobs) { @@ -684,8 +684,8 @@ void TieredHNSWIndex::executeRepairJob(HNSWRepairJob *job) { } this->idToRepairJobsGuard.unlock(); - // These deleted elements have no pending repair job left, so no element points to them anymore - - // take them out of the graph entirely, leaving no edge in or out for the swap job to deal with. + // These deleted elements have no pending repair job left, so nothing points to them anymore. + // Take them out of the graph entirely, leaving no edge in or out for the swap job to deal with. // Done outside the repair jobs guard, as isolating takes the per-element links locks. for (idType deleted_id : fully_repaired_ids) { this->isolateRepairedElement(deleted_id); diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index d7f816c9a..a348ee65a 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -1952,19 +1952,20 @@ TYPED_TEST(HNSWTieredIndexTest, invalidRepairJobOnSwap) { } // Delete 0 - a repair job is created for every (node, level) pair that points to it. Note that - // the number of jobs depends on the levels the elements got, so the counters are compared to each - // other rather than to fixed values below. + // the number of jobs depends on the levels the elements got, so the counters are compared to + // each other rather than to fixed values below. EXPECT_EQ(tiered_index->deleteVector(0), 1); ASSERT_GT(mock_thread_pool.jobQ.size(), 0); ASSERT_GT(tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load(), 0); - // Delete 1 before those jobs run. 0 is deleted but still connected (its own repairs are pending), - // so a repair job for node 0 is created here and stays pending. + // Delete 1 before those jobs run. 0 is deleted but still connected (its own repairs are + // pending), so a repair job for node 0 is created here and stays pending. EXPECT_EQ(tiered_index->deleteVector(1), 1); ASSERT_TRUE(tiered_index->idToRepairJobs.contains(0)); // Execute 0's repair jobs, so that it has no pending repair job left and is taken out of the - // graph, making its swap job ready. Its own pending repair job (for deleting 1) is still queued. + // graph, making its swap job ready. Its own pending repair job (for deleting 1) is still + // queued. while (tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load() > 0) { ASSERT_GT(mock_thread_pool.jobQ.size(), 0); mock_thread_pool.thread_iteration(); @@ -1982,8 +1983,8 @@ TYPED_TEST(HNSWTieredIndexTest, invalidRepairJobOnSwap) { EXPECT_EQ(tiered_index->idToSwapJob.at(1)->pending_repair_jobs_counter.load(), pending_for_1 - 1); - // Drain the remaining jobs: the invalid one is disposed of without being executed, and the valid - // ones complete 1's repairs - so 1 is taken out of the graph as well. + // Drain the remaining jobs: the invalid one is disposed of without being executed, and the + // valid ones complete 1's repairs - so 1 is taken out of the graph as well. while (!mock_thread_pool.jobQ.empty()) { mock_thread_pool.thread_iteration(); } From a845c55df31dc144edc480c7ffc1b242694277cf Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Tue, 18 Aug 2026 22:31:38 +0300 Subject: [PATCH 4/5] Remove both sides of an edge mutually when isolating an element Take the two elements' links locks in ascending id order and drop both sides of the edge in one step, instead of clearing the isolated element's own records first and fixing the other side afterwards. Removing the edge mutually leaves no observable point at which an edge is recorded on neither side, which is what `mutuallyRemoveNeighborAtPos` relies on when it tells a bidirectional edge from a unidirectional one by the record alone. It can therefore go back to its original form, without the links scan that the previous version needed - a scan on a path taken by every insertion and every repair. For the same reason `repairConnectionsForDeletion` can assume again that a deleted neighbour still holds the link it is about to remove. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/algorithms/hnsw/hnsw.h | 114 +++++++++++++++--------------- 1 file changed, 57 insertions(+), 57 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 16a267f95..581bca7e7 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -280,16 +280,14 @@ class HNSWIndex : public VecSimIndexAbstract, HNSWAddVectorState storeNewElement(labelType label, const void *vector_data); void removeAndSwapMarkDeletedElement(idType internalId); void removeFromGraph(idType internalId); - // Remove `id` from `level_data`'s links if it is still there. Unlike `removeLink`, tolerates - // its absence - an element that was already isolated (see `isolateDeletedElement`) holds no - // links at all, and a repair job running in parallel may have dropped the edge too. - static void removeLinkIfExists(ElementLevelData &level_data, idType id) { + // Whether `level_data` holds a link to `id`. + static bool hasLink(const ElementLevelData &level_data, idType id) { for (size_t i = 0; i < level_data.getNumLinks(); i++) { if (level_data.getLinkAtPos(i) == id) { - level_data.removeLink(id); - return; + return true; } } + return false; } // Take a marked-deleted element out of the graph entirely: drop every edge going out of it and // every edge coming into it, at every level, keeping the bookkeeping of the other side @@ -971,8 +969,7 @@ void HNSWIndex::repairConnectionsForDeletion( if (isMarkedDeleted(neighbour_id)) { // Just remove the deleted element from the neighbor's neighbors list. No need to repair as // this change is temporary, this neighbor is about to be removed from the graph as well. - // The link may already be gone if this neighbor was isolated upon completing its repairs. - removeLinkIfExists(neighbor_level, element_internal_id); + neighbor_level.removeLink(element_internal_id); return; } @@ -1562,20 +1559,7 @@ void HNSWIndex::mutuallyRemoveNeighborAtPos(ElementLevelData // mutually, so it should be sufficient to look at the removed node's incoming edges set // alone. if (!removed_node_level.removeIncomingUnidirectionalEdgeIfExists(node_id)) { - // No record of this edge on the other side. Normally that means the edge was bidirectional, - // but it also happens when the removed node was already isolated: `isolateDeletedElement` - // clears its incoming edges set together with its links. Check whether it actually points - // back before recording the remaining direction. - bool points_back = false; - for (size_t i = 0; i < removed_node_level.getNumLinks(); i++) { - if (removed_node_level.getLinkAtPos(i) == node_id) { - points_back = true; - break; - } - } - if (points_back) { - node_level.newIncomingUnidirectionalEdge(removed_node); - } + node_level.newIncomingUnidirectionalEdge(removed_node); } } @@ -1682,49 +1666,65 @@ void HNSWIndex::isolateDeletedElement(idType internalId) { assert(isMarkedDeleted(internalId) && "Only a marked-deleted element may be isolated"); auto element = getGraphDataByInternalId(internalId); for (size_t level = 0; level <= element->toplevel; level++) { - // Take the element's edges at this level, and drop its own side of them right away - from - // here on it points to nothing, so a repair job that runs for it later finds no deleted - // neighbour and returns without touching it. + // Collect the elements this one shares an edge with at this level, in either direction. No + // edge can be added to a deleted element, so this set only shrinks from here on (a repair + // job of another element may still remove an edge concurrently). lockNodeLinks(internalId); ElementLevelData &level_data = getElementLevelData(element, level); - auto neighbours = level_data.copyLinks(); - std::vector incoming_edges(level_data.getIncomingEdges().begin(), - level_data.getIncomingEdges().end()); - level_data.setNumLinks(0); - for (idType incoming_id : incoming_edges) { - level_data.removeIncomingUnidirectionalEdgeIfExists(incoming_id); - } + auto others = level_data.copyLinks(); + others.insert(others.end(), level_data.getIncomingEdges().begin(), + level_data.getIncomingEdges().end()); unlockNodeLinks(internalId); - // Drop the other side of the outgoing edges: the neighbour either recorded this element as - // an incoming unidirectional edge (the expected case, as no element points to it anymore), - // or still points back at it - an incoming edge whose repair job never ran, which is - // dropped here as well. - for (idType neighbour_id : neighbours) { - lockNodeLinks(neighbour_id); - ElementLevelData &neighbour = getElementLevelData(neighbour_id, level); - if (!neighbour.removeIncomingUnidirectionalEdgeIfExists(internalId)) { - // No record on the neighbour's side, so it still points back here. Every *live* - // element that pointed at this one had a repair job that removed its edge before - // this point, so this is an edge between two deleted elements - neither of them - // gets a repair job for the other, hence it is dropped here. - assert(isMarkedDeleted(neighbour_id) && + for (idType other_id : others) { + // Remove both sides of the edge as one atomic step, holding the two elements' locks in + // ascending id order (as every other multi-lock site here does, to avoid deadlocks). + // Doing it mutually keeps the "an edge is recorded on exactly one side" invariant true + // at every observable point, which is what lets `mutuallyRemoveNeighborAtPos` tell a + // bidirectional edge from a unidirectional one by the record alone. + idType first = std::min(internalId, other_id); + idType second = std::max(internalId, other_id); + lockNodeLinks(first); + lockNodeLinks(second); + + ElementLevelData &other = getElementLevelData(other_id, level); + bool points_to_other = hasLink(level_data, other_id); + bool other_points_here = hasLink(other, internalId); + + if (points_to_other && other_points_here) { + // Bidirectional, so neither side recorded it as an incoming edge - just drop both + // links. Only two deleted elements can still point at each other at this stage: + // neither of them gets a repair job for the other's deletion. + assert(isMarkedDeleted(other_id) && + "a live element still points to a fully repaired deleted element"); + level_data.removeLink(other_id); + other.removeLink(internalId); + } else if (points_to_other) { + // Unidirectional out - the other side recorded it as an incoming edge. + level_data.removeLink(other_id); + bool res = other.removeIncomingUnidirectionalEdgeIfExists(internalId); + (void)res; + assert(res && "The edge should be in the incoming unidirectional edges"); + } else if (other_points_here) { + // Unidirectional in - recorded as an incoming edge here. As above, at this stage it + // can only come from another deleted element. + assert(isMarkedDeleted(other_id) && "a live element still points to a fully repaired deleted element"); - removeLinkIfExists(neighbour, internalId); + other.removeLink(internalId); + bool res = level_data.removeIncomingUnidirectionalEdgeIfExists(other_id); + (void)res; + assert(res && "The edge should be in the incoming unidirectional edges"); } - unlockNodeLinks(neighbour_id); - } + // Else the edge is already gone - a repair job of another element got to it first. - // Same for any incoming edge that is still registered: remove it from its origin's links. - for (idType incoming_id : incoming_edges) { - // Same here: an incoming edge that survived all the repair jobs comes from another - // deleted element. - assert(isMarkedDeleted(incoming_id) && - "a live element still points to a fully repaired deleted element"); - lockNodeLinks(incoming_id); - removeLinkIfExists(getElementLevelData(incoming_id, level), internalId); - unlockNodeLinks(incoming_id); + unlockNodeLinks(second); + unlockNodeLinks(first); } + + lockNodeLinks(internalId); + assert(level_data.getNumLinks() == 0 && level_data.getIncomingEdges().empty() && + "the element should have no edge left at this level"); + unlockNodeLinks(internalId); } } From 6eee6e1d48fc1aa1362a933e607e51fba8ef6b04 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Tue, 18 Aug 2026 22:46:16 +0300 Subject: [PATCH 5/5] Denote repair jobs as u->v in the tiered tests comments A repair job involves two elements - the node whose links are fixed and the deleted neighbour whose edge is removed - so naming it by only one of them ("a repair job for node 0") reads ambiguously. Name it by the edge it removes instead, as the surrounding test already does ("should remove 1->0"). Co-Authored-By: Claude Opus 5 (1M context) --- tests/unit/test_hnsw_tiered.cpp | 44 ++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index a348ee65a..a0927790b 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -1888,10 +1888,10 @@ TYPED_TEST(HNSWTieredIndexTest, swapJobBasic2) { ASSERT_EQ(mock_thread_pool.jobQ.front().job->jobType, HNSW_REPAIR_NODE_CONNECTIONS_JOB); mock_thread_pool.thread_iteration(); EXPECT_EQ(tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load(), 0); - // Delete 2. Only 1 still points to it: 0 was taken out of the graph when its own repair jobs - // completed, so it has no links left and generates no repair job here. Also, expect that swap - // job for 0 will be executed, so that 2 and 0 are swapped, and the single pending repair job - // is for the "new" 0 - for deleting the old 1->2. + // Delete 2. Only the 1->2 edge is left to repair: 0 was taken out of the graph when its own + // repair jobs completed, so it no longer points to 2 and no 0->2 job is created. Also, expect + // that swap job for 0 will be executed, so that 2 and 0 are swapped - the single pending job is + // then 1->0 (originally 1->2). EXPECT_EQ(tiered_index->deleteVector(2), 1); EXPECT_EQ(tiered_index->indexSize(), 2); EXPECT_EQ(tiered_index->getHNSWIndex()->getNumMarkedDeleted(), 1); @@ -1909,7 +1909,7 @@ TYPED_TEST(HNSWTieredIndexTest, swapJobBasic2) { mock_thread_pool.thread_iteration(); EXPECT_EQ(tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load(), 0); // Delete 1. The only other element left (the "new" 0, which is the old 2) is deleted and was - // already taken out of the graph, so nothing points to 1 and no repair job is created for it. + // already taken out of the graph, so nothing points to 1 and no u->1 job is created. // Its swap job is therefore ready right away, and the swap and removal of the previous 0 is // triggered - so that 1 gets id 0. EXPECT_EQ(tiered_index->deleteVector(1), 1); @@ -1930,11 +1930,13 @@ TYPED_TEST(HNSWTieredIndexTest, swapJobBasic2) { EXPECT_EQ(tiered_index->statisticInfo().numberOfMarkedDeleted, 0); } -// Covers the invalidation of a pending repair job whose node is disposed of by a swap job. -// A deleted element is taken out of the graph as soon as *its own* repair jobs are done, so to have -// a pending repair job at the time it is disposed of, the job has to belong to *another* element's -// deletion: element 0 is deleted first, then element 1 is deleted while 0 still points to it (which -// registers a repair job for node 0), and only then 0 completes its own repairs and is swapped out. +// Covers the invalidation of a pending repair job whose node is disposed of by a swap job. A repair +// job is denoted below as the edge it removes: u->v is the job that repairs u's connections after +// its neighbour v was deleted. +// A deleted element is taken out of the graph as soon as the jobs of *its own* deletion are done, +// so for a job on it to still be pending when it is disposed of, that job has to belong to +// *another* element's deletion: 0 is deleted first, then 1 is deleted while 0 still points to it +// (registering a 0->1 job), and only then the 1->0 and 2->0 jobs complete and 0 is swapped out. TYPED_TEST(HNSWTieredIndexTest, invalidRepairJobOnSwap) { size_t dim = 4; HNSWParams params = {.type = TypeParam::get_index_type(), @@ -1951,20 +1953,21 @@ TYPED_TEST(HNSWTieredIndexTest, invalidRepairJobOnSwap) { GenerateAndAddVector(tiered_index->backendIndex, dim, i, i); } - // Delete 0 - a repair job is created for every (node, level) pair that points to it. Note that - // the number of jobs depends on the levels the elements got, so the counters are compared to - // each other rather than to fixed values below. + // Delete 0 - a u->0 job is created for every (u, level) pair that points to it, that is 1->0 + // and 2->0. Note that the number of jobs depends on the levels the elements got (and that jobs + // of the same (u, level) are merged), so the counters are compared to each other rather than to + // fixed values below. EXPECT_EQ(tiered_index->deleteVector(0), 1); ASSERT_GT(mock_thread_pool.jobQ.size(), 0); ASSERT_GT(tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load(), 0); // Delete 1 before those jobs run. 0 is deleted but still connected (its own repairs are - // pending), so a repair job for node 0 is created here and stays pending. + // pending), so a 0->1 job is created here and stays pending. EXPECT_EQ(tiered_index->deleteVector(1), 1); ASSERT_TRUE(tiered_index->idToRepairJobs.contains(0)); - // Execute 0's repair jobs, so that it has no pending repair job left and is taken out of the - // graph, making its swap job ready. Its own pending repair job (for deleting 1) is still + // Execute the 1->0 and 2->0 jobs, so that 0 has no pending repair job left and is taken out of + // the graph, making its swap job ready. The 0->1 job, which belongs to 1's swap job, is still // queued. while (tiered_index->idToSwapJob.at(0)->pending_repair_jobs_counter.load() > 0) { ASSERT_GT(mock_thread_pool.jobQ.size(), 0); @@ -1975,16 +1978,17 @@ TYPED_TEST(HNSWTieredIndexTest, invalidRepairJobOnSwap) { int pending_for_1 = tiered_index->idToSwapJob.at(1)->pending_repair_jobs_counter.load(); ASSERT_GT(pending_for_1, 0); - // Dispose of 0. The repair job that is still pending for it has to be invalidated, and 1's swap - // job should stop waiting for that job. + // Dispose of 0. The pending 0->1 job has to be invalidated, and 1's swap job should stop + // waiting for it. tiered_index->runGC(); EXPECT_EQ(tiered_index->indexSize(), 2); EXPECT_EQ(tiered_index->invalidJobs.size(), 1); EXPECT_EQ(tiered_index->idToSwapJob.at(1)->pending_repair_jobs_counter.load(), pending_for_1 - 1); - // Drain the remaining jobs: the invalid one is disposed of without being executed, and the - // valid ones complete 1's repairs - so 1 is taken out of the graph as well. + // Drain the remaining jobs: the invalidated 0->1 job is disposed of without being executed, and + // the 2->1 job (whose node id was renamed by the swap above) completes 1's repairs - so 1 is + // taken out of the graph as well. while (!mock_thread_pool.jobQ.empty()) { mock_thread_pool.thread_iteration(); }