Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/VecSim/algorithms/brute_force/brute_force_multi.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class BruteForceIndex_Multi : public BruteForceIndex<DataType, DistType> {
int addVector(const void *vector_data, labelType label) override;
int deleteVector(labelType labelType) override;
int deleteVectorById(labelType label, idType id) override;
VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) override;
double getDistanceFrom_Unsafe(labelType label, const void *vector_data) const override;
inline size_t indexLabelCount() const override { return this->labelToIdsLookup.size(); }

Expand Down Expand Up @@ -221,6 +222,31 @@ int BruteForceIndex_Multi<DataType, DistType>::deleteVectorById(labelType label,
return 0;
}

template <typename DataType, typename DistType>
VecSimRelabelCode BruteForceIndex_Multi<DataType, DistType>::relabelVector(labelType old_label,
labelType new_label) {
if (old_label == new_label) {
return VecSimRelabel_SameLabel;
}
auto old_it = labelToIdsLookup.find(old_label);
if (old_it == labelToIdsLookup.end()) {
return VecSimRelabel_OldLabelMissing;
}
if (labelToIdsLookup.find(new_label) != labelToIdsLookup.end()) {
return VecSimRelabel_NewLabelTaken;
}

// Every id under the label moves, so the whole id vector is re-keyed as-is.
auto ids = std::move(old_it->second);
labelToIdsLookup.erase(old_it);
for (idType id : ids) {
// Keep the id->label direction in sync; `topKQuery` reports results through it.
this->setVectorLabel(id, new_label);
}
labelToIdsLookup.emplace(new_label, std::move(ids));
return VecSimRelabel_OK;
}

