POST /cluster/recover can return 200 while raft silently drops the
snapshot request when no leader is known yet, leaving the test stuck
on a missing collection until timeout.
Co-authored-by: Cursor <cursoragent@cursor.com>
Wait for green on write (and read after recover_read) so collection and
partial snapshots are not taken mid-indexing. Otherwise a leftover
appendable segment survives partial merge and breaks manifest equality.
Co-authored-by: Cursor <cursoragent@cursor.com>
#9013 skipped the manual replicate_shard when a transfer was already
visible, but the recovery loop can still start one between that check
and the POST. Accept 400 "already involved in transfer" as success so
the remaining wait assertions still cover recovery.
Co-authored-by: Cursor <cursoragent@cursor.com>
Right after the no_sync snapshot recovery, the recovered replica serves local
reads immediately, but a remote read to it can transiently fail for a short
window. The read path then falls back to the other replica in hash order on
just the requesting peer, so a single routing token momentarily resolves to
different replicas across peers (observed as {A, B, B}), failing the
determinism assertion.
Wait until token-routed reads are stable across all peers for every token the
test asserts on before measuring, so the transient post-recovery fallback
window is passed. Pure test-side change; routing behaviour is unchanged.
Co-authored-by: Cursor <cursoragent@cursor.com>
* test: make test_upload_snapshot robust to shard placement balance
The final assertion in recover_from_uploaded_snapshot assumed a perfectly
balanced shard placement (peer 0 having exactly 2*n_replicas remote shards).
Shard placement across peers is not guaranteed to be balanced, so this made
the test flaky (e.g. peer 0 ended up hosting all shards locally, leaving only
3 remote replicas instead of 4).
Instead, verify the full replica layout is healthy: peer 0 observes every
replica through its local + remote shards, so assert that all replicas are
Active and every shard has exactly n_replicas copies across the cluster.
Co-authored-by: Cursor <cursoragent@cursor.com>
* test: fetch cluster info once for shard validation
Read local and remote shards from a single /cluster response so both lists
come from the same cluster revision, per review feedback.
Co-authored-by: Cursor <cursoragent@cursor.com>
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
A transfer source that misses the transfer abort (e.g. while partitioned
or paused) keeps its local shard wrapped in a proxy. When such a peer can
only catch up via consensus snapshot, snapshot application re-creates
payload indexes with an update operation that the stale forward proxy
forwards to a transfer target which may no longer have the shard. The
resulting precondition error fails snapshot application and stops the
consensus thread ("No target shard N found for update"), leaving the
peer unable to ever catch up.
Snapshot application now explicitly cleans up transfers that are no
longer registered in consensus: the transfer task is stopped and the
proxy is reverted via the new `ShardReplicaSet::discard_proxy_local`,
which is infallible, never contacts the remote, and forgets queued
updates (replica states in the same snapshot already reflect the
transfer outcome).
The consensus test reproduces the incident: pause the transfer source
mid-transfer, restart the other peers so the aborted transfer can only
be learned via snapshot, and verify the source recovers.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* Fix resharding, on queries filter shards on all shard selectors
* Add failing consensus test: search during resharding with shard keys (#9880)
Reproduces a known bug: after resharding is initialized on a custom
sharded collection with a shard key, searches (with and without the
shard key selector) fail with "does not have enough active replicas",
because the new resharding shard is included in reads before it has
an active replica.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* Exempt explicit shard id selection from resharding read filter
Explicit shard id selection is only used by internal per-shard
operations (local shard API, internal gRPC reads), including the
resharding driver reading back migrated points from the new shard.
These must reach the resharding shard before it becomes visible to
user-facing selectors, and filtering them also made per-shard reads
return silently empty results on peers lagging on hashring commits.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Explicitly set resharding filtering per match branch
---------
Co-authored-by: Andrey Vasnetsov <andrey@vasnetsov.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* Reset first_voter and prune address book on first-peer --reinit
A peer removed from consensus and killed after `RemoveNode(self)` was
committed but before it was applied keeps the old cluster's
`first_voter` and peer addresses in `raft_state.json`. First-peer
`--reinit` reset `conf_state` to a single voter (itself) but left both
untouched (`first_voter` is in fact never reset, even when the removal
is applied).
Both values are served to peers bootstrapping onto the reinitialized
cluster. A joining peer seeds its initial `conf_state` with the
advertised `first_voter`, and Raft conf-changes are deltas on top of
that base - so a stale `first_voter` permanently corrupts the joining
peer's voter set: it ends up with {old first peer, itself}, missing the
actual leader. Its `/readyz` then treats the still-alive old peer as a
cluster member and waits for the old cluster's commit index, which its
own consensus never reaches.
This only manifests when the reinitialized leader replicates its log as
plain entries (nothing applied before the kill, so the log anchor is
index 0). If the leader sends a snapshot instead, the snapshot's full
`conf_state` heals the corrupted seed - which is why
test_reinit_removed_peer only failed sporadically on CI.
Fix first-peer `--reinit` to behave like founding a fresh cluster:
reset `first_voter` to this peer (not `None`, or `recover_first_voter`
would re-derive the old first voter from the retained Raft log) and
prune `peer_address_by_id` to this peer only.
The kill-before-apply state is now injected deterministically into
test_reinit_removed_peer, reproducing the exact CI failure against the
unfixed binary. Since `--reinit` now prunes the address book, the
stale-address injection in
test_reinit_removed_peer_readyz_ignores_old_cluster moved to a restart
without `--reinit`, so it keeps exercising the /readyz `conf_state`
membership filter from #9688.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Fix test_reinit_consensus expecting stale address book after --reinit
The test waited for cluster size 2 right after starting the reinitialized
first peer, before the second peer was even started. That only passed
because first-peer --reinit used to keep the old cluster's addresses in
the address book - the stale state the previous commit removes. A
reinitialized first peer is a fresh single-member cluster; the other
peers re-join and re-register their new addresses right after.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The end-of-test teardown killed the uploader processes and slept for one
second before scrolling all peers for the consistency check. Killing the
client does not cancel an already-sent upsert server-side: on slow CI the
last PUT can take longer than the sleep, so its replication is still
propagating while the peers are scrolled at slightly different times,
making the scrolls diverge by the last batch of points.
Observed in test_shard_wal_delta_transfer_abort_and_retry: peer 0 was
scrolled at 19.089s, received the forwarded batch at 19.179s, while
peer 1 (which had already applied it locally) was scrolled at 19.267s.
Use stop_update_process() (introduced in #8713 for the pre-peer-kill
case) so the uploader exits between requests. Since uploads use
wait=true, once the last PUT returns all active replicas have applied
it, and the scrolls can no longer race.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Wait for the collection metadata to propagate to the newly added extra
peer before querying its collection cluster info. Being online and
present in consensus does not guarantee the peer has already applied the
collection-creation Raft entry locally, so get_collection_cluster_info
could race and return 404.
Co-authored-by: Cursor <cursoragent@cursor.com>
* test(consensus): de-flake replicate_points_stream_transfer_updates override case
With override_points=True the background writer re-upserts points
9990-9999, re-rolling their city payload. Points flipping away from
"London" legitimately drop out of the filtered count on both shards, so
asserting dest_filtered_count >= original snapshot count is not a valid
invariant. On a slow CI runner the sleep(1)+kill() stopped the writer
right after the overrides, before new inserts could compensate, making
a net-negative flip likely (observed: 4954 >= 4959 failure).
Replace the blind kill with a bounded workload (60 points) joined
cleanly before the ~10s transfer of 10k points can finish, assert the
writer exit code, and allow the filtered count to drop by up to the
number of overridden points.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(consensus): guard that writer finishes while transfer is running
The exact count consistency check requires every concurrent write to go
through the transfer proxy. Make that precondition explicit: if the
transfer ever finishes before the writer, fail with a clear message
instead of a confusing count mismatch.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(consensus): make replicate_points update consistency checks exact
Assign the city payload deterministically by point ID parity so filter
membership can never change under concurrent overwrites. All assertions
become exact ID-set comparisons with no slack: random city re-rolls made
count-based checks unsound, since the forward proxy filters forwarded
updates by post-update state and a point flipping out of the filter
legitimately goes stale or missing on the destination.
Replace the background writer process (sleep/kill/join choreography)
with synchronous wait=true upserts issued while the transfer streams the
initial points. Leave low point IDs unoccupied and insert into them
during the transfer: the stream cursor passes them immediately, so these
points can only reach the destination through live update forwarding,
which the previous layout (writes at the stream tail) never verified.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* Resolve filter-based update operations to point ids before WAL write
Filter/condition-resolving operations (delete-by-filter, conditional
upsert, the *-by-filter payload/vector operations) stored their filter
in the WAL and re-resolved it against live segment state on every
apply. Replay-time state can differ from the original apply-time state
(the optimizer drops deleted points and their version records during
compaction), so WAL replay was not a deterministic function of the log
and could resurrect filter-deleted points.
Resolve such operations into concrete point ids at submit time, under a
fence that guarantees the resolution sees exactly the operations that
precede it in WAL order. The WAL now only ever contains id-based
operations (pre-existing variants only — no format change), so replay
applies the exact same point set as the original run.
Fixes#9575
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DfVDMFQcy9Ww791x8sHobW
* Fix rustfmt
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DfVDMFQcy9Ww791x8sHobW
* Drop coordinator-side resolution: every replica resolves locally
Replicas holding the same data resolve the same filter to the same point
set, and replicas that already diverged would not become consistent by
agreeing on a filter's resolution. Forward the original filter operation
as usual and let each replica's submit fallback resolve it under its own
fence — one uniform path regardless of where the update lands.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DfVDMFQcy9Ww791x8sHobW
* Guard against is_filter_resolving / resolve_operation drift
A resolved operation must never still classify as filter-resolving,
otherwise a filter-carrying record could reach the WAL again (#9575).
Catch one direction of drift between the gate and the rewriter with a
debug assertion right after resolution.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Dedup points-vs-filter precedence into resolve_points_or_filter
The "explicit id list wins over the filter" rule was written twice on
the resolver side (DeletePayload arm and resolve_set_payload); a future
tweak landing in one copy only would make SetPayload and DeletePayload
silently diverge in what gets persisted to the WAL.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Assert rewritten WAL record reuses the incoming clock tag
The single-record-reuses-the-tag property is what WAL-delta recovery
and replica dedup rely on, but no test asserted it: submit the
delete-by-filter with a real clock tag and check the resolved
DeletePoints record carries it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Test replay of old-style filter records left in the WAL
Upgraded nodes can still hold WALs with unresolved filter operations;
the by-filter apply paths are kept so they replay one final time with
the old semantics. No test covered that path (the new submit flow can
no longer produce such WALs), so append a raw DeletePointsByFilter
record at the WAL layer, reload, and assert the matched points are
gone.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Add consensus test for per-replica filter-op resolution
Exercises the replicated path for filter/condition-resolving updates:
the coordinator forwards the original filter op and each replica
resolves it locally (delete-by-filter, insert-only and update-filter
conditional upserts, set-payload-by-filter, including per-shard empty
resolutions on a 2-shard collection). Asserts both replicas hold
identical state (reads prefer the local replica), then restarts the
whole cluster and asserts each replica replays its id-based WAL to the
same state.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com>
Applying `RemoveNode(self)` prunes all other peers from the removed
peer's persisted address book, but the process may be stopped after the
removal is committed and before the entry is applied. The old first
peer's address then survives `--reinit`, and the readiness checker -
which treated every `peer_address_by_id` entry as a cluster member -
would wait for the reinitialized peer to reach the *old* cluster's
commit index: a foreign consensus it can never catch up with, so
`/readyz` never passed.
Filter the address book by current `conf_state` membership instead,
falling back to all known addresses while `conf_state` is still empty
(a bootstrapping node that has not applied any configuration change
yet). After `--reinit` the `conf_state` is reset to a single voter, so
the readiness check correctly ignores peers of the old cluster.
Fixes flaky `test_reinit_removed_peer`, which hit this race when the
removed peer was killed before applying its own `RemoveNode`. The new
regression test simulates that state and fails with the exact CI error
without the fix.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* fix: unblock optimizer after deleting a named vector
Deleting a named vector could permanently block the config-mismatch
optimizer. The source-superset check in SegmentBuilder::update cancelled
every rebuild that found the deleted vector still in old segment files,
and each retry cancelled again, so optimizations got stuck forever.
Removing the check (as in #9609) would fix delete but reintroduce data
loss for the CreateVectorName race. Instead, tell the two cases apart
with the live collection schema: prune a source vector that is gone from
the schema (a real deletion), but cancel when it is still present (a
freshly created vector this optimizer has not yet seen). This is safe
because the schema is persisted before the op reaches segments, and the
live schema is read after the source segments are frozen.
The live set covers dense and sparse vectors, since a segment stores both
together. When no live source is wired in, the conservative always-cancel
behavior is kept.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: wire live vector names into edge optimizers
Deleting a named vector left the edge path with the pre-fix behavior:
segment_optimizer_config hardcoded live_vector_names to None, so a merge
touching a segment that still carried the deleted vector cancelled, and
EdgeShard::optimize() propagated the cancellation as a hard error forever.
Share the shard config behind an Arc and hand the blocking optimizers a
provider that reads the current vector names on every call. Same safety
argument as the server wiring: update() holds the segments read guard
across both the segment application and the config update, so any name a
frozen source segment carries is visible to the live read.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* refactor: share vector-name enumeration via CollectionParams::vector_names
The optimizer's live-schema set and the WAL-recovery valid-name set are
the same dense+sparse enumeration and must stay in lockstep; a drift
between them would reintroduce a wrong prune/cancel decision. Replace
the private helper in optimizers_builder and the inline block in WAL
recovery with a single CollectionParams method.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* refactor: drop SegmentOptimizer::live_vector_names forwarding hop
The default trait method only forwarded to the config getter and had a
single caller; ShardOptimizationStrategy now reads the config directly,
removing one layer of indirection.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Killing the background load processes does not cancel requests already
executing server-side: a wait=true upsert accepted just before the kill
can still be propagating to the second replica while the test counts
points, so peers transiently observe different totals (e.g. [20120,
20118, 20118] on CI). Poll the exact counts until they converge instead
of asserting on the first sample.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* Fix reinit failing with "removed all voters" on a removed peer
When `--reinit` is run on the first peer, `conf_state` is reset to a
single voter (this peer), but the Raft log is left untouched. If this
peer had been removed from consensus before reinit, its log still holds
a committed-but-unapplied `RemoveNode(self)` conf-change. On startup that
entry is replayed on top of the freshly reset single-voter config, and
Raft aborts with "removed all voters", so the node can never start.
Resetting only the apply-progress queue is not enough: a fresh
single-node leader re-commits and re-applies any log entries still
physically present beyond `commit`, re-triggering the failure. The stale
tail has to be physically dropped from the WAL.
On the first-peer reinit path, discard committed-but-unapplied entries
inherited from the previous cluster: truncate the WAL to the last applied
index, pin `commit` to it and clear the apply-progress queue. The entry
at `commit` is retained as the snapshot anchor, so the first peer can
still serve snapshots to bootstrapping peers.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Add regression test for reinit of a removed peer
Starts a 2-node cluster, gracefully removes the second peer from consensus
(so it commits `RemoveNode(self)` into its own WAL), then reinitializes it
as a fresh first peer. Before the fix this panicked on startup with
"removed all voters"; the test asserts the peer comes back online, elects
itself leader, and can still seed a fresh bootstrapping peer.
Verified the test fails against the pre-fix binary with exactly:
Failed to apply configuration change entry
Caused by: Error in Raft consensus: removed all voters
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Restart consensus thread on failure instead of stopping permanently
If the consensus loop failed (e.g. due to a transient I/O error such as
running out of disk space), the consensus thread stopped permanently and
the only way to recover was a full service restart.
Now the consensus thread rebuilds the Raft node from persisted state and
retries with exponential backoff (1s initial, doubled up to 5 min cap,
retrying indefinitely). Rebuilding from persisted state is equivalent to
a process restart, so this introduces no new recovery semantics. The
`reinit` logic is never repeated on restart, and initial startup remains
fail-fast.
The consensus message channel is created outside of `Consensus` so that
internal gRPC handlers and the forward-proposals thread keep their
senders across restarts, and messages buffered during the outage are
drained after recovery.
While in restart backoff, the node reports the existing
`StoppedWithErr` cluster status with restart attempt info appended, and
flips back to `Working` after a successful restart.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Add staging TestTransientError op and consensus restart integration test
New staging-only cluster operation `test_transient_error` (mirroring
`test_slow_down`): applying it fails with the given probability on the
targeted peer, stopping its consensus thread with a service error. The
peer re-applies the entry on every consensus thread restart, rolling the
probability again, so it exercises the consensus restart loop end to
end. Probability 1.0 simulates a permanently failing operation.
The integration test runs a 3-peer cluster, poisons a follower through
its own API (so the simulated failure is reported deterministically in
the response), and asserts that the restart loop engages, the remaining
peers keep serving consensus operations with a quorum, and the failed
peer eventually recovers and catches up.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Fix import order to satisfy rustfmt
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* remove unused iter_offsets
* Replace MultivectorOffsetsStorage::iter_offsets with callback-based for_each_offset
First step of removing the iterator-returning read API (whose deep,
composable generic types blow up mangled symbol size). Convert the
offsets read from an iterator to a callback the caller pushes into:
- trait method iter_offsets -> for_each_offset(ids, FnMut(usize, MultivectorOffset))
returning common::universal_io::Result<()>
- Mmap impl now uses the callback read_batch (drops one read_iter use)
- Ram / Chunked impls push into the callback; Chunked still goes through
iter_vectors for now (converted in a later step)
- the single caller (for_each_in_multi_batch) passes a closure
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Implement read_batch directly on ReadPipeline, not via read_iter
read_batch now drives the pipeline itself (refill-then-wait loop, like
read_multi_iter) and invokes the callback per result, instead of
consuming the iterator returned by read_iter. A step toward removing the
iterator-returning read API.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Remove the ReadMulti RPC from the StorageRead gRPC service
ReadMulti was the only real consumer of UniversalRead::read_multi (which
itself relies on read_multi_iter). Removing the RPC end-to-end clears the
path to dropping that read API. StorageReadService keeps all its other
RPCs (ListFiles, FileExists, FileLength, ReadBytes, ReadBytesStream,
ReadWhole, ReadBatch).
- proto: drop `rpc ReadMulti` + ReadMulti{Entry,Request,Response}
- regenerated lib/api + uio-client generated code; drop ReadMulti
validation rules in lib/api/build.rs
- tonic: delete the read_multi handler + its 2 tests
- uio-client: delete Client::read_multi, the mock-server impl, and 2 tests
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Remove UniversalRead::read_multi
Its only real consumer was the StorageRead ReadMulti gRPC handler (removed
in the previous commit); the two wrapper forwarders had no callers. Drop
the trait method and both forwarders (typed/read_only), and remove the
io_uring test that only existed to compare read_multi vs read_multi_iter
(read_multi_iter stays covered by the other tests). Another step toward
removing the iterator-returning read API.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Drive ReadPipeline directly in gridstore read_from_pages
Replace the read_multi_iter call in Pages::read_from_pages with a direct
pipeline loop (refill-then-drain), scheduling each multi-page read on its
own page file. Behavior unchanged; another step toward removing the
iterator-returning read_multi_iter.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Drive ReadPipeline directly in gridstore read_batch_from_pages
Replace the second read_multi_iter call (in Pages::read_batch_from_pages)
with a direct pipeline loop, scheduling each (ReadMeta, page, range) on its
own page file and propagating errors via GridstoreError. Single/multi-page
buffering and out-of-order reassembly are unchanged. No more read_multi_iter
in gridstore.
Measured overhead on warm mmap (both paths are zero-copy borrows): ~0.3 ns
per read of fixed control cost, flat across read sizes — well under 0.1% of
a real payload read.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Drive ReadPipeline directly in on-disk postings with_posting_views
Replace read_iter in OnDiskPostings::with_posting_views with a direct
pipeline loop. wait_bytemuck yields a file-borrowed Cow, so postings are
still stored zero-copy in raw_postings (a read_batch swap would have forced
an owned copy of every posting list per query on the mmap backend).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Read on-disk posting headers via read_batch, drop the HeadersBatch iterator
headers_iter now reads headers with the callback read_batch API: each header
is parsed (copied) out of the read bytes, so nothing borrows the file past the
read — no pipeline needed. Since the read is now eager, HeadersBatch holds the
collected Vec<HeaderResult> directly instead of a Box<dyn Iterator>, dropping
the boxing, the dynamic dispatch, and the struct's lifetime parameter.
with_posting_views takes the Vec and still pipelines the posting reads.
Removes the last read_iter use in this file.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Use read_batch in simple_disk_cache populate_from
populate_from reads one byte per block only to fault blocks into the local
cache, discarding the bytes — a no-op-callback read_batch fits exactly. Drives
the same DiskCachePipeline as before; one less read_iter caller.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Implement read_iter directly on ReadPipeline, not via read_multi_iter
read_iter now drives the pipeline itself (refill-then-wait loop, mirroring
read_bytes_iter) instead of mapping its ranges onto self and calling
read_multi_iter. Same signature and iterator contract, so all callers are
unchanged. Leaves iter_vectors as read_multi_iter's only remaining caller.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Revert "Drive ReadPipeline directly in on-disk postings with_posting_views"
This reverts commit c02c41f17b.
* Add callback-based for_each_vector next to iter_vectors
for_each_vector drives the ReadPipeline directly across chunk files and
invokes a fallible callback per flattened multi-vector, returning
OperationResult, instead of returning an iterator built on read_multi_iter.
Callers will migrate onto it so iter_vectors (read_multi_iter's last caller)
can be removed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Make dense for_each_in_batch / for_each_in_dense_batch fallible
Thread OperationResult up the dense batch-read path so io_uring read
errors propagate instead of being .expect()ed deep inside the storage.
The for_each_in_dense_batch scorer path (custom/metric query scorers)
now carries the Result to the infallible score() boundary where it is
.expect()ed; read_vectors keeps its () signature and .expect()s locally.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Convert dense read_vectors to for_each_vector
The read-only and appendable dense storage read_vectors impls drove
ChunkedVectors::iter_vectors directly; switch them to the callback-based
for_each_vector and .expect() the result at the (infallible) read_vectors
boundary. Removes the last dense-path iter_vectors callers.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Convert quantized for_each_offset to for_each_vector + OperationResult
The two chunked MultivectorOffsetsStorage impls drove iter_vectors to read
the offset table; switch them to the callback-based for_each_vector.
for_each_vector returns OperationResult, so upgrade the for_each_offset
trait (and all four impls) from universal_io::Result to OperationResult
(the universal_io -> Operation direction, via ?). No error downgrade.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Convert EncodedStorage/EncodedVectors iter_batch to callback for_each_batch
The two chunked-mmap EncodedStorage impls drove ChunkedVectors::iter_vectors
to back iter_batch. Replace the iterator-returning iter_batch on both the
EncodedStorage and EncodedVectors traits (quantization crate) with a callback
for_each_batch(FnMut(usize, &[u8])), and switch the chunked impls to
for_each_vector. The callback is infallible: the chunked impls .expect() the
read internally, matching iter_vectors' prior panic-on-read-error behavior,
so no OperationError is downgraded. Scorers and the multivector readers adopt
the callback; the accumulating multivector path owns (to_vec) only when it
must buffer across components.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Convert multivector read paths to for_each_vector
The read_only multivector free fn chained two iter_vectors (offsets feeding
vectors) - the recursive iterator nesting behind the worst symbol bloat.
Replace it with a callback for_each_vector that resolves the per-point
offsets into a Vec first, then drives ChunkedVectors::for_each_vector over
the flattened vectors. Migrate both multivector storages' read_vectors and
for_each_in_batch_multi accordingly, .expect()ing at their infallible
boundaries.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Remove read_multi_iter and ChunkedVectors::iter_vectors
With every caller migrated to callback-based for_each_vector/for_each_batch,
delete the last iterator-returning multi-read APIs: ChunkedVectors::iter_vectors
(segment) and the read_multi_iter trait method plus its mmap override, the
TypedStorage/ReadOnly wrapper forwarders, and the two io_uring unit tests.
These deeply-nested monomorphized iterator types (read_multi_iter feeding
read_multi_iter) produced >1 MiB mangled drop_in_place symbols that overflowed
the macOS ld symbol-name limit; the callback rewrite eliminates them.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Pass owned Cow through for_each_vector/for_each_batch to avoid a copy
The callback-based readers handed the callback a borrowed `&[u8]`/`&[T]`,
forcing the quantized multivector batch read to `to_vec()` each sub-vector
into its per-point buffer. But io_uring-like backends already return a freshly
owned buffer per read (`ACow::Owned`), so that was a redundant second copy.
Change `ChunkedVectorsRead::for_each_vector` and the `EncodedStorage` /
`EncodedVectors` `for_each_batch` callbacks to receive `Cow<[..]>` by value.
The buffering path now `into_owned()`s it — a move when the backend returned
owned (the case this path targets), a copy only for a borrowed Cow (mmap),
which never reaches this path. Immediate-use callers (scorers,
score_point_max_similarity, the dense/multivector readers) just deref the Cow;
the dense readers drop their now-redundant `Cow::Borrowed` wraps.
Also clarifies the multivector reader: `SubVectorOwner`/`owners`/
`sub_vector_offsets` naming, docs, and a corrected comment noting the per-point
buffer is what makes regrouping order-independent under out-of-order completion.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Drop the removed ReadMulti JWT access test; fix clippy unwrap_or_default
The ReadMulti StorageRead RPC was removed earlier in this branch, so the
consensus JWT-access test (and its registry entry) for it must go too. Also
switch the multivector buffer's `or_insert_with(SmallVec::new)` to
`or_default()` per clippy::unwrap_or_default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Add MmapFile::read_batch
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: xzfc <xzfcpw@gmail.com>
* Add routing token structure
* Implement routing token in read operation executor as per design doc
* Add TODO to glue routing token to user requests
* Implement routing header for REST API
* Source routing token from request, not from JWT token
* Implement routing token in gRPC API
* Add test
* Review remarks
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* Use lower case header name to prevent panic
* Rename header to X-Qdrant-Route-Affinity
* Assert routing consistency in test on all peers
---------
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
The test asserts that creating a partial snapshot between two in-sync peers
returns 304 (empty diff). It only waited for the write peer to become green,
but read the read peer's manifest for the comparison. An async optimization
reshaping the read peer's segments after collection-snapshot recovery makes
its manifest diverge from the write peer's files, producing 200 instead of
304 (assert 200 == 304).
Wait for the read peer to become green as well before comparing manifests,
mirroring the earlier flaky-test fixes (#7358, #7360).
Co-authored-by: Cursor <cursoragent@cursor.com>
The test relied on concurrent background upserts adding *new* matching
points to the destination shard during the streamed transfer, asserting
the destination count was strictly greater than the original snapshot.
Whether new matching points land in the destination before the transfer
completes is timing-dependent (especially under pytest-xdist load), so
the strict `>` assertion is racy and occasionally fails with equal
counts (e.g. `assert 5031 > 5031`).
Relax to `>=` and keep the strict `dest == src` consistency check, which
is the actual invariant being verified.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Add block level timeout to snapshot stream writer
* Add tooling to exercise streaming snapshot stalls
- tests/manual/slow_snapshot_download.py: slowly / partially download a
streaming shard snapshot from a URL to exercise sender-side backpressure.
Supports hold / rst / fin / blackhole termination to simulate a stalled,
killed, or offline (network-partitioned) consumer against a remote node.
Stdlib only; read-only against the target.
- tests/consensus_tests/test_streaming_snapshot_receiver_kill.py: throttled
receiver killed mid-flight + a second receiver, to observe whether the
sender releases the SegmentHolder lock and recovers.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: timvisee <tim@visee.me>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The initial 20k-point insert was sent as a single HTTP request (no
batch_size), saturating all cores long enough to starve the consensus
thread (cascading leader elections) and to exceed the 2000ms per-shard
update healthcheck deadline, returning a flaky 408 before the test's
actual snapshot-transfer logic even started.
Batch the insert into 1k-point requests, matching the convention used
by every other large initial upsert in the suite.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The test fired a single 20k-point `wait=true` upsert into a fresh
3-shard x 2-replica collection. Under CI load that one op keeps a replica
busy past the hardcoded 2s inter-node health-check (transport_channel_pool
HEALTH_CHECK_TIMEOUT), so the coordinator fails the forward with a transient
"Healthcheck timeout 2000ms exceeded" 408 before resharding even starts.
It's the only resharding test doing a 20k single-shot upsert (others do
~1k), which is why it flakes and they don't. Batch the load at 1000 points
so each op stays well under the health-check window, matching the
proven-stable pattern. Data and transfer behavior are unchanged.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test_resharding_down_abort_converges_when_killed_mid_abort fired the
fire-and-forget abort_transfer request at alive_uri after waiting only for
the *victim* to apply the transfer-start. When alive_uri lagged in replicating
that consensus entry, the abort handler's local check_transfer_exists returned
404 (swallowed by the fire-and-forget thread); the transfer then completed
naturally, resharding was never aborted, and _victim_in_window never opened,
timing out after 30s.
Wait until both alive_uri (the abort target that gates the 404) and the victim
have applied the transfer-start before firing the abort.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Abort resharding before we abort transfer
* Test resharding-down abort converges when a peer is killed mid-abort
---------
Co-authored-by: tellet-q <elena.dubrovina@qdrant.com>
* test: wait for WAL flock release on peer restart in dirty-shard test
The flake addressed by #9124 (bumping `wait_for_peer_online` to 60s) was
misdiagnosed as CPU contention. Logs show the restarted peer panics within
~1s of startup at `consensus_wal.rs:36` with:
Wal error: Can't init WAL: Kind(WouldBlock)
Panic: Can't open consensus WAL: Kind(WouldBlock)
`wal::Wal::open` calls `fs4::FileExt::try_lock` (non-blocking `flock`) on
the WAL directory fd. After `p.kill()` (SIGKILL + waitpid) the kernel
normally releases the killed peer's flock immediately, but under
pytest-xdist load there is a small window where it lags. The fresh peer's
startup then races and panics. After the panic `/readyz` never returns
200, so neither 30s nor 60s rescues the test.
Fix: add a `wait_for_wal_unlocked` helper that polls both the consensus
WAL directory and the local-shard WAL directory with the same exclusive
non-blocking flock that qdrant uses, and call it after every `p.kill()`
that is followed by a `start_peer` on the same `peer_dir`. The two
60s timeouts are restored to the default 30s now that the underlying
race is gone.
Co-authored-by: Cursor <cursoragent@cursor.com>
* test: fail fast if sync restart crashes in dirty-shard test
The sync restart in `test_dirty_shard_survives_update_collection` used
plain `wait_for_peer_online(sync_uri)`, which only polls `/readyz`. If
the freshly started peer panics on startup (e.g. WAL `WouldBlock`), the
test waits the full 30s timeout and then reports a `/readyz` timeout
instead of the actual panic message and exit code.
Switch the sync restart to `wait_for_peer_online_or_crash(...)` (same
helper already used for the dirty restart). On crash it dumps the peer
log tail so the next CI failure shows the real reason instead of a
generic timeout.
Co-authored-by: Cursor <cursoragent@cursor.com>
* test: drop speculative WAL flock wait, keep crash-detection switch
Reverts the `wait_for_wal_unlocked` helper that polled the WAL
directories with non-blocking flock. The kernel-side flock race it was
guarding against could not be reproduced in isolation (0/300 iters of
SIGKILL+wait+re-flock on bare Linux), so it was speculative.
Kept:
- Sync restart now uses `wait_for_peer_online_or_crash(...)` instead of
plain `wait_for_peer_online`, so any startup panic surfaces fast with
the actual log tail instead of hiding behind a 30s `/readyz` timeout.
- The two 60s timeouts bumped in #9124 are restored to the default 30s.
If the flake recurs in CI, the new failure output will tell us the
actual cause (panic message + exit code), which is more useful than
papering over it.
Co-authored-by: Cursor <cursoragent@cursor.com>
---------
Co-authored-by: Cursor Agent <agent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
* tests: use uniform `from .utils import *` in consensus tests
Eight consensus tests imported specific symbols from `utils` instead of
using `from .utils import *`. The most consequential side effect was
missing the `every_test` autouse fixture, which is responsible for
cleaning up leaked `processes` and resetting the port-slice allocator
between tests. Without it, leaks from one test can carry into the next
on the same xdist worker, causing `processes.pop(target_idx)` to return
the wrong peer in tests like `test_dirty_shard_crash_loop` and triggering
WAL-lock conflicts when the intended target is left running.
Make the imports uniform across the package so the autouse fixture is
always in scope.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* tests: keep `test_issues_api` on explicit imports
This module uses a `@pytest.fixture(scope="module")` cluster setup
shared across all tests in the file. With `from .utils import *` the
function-scoped `every_test` autouse fixture comes into scope and runs
after the module-scoped `setup`, so on the first test it sees the
already-populated `processes` and calls `kill_all_processes()` — killing
the shared cluster before the test body runs. The test then fails with
connection refused on the cluster's port.
Keep explicit imports here so `every_test` is not autoloaded; the
module teardown already cleans up via `kill_all_processes()`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`commit_read_hashring` returns once the leader has applied the entry,
but followers may not have applied it yet. The CommitRead handler calls
`invalidate_clean_local_shards` for `old.nodes()`, which cancels any
ongoing shard clean task. If the test's subsequent `cleanup?wait=true`
request to a follower arrives just before that follower applies
CommitRead, the cleanup task is started and then cancelled mid-flight,
and the endpoint returns HTTP 500 "Failed to clean shard points due to
cancellation, please try again".
Add `wait_for_same_commit` after `commit_read_hashring` so every peer
has applied the entry (and thus already invalidated any clean tasks
that don't exist yet) before we issue cleanup.
Observed flake:
https://github.com/qdrant/qdrant/actions/runs/26259341982/job/77289143792
Co-authored-by: Cursor Agent <agent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
* test(consensus): snapshot transfer with set-payload for missing points
Add a consensus test reproducing a shard transfer abort caused by
set-payload (and other partial-update) operations targeting non-existing
points.
Such operations are written to the WAL before the point-existence check
rejects them, so the queue proxy replays them to the receiver during a
snapshot transfer. The receiver applies them with force=true, bypassing
the missing-point tolerance in handle_failed_replicas, and the operation
hard-fails with `NotFound: No point with id ... found`. Under sustained
load the bounded queue/driver retries are exhausted, the receiver replica
is marked Dead and the transfer is aborted.
The test keeps the missing-point load running while checking the result,
because consensus auto-recovers Dead replicas: stopping the load first
would let the next recovery transfer succeed and mask the bug. It must
FAIL on current code and PASS once the receiver tolerates missing-point
operations during recovery.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(transfer): skip non-transient errors during queue proxy WAL replay (#9126)
During a shard transfer the queue proxy replays operations from the
sender's WAL to the receiver. Some of these are partial updates
(set_payload, update_vectors, ...) that the receiver rejects with a
non-transient error - most commonly `NotFound: No point with id ...`
for a point that does not exist on the receiver, but also any other
client-caused bad request.
These operations were replayed from the WAL, meaning they were already
applied (and rejected the same way) on the sender, so the sender's state
reflects them as no-ops. Propagating the error aborted the whole transfer;
under sustained load the bounded queue/driver retries were exhausted and
the receiver replica was marked Dead.
Handle the error where the semantic context lives - the transmitter
(`transfer_operations_batch`): skip operations the remote rejects with a
non-transient error and keep going, while still propagating transient
errors so the caller retries delivery. Because the batch update API aborts
at the first failing operation, a non-transient batch error falls back to
one-by-one sending to isolate and skip the offending operation(s).
This complements PR #5991, which handles missing points on the live
forwarded-update path (handle_failed_replicas) but not the WAL replay path.
Fixes the abort reproduced by
test_shard_snapshot_transfer_with_missing_point_updates.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Under parallel pytest-xdist load the rejoining peer needs more than the
default 30s budget to replicate the UpdateCollection entry and report
ready, causing intermittent CI failures:
Exception: Timeout waiting for condition peer_is_online to be satisfied
in 30 seconds
Bump both peer-online waits in the dirty shard crash-loop test to 60s,
matching the precedent set by #8963 for similar CPU-contention flakes.
Co-authored-by: Cursor Agent <agent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
* Recreate optimizers in non-blocking fashion from consensus calls
* Update comments
* On optimizer config update failure, report error status to local shard
* Add a test to confirm we don't block consensus
* Rerun recreation if called multiple times
* Use atomics instead
* Move to the bottom
* Reformat
* Use cluster default shard transfer method for fallback
When a WAL delta automatic transfer fails, the driver falls back to the
method passed via `fallback_method`. This was hard-coded to
`StreamRecords` (unless `prevent_unoptimized` was enabled), which is
inconsistent with the 1.18.0+ default of `Snapshot` and ignores any
configured `default_shard_transfer_method`.
Use `Collection::default_shard_transfer_method()` instead, so the
fallback matches the cluster default. With `prevent_unoptimized` we
still pin to `Snapshot` to preserve deferred point state exactly (raw
segment copy); stream_records would send deferred points but they
would not be deferred on the target.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Avoid wal_delta fallback, update fallback test
If the cluster default transfer method is wal_delta, the same-method
fallback would be refused by the driver. Use snapshot as a safe fallback
in that case; snapshot is also the 1.18.0+ default.
Update test_shard_wal_delta_transfer_fallback to assert the new
snapshot fallback (was stream_records).
Co-authored-by: Cursor <cursoragent@cursor.com>
* Address clippy wildcard_enum_match_arm
Co-authored-by: Cursor <cursoragent@cursor.com>
---------
Co-authored-by: Cursor Agent <agent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
* Add test to show cleanup may conflict with update queue
* When invoking clean task, first wait for current update queue
* Don't hold shard holder lock for a long time
* Also assert the clean task finished completely
The test_shard_snapshot_transfer_throttled_updates test was flaky because
it checked data consistency immediately after killing background upload
processes, without waiting for in-flight writes to propagate across peers.
All sibling tests (test_shard_snapshot_transfer_fast_burst,
test_shard_stream_transfer_throttled_updates, etc.) already include a
sleep(1) after killing uploaders. This was the only variant missing it.
Co-authored-by: Cursor Agent <agent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix: validate vector dimensions before WAL write for async upserts
When upserting points with wait=false (the default), dimension
mismatches were silently discarded during background processing.
The API returned 200 "acknowledged" but the points were never stored,
causing silent data loss with no error feedback to the user.
This adds an early dimension validation check in do_upsert_points()
that runs before the operation is written to WAL. This ensures that
dimension errors are returned to the client regardless of the wait
parameter, matching the behavior of wait=true.
The validation handles all vector types:
- Dense single vectors
- Multi-dense vectors
- Named vectors (dense, multi-dense, sparse)
- Sparse vectors are skipped (no fixed dimension)
Closes#9039
Co-authored-by: Cursor <cursoragent@cursor.com>
* refactor: move vector dimension validation into dedicated module
Extract validate_vector_dimensions and helper functions from update.rs
into src/common/validate_vectors.rs for better code organization.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix: update shard update test for early dimension validation
The test expected a shard-level error message, but now dimension
mismatches are caught before reaching the shards. Update the assertion
to accept either the early validation error or the shard-level error.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix: assert actual dimension error message in shard update test
Check for the descriptive error ("Vector dimension error: expected dim: 4, got 3")
rather than the generic shard failure wrapper.
Co-authored-by: Cursor <cursoragent@cursor.com>
---------
Co-authored-by: Cursor Agent <agent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
When the restarted peer's dummy shard is auto-recovered by the
cluster's recovery loop before the test issues its manual
`replicate_shard` call, the manual call returns 400 "already involved
in transfer". Skip the manual call when a transfer is already in
flight — the existing wait_for / transfer-count / replica assertions
still verify the shard recovers.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Use unique point IDs across all phases instead of overwriting the same
id repeatedly. The gridstore payload storage size estimate is
bitmask-based: overwrites keep old blocks allocated until a periodic
flush reclaims them, so the test was racing the 5s flush worker. With
unique ids every block stays live and the post-flush size still
reflects all inserted points.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The test waited a fixed 0.5s after each PUT/DELETE before reading from
every peer, which raced with raft apply on followers under CI load.
Replace the fixed sleeps with wait_for-based polling so each per-peer
read retries until the expected value is observed.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Under stress, three concurrent user-requested snapshot transfers (10 000
points each) running while the killed peer recovers can starve the
leader's heartbeats long enough to trigger a raft election. If the
recovery transfer's `RecoveryToPartial` proposal is submitted while no
leader exists, raft drops it silently — `recovered_switch_to_partial`
returns Ok regardless because it only sends to a channel — and the
retry path then waits a full CONSENSUS_CONFIRM_TIMEOUT (10s) before
trying again. Combined with sequential per-shard recovery (auto
transfer limit = 1 × 3 shards), the 30s `/readyz` budget runs out.
Add an optional `wait_for_timeout` to `wait_for_peer_online` and bump
this test's wait to 60s. Default behaviour for other callers is
unchanged.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* [ai] Add integration test for triggering inconsistent resharding state
* [ai] Also add test for resharding down
* Update test
* Resolve resharding idempotency through setting replica states
* Remove resharding down test
* Reformat
* [ai] Remove resharding abort order, abort before setting replica state
* [ai] Resolve test flakiness
* Collapse matches into helper function
* Check preconditions before aborting resharding
* Fix test flakiness by not waiting for a dead node
* Abort resharding before aborting transfer for idempotency
* Update comment
* Release shard holder lock on transfer/reshard abort to prevent deadlock
* Remove now unused shard holder parameter
* Split handle_replica_changes to eliminate need for juggling locking
* In resharding tests, import all utils to enable every_test cleanup
Resolves flakiness I've been seeing in
test_set_replica_dead_clears_resharding_state test
The test killed the last peer immediately after start_cluster, but
start_cluster only waits for cluster size and a known leader — not for
all peers to be promoted from learner to voter. If the last peer caught
up first, it became the only other voter alongside the leader; killing
it left a 2-of-2 quorum with one voter dead, and the subsequent
CreateCollection commit timed out after 10s.
Wait for all peers to be voters before killing one, so the survivors
form a 2-of-3 voter quorum.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The rapid drop/create loops in test_rejoin_cluster intentionally use
short 3s timeouts to accumulate Raft log entries quickly. Under CI
load the consensus apply for CreateCollection can exceed 3s (segment
setup competes with background flushes/optimizations), and the API
returns 500 even though the operation reaches consensus right after.
The matching upserts already pass `fail_on_error=False`; do the same
for `create_collection` to make the test resilient to that race.
The deadline loop only waited for the download phase to become
"streaming", but the assertion also required bytes > 0. On CI the
assertion could fire before iter_content yielded the first chunk,
causing a flaky failure. Wait for bytes > 0 in the deadline loop
and improve the error message to show bytes/error state.
Co-authored-by: Cursor Agent <agent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
* test: keep optimizers disabled during snapshot transfer in deferred test
The snapshot variant of test_shard_transfer_includes_deferred_points was
flaky because optimizers were enabled before the transfer, letting the
optimizer race ahead and fully index the segment before the snapshot was
captured (~1s of HNSW build for 500 small vectors fits comfortably before
the snapshot is taken). The deferred-state assertion then fails since all
points are already visible.
Only enable optimizers before the transfer for stream_records (which needs
them for its internal wait=true). For snapshot, leave optimizers disabled
through the transfer so deferred state is preserved on the wire, then
enable them afterwards for trigger_upsert_wait_true. The hung server-side
wait=true from the timeout-and-retry block does not block the snapshot —
wait_for_deferred_points_ready runs in a detached tokio::spawn.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test: skip wait=true probe for snapshot variant
CI showed that with optimizers kept disabled through the snapshot transfer
(needed to preserve deferred state on the wire), the wait=true probe at
the start of the test leaves a hung server-side request: update_local
holds local.read() until the deferred wait resolves, and there is no
optimizer to resolve it. The subsequent shard transfer's apply path
deadlocks against that held read lock when queue_proxify_local tries to
take local.write().
For stream_records the config update later cancels the hung worker, so
the probe is fine there. Move the probe (and config update) under the
stream_records branch so the snapshot variant doesn't leave a hung
update around. The probe was auxiliary behaviour verification, not
central to the snapshot-of-deferred-points assertion.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Revert "test: skip wait=true probe for snapshot variant"
This reverts commit d07aac78a263d7a91691e43444b9dae44e3d179f.
* test: add reproducer for deferred-wait shard-transfer deadlock
Adds test_shard_transfer_with_hung_deferred_wait_does_not_deadlock as a
focused reproducer for the engine bug surfaced by the snapshot variant
of test_shard_transfer_includes_deferred_points.
Lock-ordering chain:
1. With prevent_unoptimized=true and max_optimization_threads=0, a
wait=true upsert on deferred points enters
wait_for_deferred_points_ready (update_worker.rs:241), which loops
on tokio::select over cancel and optimization_finished. The
optimization_worker hits limit==0 and `continue`s without firing
optimization_finished_sender (optimization_worker.rs:172-174), so
neither branch of the select ever fires.
2. update_local (replica_set/update.rs:49) holds self.local.read()
across the entire update await. actix-web does not cancel the
response future on client disconnect, so the read guard stays alive
even after the client's 5s timeout.
3. A subsequent snapshot transfer eventually calls queue_proxify_local
(replica_set/shard_transfer.rs:122), which needs self.local.write().
tokio::sync::RwLock is write-preferring: the queued writer blocks
new readers, including is_local() calls on the consensus apply
path itself (shard_transfer.rs:129-130). The apply never returns,
the consensus broadcast never fires, POST /cluster times out with
"Waiting for consensus operation commit failed".
The new test asserts the symptom (POST /cluster must return promptly)
without papering over the bug, so it stays red until the engine is
fixed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(replica_set): release local read guard around deferred-points wait (#8862)
* fix(replica_set): drop remotes read guard early in update_impl
`update_impl` was holding `self.remotes.read()` and `self.local.read()`
across the entire update await, including the deferred-points wait that
can park indefinitely under prevent_unoptimized + max_optimization_threads=0.
When a shard transfer is started concurrently with a parked wait=true
update, the consensus apply runs `add_remote`, which calls
`self.remotes.write().await`. tokio::sync::RwLock is write-preferring:
the queued writer is blocked behind the held read, the apply never
returns, and `POST /cluster` times out with "Waiting for consensus
operation commit failed".
Fix: snapshot updatable remote shards into owned `Vec<RemoteShard>` and
drop the read guard before the await. The remote_update futures now own
the cloned RemoteShards, so they no longer borrow from the guard.
The `local` guard is still held across the await (futures borrow
`&Shard` from it). Releasing it would unblock `queue_proxify_local`'s
`local.write()` too, but that requires wrapping `Shard` in `Arc` —
deferred to a follow-up. For the consensus-commit-timeout deadlock
exposed by `test_shard_transfer_with_hung_deferred_wait_does_not_deadlock`,
dropping `remotes` is sufficient.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(updater): wake deferred wait on caller-receiver drop
`wait_for_deferred_points_ready` parked on a `tokio::select` over
`cancel.cancelled()` and `optimization_finished_receiver.changed()`.
Under prevent_unoptimized + max_optimization_threads=0, neither fires:
optimization_worker.rs:171-174 hits `limit == 0` and `continue`s
without notifying, and the cancel token is the worker's lifecycle
token (only fired by stop_update_worker on config update / shutdown).
The top-of-loop `is_closed()` poll didn't help — the loop never
re-runs once the select parks.
Take `feedback_sender` by `&mut` and add `feedback_sender.closed()`
as a third select branch. When the matching `Receiver` is dropped
(by upstream cancellation, client-supplied timeout, or any future
cancellation), the detached task wakes immediately and exits with
WaitTimeout instead of staying parked until the next worker restart.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* [AI] split update operarion into submit and independent wait function
* [AI] refactor `update_local` to drop local shard lock after submitting update operation
* [AI] refactor `update_impl` for early release of the lock in case of local shard update
* fmt
* Apply suggestion from @generall
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
