[MOD-18494] Stop a relabel mid-query from reporting one vector twice - #1047
Draft
nonirosenfeldredis wants to merge 3 commits into
Draft
[MOD-18494] Stop a relabel mid-query from reporting one vector twice#1047nonirosenfeldredis wants to merge 3 commits into
nonirosenfeldredis wants to merge 3 commits into
Conversation
A tiered query reads the flat buffer, releases its guard, then reads the main index, and `merge_result_lists` collapses a vector that both tiers report by matching labels. A relabel landing in that window defeats the match: the flat half of the pair still says `old_label` while the main half now says `new_label`, so the merge keeps both and the caller gets one vector twice, under a pair of labels the index never held at the same time. The label-keyed merge is load-bearing for the migration race it was written for -- `TopKQuery_SameVectorDifferentScores_DueToQuantization` pins that -- and it holds there only because migration preserves the label. Relabel moves the very key the merge rests on. Detection rather than exclusion. `relabelEpoch` is bumped by every relabel that moves a label, while it still holds both guards; a query samples it around its pair of reads and redoes the read if it changed. The redo passes `pin_flat`, holding the flat guard across both reads, which is what makes the window absent the second time: relabel needs that guard exclusively. So the retry is bounded at one -- no loop, no livelock. The fast path costs two atomic loads. Pinning is only paid when a relabel actually interleaved, and it cannot deadlock: no path takes the flat guard while holding the main guard, so a query holding flat-then-main cannot cycle against relabel's flat-then-main. Rejected: validating flat-sourced results against current label state after the main read. It fixes this, but it also drops stale results for concurrently *deleted* labels -- a behaviour change well outside relabel's scope. Not covered: `TieredHNSW_BatchIterator` merges the tiers incrementally across many calls, so redo-on-detect does not map onto it. A relabel concurrent with a live batch iterator can still duplicate. Tests drive a relabel into the window through a new BUILD_TESTS hook, for topK and range. Both assert the vector is reported once and that the retry actually fired, so they fail loudly rather than passing on the fast path if the window moves. Verified by mutation: with the epoch bump removed both report `Which is: 2`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nonirosenfeldredis
changed the title
Detection alone forces a re-read, which only works for a reader that can start
over. Record what each relabel did and the merge can repair the pair it was
handed: a flat result under a label that has since moved is rewritten to the
label it moved to, so the label-keyed merge collapses it against the main
result as it always did.
`recordRelabel` appends {old, new, epoch} under the guards that made the move.
A query samples the epoch while the flat guard still covers its snapshot, so
the sample names exactly the state its results came from, and after the main
read it replays every recorded move newer than that sample onto both lists.
Chains are followed: a label that moved twice inside one window (A->B->C) must
land on C, since stopping at B matches nothing in the main results and
duplicates just the same. The walk is bounded by the number of moves, which
also stops a cycle (A->B then B->A) from spinning. Rewriting labels can break
the by-id tie-break the merge assumes, so both lists are re-sorted first.
The log is a fixed ring, so it never allocates and never grows. Past its
capacity the oldest moves are dropped, and a query whose snapshot is no longer
covered falls back to the pinned re-read from the previous commit, which is
always correct. That keeps the previous mechanism as the fallback rather than
replacing it: the epoch is the version stamp, the pin is the backstop.
Why record rather than only detect: a reader that cannot restart can still
consult the log. That is the route to covering `TieredHNSW_BatchIterator`,
which merges the tiers incrementally across many calls and which redo-on-detect
cannot help. Not wired up here.
Rejected: keying the log off the flat buffer and reading it with the flat
snapshot. Both fail. At flat-read time the relabel has not happened yet, so the
list is empty; and a relabel that touches only the main index -- reachable when
the snapshot predates migration -- would never be recorded at all.
Tests add the chain case and the overflow fallback, and the two duplicate tests
now assert the log served them without a re-read. Verified by mutation: with
the replay disabled all three report `Which is: 2`; with the chain walk cut to
one step the chain test alone fails, so each test discriminates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The batch iterator holds `mainIndexGuard` shared from its first call until the HNSW iterator depletes, and a relabel needs that guard exclusively. So for all but the first call a relabel simply cannot run, and the iterator has exactly one window: between releasing the flat guard and taking the main one on that first call. A relabel there moves a label out from under the flat snapshot already taken, and the iterator then reports the same vector once per label across its batches. Take the main guard before releasing the flat one and the window is gone: a relabel needs both and can hold neither. The flat->main order is the one every other acquisition already uses, so overlapping them cannot deadlock. This is why the iterator needs no part of the relabel log. It cannot start over, which is what the log was meant to buy it -- but it does not have to, because it already excludes relabel for its whole life and only the seam between its two acquisitions was open. The log stays for the queries, which genuinely release both guards mid-read. The test drives the relabel from another thread, since the hook now runs while the flat guard is held and relabeling inline would deadlock against it. It blocks on the flat guard, then on the main guard for the rest of the iteration, and completes only once the iterator is freed -- so the iteration stays consistent with the snapshot it took. Verified by mutation: restoring the original release order makes it report `Which is: 2`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nonirosenfeldredis
added a commit
that referenced
this pull request
Sep 11, 2026
`SVSIndex::relabelVector` moved a label, but wrapping that index in a tier did not inherit the ability: neither `TieredSVSIndex` nor `VecSimTieredIndex` overrode `relabelVector`, so a tiered SVS index fell through to the interface default and answered `Unsupported`. That is correct for a caller which honours the code, but it meant a tiered SVS index never took the relabel path however capable its backend was -- and tiered is how it is used. Both tiers are asked, because a multi-value label routinely has copies in each and a target taken in either would collide once the buffer drains. The backend moves first, since it is the tier that can refuse -- `Unsupported` when built against an SVS without `replace_external_id` -- and refusing after the buffer had moved would leave the label half applied. `updateJobMutex` is taken first, in the order `updateSVSIndex` takes its own locks. That is not defensive: an update job snapshots the buffer's labels *by value* and afterwards reconciles only id swaps and deletions, so a rename landing between the snapshot and the backend insert is invisible to it and the vector reaches the backend under the old label, leaving it under both. All three guards are held across the checks and the mutations. Checking under shared locks and reacquiring exclusively would be cheaper for rejections, but `std::shared_mutex` cannot upgrade, and async `addVector` and `deleteVector` need neither `updateJobMutex` nor a held guard -- so either could land in the gap and leave the move applied to one tier only. Tests mirror what insertion has for this mode. `MovesTheLabelInBothWriteStates` covers buffered-with-a-pending-job and moved-to-the-backend, the states `addVector` and `insertJob` cover; `RejectsOnATier` covers each code, with the taken target tried in both tiers; `DuringUpdateJob` is the `insertJobAsync` analogue, relabelling 200 labels against live workers. The fixture's type set spans single, multi and Quant_8, so this also answers whether a compressed backend can move a label: it can, since `replace_external_id` renames an id and never touches the stored vector. `CannotLandInsideAnUpdateJobsWindow` is what pins the mutex, via the `UpdateJob::before_add_to_svs` tracing hook that sits exactly in the window. Worth stating why it exists: `DuringUpdateJob` still passes with the mutex removed, so concurrency alone does not demonstrate the need. The hook test fails, reporting a label count of 2 for one vector. It relabels from another thread, since the job holds the mutex throughout and relabelling inline would block on a lock the thread already holds. Not added: relabel concurrent with a query. The tiers share `VecSimTieredIndex::topKQueryImp`, so SVS inherits the cross-tier duplicate that #1047 fixes, and such a test belongs with that fix rather than failing here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nonirosenfeldredis
added a commit
that referenced
this pull request
Sep 11, 2026
The relabel counterpart to `test_parallel_insert_search`: the operation on one thread, queries on another, with the relabels aimed at labels still being ingested so a label is briefly held by both tiers. The two tests differ in what they can assert, and that is the point. Inserting concurrently makes a query legitimately miss vectors, so the insert test can only check that recall did not regress. A relabel adds and removes nothing, so the invariant is exact: a reply must never list one label twice. That is the defect -- `merge_result_lists` collapses a vector both tiers report by matching labels, and a relabel inside a query's window moves that key, so the two copies survive as one vector under two labels. Skipped on this branch. Not because the assertion is unsound -- a duplicate label in a reply is always wrong -- but because the fix is on another branch (#1047, MOD-18494) and the failure is intermittent, so leaving it live would redden this PR's CI for a defect it did not introduce. Remove the marker once that lands; nothing else about the test changes. Being a canary is the honest description: the window is a few instructions, so it can pass on a broken build. It cannot report a failure that is not real, which is the acceptable direction. The deterministic coverage is the hook-based C++ tests on #1047. `import pytest` is added because `common.py` does not import it and the skip marker needs it -- the same omission that bit `test_svs.py` earlier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 tasks
nonirosenfeldredis
added a commit
that referenced
this pull request
Sep 12, 2026
A test that never runs earns nothing. It was added skipped because the fix it depends on is on another branch, which makes it dead weight here: it cannot catch a regression, and the marker is one more thing to notice and remove later. The deterministic coverage for this is the hook-based C++ tests on #1047, which is also where a flow-level version belongs once that lands. `import pytest` goes with it; nothing else in the file used it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the changes in the pull request
A tiered query reads the flat buffer, releases
flatIndexGuard, then reads the main index, andmerge_result_listscollapses a vector that both tiers report by matching labels. A relabel landing in that window defeats the match: the flat half of the pair still saysold_labelwhile the main half now saysnew_label, so the merge keeps both and the caller gets one vector twice, under a pair of labels the index never held at the same time.The label-keyed merge is load-bearing for the migration race it was written for —
TopKQuery_SameVectorDifferentScores_DueToQuantizationpins that — and it works there only because migration preserves the label.relabelVector(#1017) moves the very key the merge rests on.Replay, not just detection.
recordRelabelappends{old, new, epoch}under the guards that made the move. A query samples the epoch while the flat guard still covers its snapshot — so the sample names exactly the state its results came from — and after the main read replays every recorded move newer than that sample onto both lists. A flat result under a label that has since moved is rewritten to the label it moved to, and the label-keyed merge collapses it as it always did.Details that matter:
A→B→C) must land onC; stopping atBmatches nothing in the main results and duplicates just the same. The walk is bounded by the number of moves, which also stops a cycle (A→BthenB→A) spinning.merge_result_listsassumes.Why record rather than only detect. A reader that cannot restart can still consult the log. That is the route to covering
TieredHNSW_BatchIterator, which merges the tiers incrementally across many calls and which redo-on-detect cannot help. Not wired up here — the mechanism now exists for it.Rejected: keying the log off the flat index and reading it with the flat snapshot. Both halves fail. At flat-read time the relabel has not happened yet, so the list comes back empty — the query needs to know about a move in its future. And a relabel touching only the main index (reachable when the snapshot predates migration) would never be recorded by a flat-scoped log at all.
Rejected alternative. Validating flat-sourced results against current label state after the main read also fixes this, but it additionally drops stale results for concurrently deleted labels — a behaviour change well outside relabel's scope.
Not covered.
TieredHNSW_BatchIteratormerges the tiers incrementally across many calls, so redo-on-detect does not map onto it; a relabel concurrent with a live batch iterator can still duplicate. Flagging rather than papering over it.Tests. Two tests drive a relabel into the window via a new
BUILD_TESTShook, one for topK and one for range. Each asserts the vector is reported once and that the retry actually fired, so they fail loudly rather than passing on the fast path if the window moves. The hook deliberately does not fire on the pinned retry — there is no window there, and injecting one would self-deadlock against the guard the querying thread holds.Verified by mutation, twice. With the replay disabled, all three duplicate tests report
Which is: 2— the bug. With the chain walk cut to a single step, the chain test alone fails while the other two pass, so each test discriminates rather than merely passing.Full suites green:
test_hnsw191/191,test_hnsw_parallel4/4,test_hnsw_sq855/55,test_svs376 passed / 101 skipped (pre-existing SVS gating) / 0 failed.make check-formatclean.Which issues this PR fixes
Main objects this PR modified
VecSimTieredIndex—relabelEpoch+bumpRelabelEpoch();topKQuery/rangeQuerysample the epoch and redo once;topKQueryImp/rangeQueryImptakepin_flat;FlatGuardReleasereleases the pinned guard on the early-return paths.TieredHNSWIndex::relabelVector— bumps the epoch under both guards.tests/unit/test_hnsw_tiered.cpp—relabelDuringQueryDoesNotDuplicate,relabelDuringRangeQueryDoesNotDuplicate.Mark if applicable
🤖 Generated with Claude Code