template <typename DataType, typename DistType>
double
BruteForceIndex_Multi<DataType, DistType>::getDistanceFrom_Unsafe(labelType label,
Expand Down
23 changes: 23 additions & 0 deletions src/VecSim/algorithms/brute_force/brute_force_single.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ class BruteForceIndex_Single : public BruteForceIndex<DataType, DistType> {
int addVector(const void *vector_data, labelType label) override;
int deleteVector(labelType label) override;
int deleteVectorById(labelType label, idType id) override;
VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) override;
double getDistanceFrom_Unsafe(labelType label, const void *vector_data) const override;

std::unique_ptr<vecsim_stl::abstract_results_container>
Expand Down Expand Up @@ -197,6 +198,28 @@ int BruteForceIndex_Single<DataType, DistType>::deleteVectorById(labelType label
return deleteVector(label);
}

template <typename DataType, typename DistType>
VecSimRelabelCode BruteForceIndex_Single<DataType, DistType>::relabelVector(labelType old_label,
labelType new_label) {
if (old_label == new_label) {
return VecSimRelabel_SameLabel;
}
auto old_it = labelToIdLookup.find(old_label);
if (old_it == labelToIdLookup.end()) {
return VecSimRelabel_OldLabelMissing;
}
if (labelToIdLookup.find(new_label) != labelToIdLookup.end()) {
return VecSimRelabel_NewLabelTaken;
}

const idType id = old_it->second;
labelToIdLookup.erase(old_it);
labelToIdLookup.emplace(new_label, id);
// Keep the id->label direction in sync; `topKQuery` reports results through it.
this->setVectorLabel(id, new_label);
return VecSimRelabel_OK;
}

template <typename DataType, typename DistType>
double
BruteForceIndex_Single<DataType, DistType>::getDistanceFrom_Unsafe(labelType label,
Expand Down
47 changes: 47 additions & 0 deletions src/VecSim/algorithms/hnsw/hnsw.h
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,12 @@ class HNSWIndex : public VecSimIndexAbstract<DataType, DistType>,
// Remove label from the index.
virtual int removeLabel(labelType label) = 0;

// Check whether a label currently maps to at least one element. Note that a label whose
// element was marked deleted is *not* considered to exist, matching `getElementIds`.
virtual bool isLabelExists(labelType label) = 0;

VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) override;

#ifdef BUILD_TESTS
void fitMemory() override {
if (maxElements > 0) {
Expand Down Expand Up @@ -478,6 +484,47 @@ void HNSWIndex<DataType, DistType>::unmarkInProcess(idType internalId) {
unmarkAs<IN_PROCESS>(internalId);
}

/**
* Move every element stored under `old_label` to `new_label`. The graph is keyed purely on internal
* ids - links, entry point and levels never mention a label - so this only has to fix the two
* places a label is kept: `idToMetaData[id].label` (id -> label, read by `getExternalLabel` on
* every query result) and the derived class's label -> id lookup.
*
* Takes `indexDataGuard` exclusively, like `markDelete`, since both containers it mutates are
* guarded by it. A tiered index calling this while holding its main index guard exclusively is
* consistent with the main-guard-then-data-guard order used by `insertVectorToHNSW`.
*/
template <typename DataType, typename DistType>
VecSimRelabelCode HNSWIndex<DataType, DistType>::relabelVector(labelType old_label,
labelType new_label) {
if (old_label == new_label) {
return VecSimRelabel_SameLabel;
}
std::unique_lock<std::shared_mutex> index_data_lock(indexDataGuard);

// An absent source is reported before an occupied target, so that a caller which resolves
// `NewLabelTaken` by freeing the target is never told to do so for a move that has nothing to
// move. Elements that were marked deleted are already out of the label lookup, so they are
// reported as absent here and are left alone - their `idToMetaData` label is still needed by
// the pending swap/repair jobs that reference their id.
auto ids = getElementIds(old_label);
if (ids.empty()) {
return VecSimRelabel_OldLabelMissing;
}
if (isLabelExists(new_label)) {
return VecSimRelabel_NewLabelTaken;
}

removeLabel(old_label);
for (idType id : ids) {
// Assign the label field rather than the whole struct: `ElementMetaData`'s constructor
// resets `flags` to IN_PROCESS, which would hide a live element from queries.
idToMetaData[id].label = new_label;
setVectorId(new_label, id);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HNSW relabel races with queries

Medium Severity

HNSWIndex::relabelVector writes idToMetaData[id].label under only indexDataGuard, but searches call getExternalLabel without that lock. ElementMetaData is #pragma pack(1), so the label store is not atomic. Concurrent search can observe a torn or stale label. Tiered relabel avoids this with an exclusive main guard; standalone HNSW does not.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1a23d0e. Configure here.

return VecSimRelabel_OK;
}

template <typename DataType, typename DistType>
void HNSWIndex<DataType, DistType>::lockIndexDataGuard() const {
indexDataGuard.lock();
Expand Down
9 changes: 9 additions & 0 deletions src/VecSim/algorithms/hnsw/hnsw_base_tests_friends.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,12 @@ INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_deleteFromHNSWMultiLevels_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_deleteFromHNSWWithRepairJobExec_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_swapJobBasic_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_deleteInplaceAvoidUpdatedMarkedDeleted_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTest_relabelVector_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTest_relabelVectorRejects_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTest_relabelVectorMarkedDeleted_Test)
INDEX_TEST_FRIEND_CLASS(HNSWMultiTest_relabelVectorMulti_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorFlatOnly_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorBothTiers_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorMulti_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorRejects_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorDuringIngestion_Test)
3 changes: 3 additions & 0 deletions src/VecSim/algorithms/hnsw/hnsw_multi.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ class HNSWIndex_Multi : public HNSWIndex<DataType, DistType> {
return getDistanceFromInternal(label, vector_data);
}
int removeLabel(labelType label) override { return labelLookup.erase(label); }
bool isLabelExists(labelType label) override {
return labelLookup.find(label) != labelLookup.end();
}
};

/**
Expand Down
3 changes: 3 additions & 0 deletions src/VecSim/algorithms/hnsw/hnsw_single.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ class HNSWIndex_Single : public HNSWIndex<DataType, DistType> {
return getDistanceFromInternal(label, vector_data);
}
int removeLabel(labelType label) override { return labelLookup.erase(label); }
bool isLabelExists(labelType label) override {
return labelLookup.find(label) != labelLookup.end();
}
};

/**
Expand Down
96 changes: 96 additions & 0 deletions src/VecSim/algorithms/hnsw/hnsw_tiered.h
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ class TieredHNSWIndex : public VecSimTieredIndex<DataType, DistType> {

int addVector(const void *blob, labelType label) override;
int deleteVector(labelType label) override;
VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) override;
size_t getNumMarkedDeleted() const override {
return this->getHNSWIndex()->getNumMarkedDeleted();
}
Expand Down Expand Up @@ -915,6 +916,101 @@ int TieredHNSWIndex<DataType, DistType>::deleteVector(labelType label) {
return num_deleted_vectors;
}

/**
* Move a label across every place the tiered index records one. Unlike add/delete this needs no
* jobs and no vector data movement - the swap/repair machinery is keyed purely on internal ids, so
* it is unaffected.
*
* A label can legitimately live in *both* tiers at once: `executeInsertJob` inserts into HNSW
* before removing the vector from the flat buffer, so all four updates below are independently
* guarded by their own existence check rather than treated as mutually exclusive.
*
* Locking: `flatIndexGuard` exclusively (it guards the flat buffer's lookups, `labelToInsertJobs`
* and - per the note in `executeInsertJob` - the job fields themselves), then the main index guard
* exclusively. That is the flat-then-main order used by `insertVectorToHNSW` and `topKQueryImp`,
* so it cannot deadlock against them; repair and swap jobs only ever take the main guard.
* The main guard must be *exclusive* rather than shared: query threads read
* `getExternalLabel` under a shared main guard, and `ElementMetaData` is byte-packed, so its
* `label` field can be unaligned and its store is not atomic.
*/
template <typename DataType, typename DistType>
VecSimRelabelCode TieredHNSWIndex<DataType, DistType>::relabelVector(labelType old_label,
labelType new_label) {
if (old_label == new_label) {
return VecSimRelabel_SameLabel;
}

this->flatIndexGuard.lock();
this->lockMainIndexGuard();

auto *hnsw_index = this->getHNSWIndex();
// A label can live in any subset of its three homes, so both questions are asked of all of
// them: it is present if any home holds it, and the target is free only if none does. Reject if
// the target is taken anywhere, so that a partially applied move is impossible - a free target
// in all three homes means none of the updates below can collide.
const bool source_exists =
this->frontendIndex->isLabelExists(old_label) ||
this->labelToInsertJobs.find(old_label) != this->labelToInsertJobs.end() ||
hnsw_index->isLabelExists(old_label);
const bool target_taken =
this->frontendIndex->isLabelExists(new_label) ||
this->labelToInsertJobs.find(new_label) != this->labelToInsertJobs.end() ||
hnsw_index->isLabelExists(new_label);

// Each home is asked to move the label only once it reported holding it, and the checks above
// ruled out every other rejection - the label is present, the target is free everywhere, and
// the labels differ - so while both guards are held a home that is asked can only answer OK.
// The asserts below state that; a refusal would mean the state changed underneath us.
if (source_exists && !target_taken) {
if (this->frontendIndex->isLabelExists(old_label)) {
const VecSimRelabelCode flat_ret =
this->frontendIndex->relabelVector(old_label, new_label);
#ifdef BUILD_TESTS
assert(flat_ret == VecSimRelabel_OK &&
"the flat buffer just reported holding this label");
#endif
UNUSED(flat_ret);
}

// Re-key the pending insert jobs *and* rewrite each job's own copy of the label. Both must
// move together: `executeInsertJob` indexes into HNSW under `job->label` and then looks the
// job up with `labelToInsertJobs.at(job->label)`, so a half-applied move either indexes the
// vector under the stale label or throws out of the worker thread.
auto jobs_it = this->labelToInsertJobs.find(old_label);
if (jobs_it != this->labelToInsertJobs.end()) {
auto jobs = std::move(jobs_it->second);
this->labelToInsertJobs.erase(jobs_it);
for (HNSWInsertJob *job : jobs) {
job->label = new_label;
}
this->labelToInsertJobs.emplace(new_label, std::move(jobs));
}

// `relabelVector` takes the HNSW index data guard internally, which is the same
// main-guard-then-data-guard order that `insertVectorToHNSW` uses.
if (hnsw_index->isLabelExists(old_label)) {
const VecSimRelabelCode hnsw_ret = hnsw_index->relabelVector(old_label, new_label);
#ifdef BUILD_TESTS
assert(hnsw_ret == VecSimRelabel_OK && "HNSW just reported holding this label");
#endif
UNUSED(hnsw_ret);
}
}

this->unlockMainIndexGuard();
this->flatIndexGuard.unlock();

// An absent source outranks an occupied target: a caller that resolves `NewLabelTaken` by
// freeing the target would otherwise drop an unrelated vector for a move with nothing to move.
if (!source_exists) {
return VecSimRelabel_OldLabelMissing;
}
if (target_taken) {
return VecSimRelabel_NewLabelTaken;
}
return VecSimRelabel_OK;
}

// `getDistanceFrom` returns the minimum distance between the given blob and the vector with the
// given label. If the label doesn't exist, the distance will be NaN.
// Therefore, it's better to just call `getDistanceFrom` on both indexes and return the minimum
Expand Down
5 changes: 5 additions & 0 deletions src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,8 @@ INDEX_TEST_FRIEND_CLASS(BM_VecSimBasics)
INDEX_TEST_FRIEND_CLASS(BM_VecSimCommon)

friend class BM_IncomingEdgesBase;
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorFlatOnly_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorBothTiers_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorMulti_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorRejects_Test)
INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorDuringIngestion_Test)
5 changes: 5 additions & 0 deletions src/VecSim/vec_sim.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,11 @@ extern "C" int VecSimIndex_DeleteVector(VecSimIndex *index, size_t label) {
return index->deleteVector(label);
}

extern "C" VecSimRelabelCode VecSimIndex_RelabelVector(VecSimIndex *index, size_t old_label,
size_t new_label) {
return index->relabelVector(old_label, new_label);
}

extern "C" double VecSimIndex_GetDistanceFrom_Unsafe(VecSimIndex *index, size_t label,
const void *blob) {
return index->getDistanceFrom_Unsafe(label, blob);
Expand Down
20 changes: 20 additions & 0 deletions src/VecSim/vec_sim.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,26 @@ int VecSimIndex_AddVector(VecSimIndex *index, const void *blob, size_t label);
*/
int VecSimIndex_DeleteVector(VecSimIndex *index, size_t label);

/**
* @brief Move the vector(s) stored under `old_label` to `new_label`, leaving the stored vector data
* and the index structure untouched.
*
* Only label bookkeeping is rewritten, so this costs O(1) per stored vector - no distance
* computations and, for HNSW, no graph mutation. Intended for callers whose external id changed
* while the vector did not, as a cheap replacement for delete-then-add.
*
* The move is rejected, leaving the index untouched, if `old_label` does not exist, if `new_label`
* is already in use, or if the labels are equal - each with its own code. Not all index types
* support this; those report `VecSimRelabel_Unsupported`, which a caller has to serve by delete +
* insert rather than treat as a no-op.
*
* @param index the index holding the vector(s).
* @param old_label the label currently holding the vector(s).
* @param new_label the label to move them to.
* @return `VecSimRelabel_OK` if the label was moved, otherwise the reason it was not.
*/
VecSimRelabelCode VecSimIndex_RelabelVector(VecSimIndex *index, size_t old_label, size_t new_label);

/**
* @brief Calculate the distance of a vector from an index to a vector. This function assumes that
* the vector fits the index - its type and dimension are the same as the index's, and if the
Expand Down
12 changes: 12 additions & 0 deletions src/VecSim/vec_sim_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,18 @@ typedef enum {
VecSimDebugCommandCode_MultiNotSupported
} VecSimDebugCommandCode;

// Outcome of `relabelVector`. The rejections are reported separately because a caller acts on them
// differently: `OldLabelMissing` and `SameLabel` mean there is nothing to do, `NewLabelTaken` is a
// caller-side conflict to resolve, and `Unsupported` says this index type never relabels - so the
// caller has to fall back to delete + insert rather than treat it as a no-op.
typedef enum {
VecSimRelabel_OK = VecSim_OK, // for returning VecSim_OK as an enum value
VecSimRelabel_OldLabelMissing, // `old_label` is not in the index
VecSimRelabel_NewLabelTaken, // `new_label` is already in the index
VecSimRelabel_SameLabel, // `old_label` and `new_label` are equal
VecSimRelabel_Unsupported // this index type does not implement relabeling
} VecSimRelabelCode;

typedef struct AsyncJob AsyncJob; // forward declaration.

// Write async/sync mode
Expand Down
27 changes: 27 additions & 0 deletions src/VecSim/vec_sim_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,33 @@ struct VecSimIndexInterface : public VecsimBaseObject {
*/
virtual int deleteVector(labelType label) = 0;

/**
* @brief Move the vector(s) stored under `old_label` to `new_label`, without touching the
* stored vector data or the index structure.
*
* Only the label bookkeeping changes, so this is O(1) per stored vector: no distance
* computations, no graph mutation, and no invalidation of internal ids. It is the cheap
* alternative to delete-then-add for callers whose external id changed while the vector
* itself did not.
*
* The operation is rejected - index left untouched - if `old_label` is not present, if
* `new_label` is already taken, or if the two labels are equal. Rejecting rather than
* overwriting keeps the caller in control: overwriting `new_label` would silently drop a
* vector, and unlike `addVector` there is no replacement data to justify it. Each rejection has
* its own code, so a caller can tell a conflict it may resolve from an index type that will
* never relabel and has to be served by delete + insert instead.
*
* The default implementation reports `VecSimRelabel_Unsupported` so that index types which
* delegate label management to an external library are not forced to implement it.
*
* @param old_label the label currently holding the vector(s).
* @param new_label the label to move them to.
* @return `VecSimRelabel_OK` if the label was moved, otherwise the reason it was not.
*/
virtual VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) {
return VecSimRelabel_Unsupported;
}

/**
* @brief Calculate the distance of a vector from an index to a vector.
* @param index the index from which the first vector is located, and that defines the distance
Expand Down
7 changes: 7 additions & 0 deletions tests/benchmark/bm_initialization/bm_basics_initialize_fp32.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,10 @@ BENCHMARK_REGISTER_F(BM_VecSimBasics, BM_DELETE_LABEL_ASYNC)
->Arg(100)
->Arg(BM_VecSimGeneral::block_size)
->ArgName("SwapJobsThreshold");

// RelabelLabel
BENCHMARK_TEMPLATE_DEFINE_F(BM_VecSimBasics, BM_RELABEL_LABEL, fp32_index_t)
(benchmark::State &st) { RelabelLabel(st); }
REGISTER_RelabelLabel(BM_RELABEL_LABEL, INDEX_BF);
REGISTER_RelabelLabel(BM_RELABEL_LABEL, INDEX_HNSW);
REGISTER_RelabelLabel(BM_RELABEL_LABEL, INDEX_TIERED_HNSW);
Loading