Fix concurrent VADD deadlock on disk-tiered vector sets - #2064
Merged
Conversation
Concurrent VADD to a vector set whose DiskANN records spill to the object log could deadlock the server. A network VADD that recreates a disk-tiered index blocks on a pending object-log disk read (CompletePending) while holding the per-set ReadOptimizedLock exclusively. The quantization workers run on the .NET thread pool and spin-waited (Thread.Yield) on that same lock, consuming every pool thread so the disk-read IO completion - itself a pool work item - could never be scheduled. The lock was never released and all workers stalled permanently. The workers now acquire the set lock non-blockingly and yield their pool thread on contention instead of spin-waiting, so a disk-IO completion can always be scheduled to release the lock: - ReadVectorIndex splits into a thin wrapper over ReadVectorIndexCore(nonBlocking, out wouldBlock). In non-blocking mode it uses TryAcquireExclusiveLock/TryAcquireSharedLock and returns a default (empty) lock with wouldBlock=true on contention, and likewise reports contention instead of spin-waiting for a pending index drop. The blocking path used by every network caller (e.g. VSIM) is unchanged. - The quantization worker loop processes each request through a synchronous TryProcessQuantizationRequest helper (all ref struct / Span / native interop stays off the await path) and, on contention, awaits (Task.Yield, then Task.Delay(1)) and retries. Adds ConcurrentVaddToSpilledSetMakesProgress, a liveness regression test that fails if concurrent VADD to a spilled set stops making forward progress. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 00a8e6b4-cc07-41c5-bcf9-4b262ad55451
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes thread-pool starvation during concurrent disk-tiered VADD operations.
Changes:
- Adds cooperative lock retries for quantization workers.
- Safely initializes and drains quantization tasks.
- Adds a disk-spill concurrency regression test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
VectorManager.Quantization.cs |
Implements asynchronous contention retries. |
VectorManager.Locking.cs |
Adds non-blocking vector-index locking. |
VectorManager.cs |
Initializes and drains worker tasks safely. |
ConcurrentVaddDiskSpillTests.cs |
Adds the liveness regression test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- ConcurrentVaddDiskSpillTests: on the deadlock-detection branch, dispose the server on a background task with a bounded wait and abandon it if that does not complete, instead of disposing inline via `using`. Under a regression the quantization workers spin-wait permanently and VectorManager.Dispose() blocks on Task.WhenAll of them, so inline disposal would hang teardown before NUnit could report the failure. This keeps the failure deterministic in-process, without a separate killable child process. OnTearDown resets the leaked epoch instances and only fails on leaks when the test passed, so the abandoned server does not mask the reported failure. - VectorManager ctor: assign quantizationTaskCount directly and drop the redundant local vectorSetQuantizationTaskCount. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 00a8e6b4-cc07-41c5-bcf9-4b262ad55451
The liveness smoke test asserted more than 1000 concurrent VADD inserts in its 60s window to confirm the object-log spill path was exercised. That floor was calibrated for a fast dev disk; the shared, disk-bound windows-latest CI runner only completes ~800 inserts in the same window, so the test failed on the final assertion (observed 787/834) even though it never deadlocked and the no-progress stall monitor never fired. Records spill to the object log within the first few inserts (4 KB pages vs ~8 KB records), so a much smaller floor already proves the spill/flush path ran; the floor only guards against the workers never executing at all. Lower it to 100 (well below the observed slow-CI throughput) so slow hardware cannot make it flake. The deadlock itself is still detected by the stall monitor, not this count. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 00a8e6b4-cc07-41c5-bcf9-4b262ad55451
kevin-montrose
approved these changes
Aug 13, 2026
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
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.
Summary
Fixes the deadlock defect reported in gist
badrishc/1da9f5175490b3cbd74b93c89f03cb6e: concurrentVADDto a single vector set whose ~8 KB DiskANN records spill to the object log (small--memory/--page+--storage-tier, or thelowMemorytest helper) can hang the server through .NET thread-pool starvation.This PR supersedes #2017, which bundled this fix with an unrelated object-log flush/overflow change. That flush work is being carried separately (to be reworked on top of the no-copy
tedhar/aof-chunkeffort), so only the deadlock fix and its regression test are included here.The defect
A full process dump of the reproduced hang shows the cause is thread-pool starvation, not a lock-correctness bug:
StartQuantizationTasks) run asTasks on thread-pool threads and spin-wait (Thread.Yield()) on the per-setReadOptimizedLockviaReadVectorIndex.VADDrecreating a disk-tiered index (RecreateIndex,indexPtr == 0) blocks inCompletePending(wait: true)on a pending object-log disk read while holding that same lock exclusively.VADDwith a quant type enqueues aBuildQuantizationTablerequest, so up toProcessorCountworkers pile onto the one hot set. With all pool threads spinning or blocked, the disk-read IO completion — itself a pool work item — can never be scheduled, the semaphore never signals, and the exclusive lock is never released. Permanent, self-reinforcing deadlock.Proof of mechanism: setting
ThreadPool.SetMinThreads(512, 512)in the repro clears the hang (CPU drops from ~37 busy cores to ~1.4 and inserts resume, disk-bound).Fix
Keep the quantization workers on the thread pool, but make them acquire the vector-set lock cooperatively so they can never starve the pool. Instead of spin-waiting, a worker tries the non-blocking
TryAcquire*path and, on contention, yields its pool thread (await) and retries. A blocked pool thread is then always freed for the disk-IO completion that releases the lock, so concurrentVADDmakes progress. Quantization parallelism and backfill sharding are preserved, and no non-pool background threads are introduced.VectorManager.Locking.cs— splitReadVectorIndexinto a thin wrapper plusReadVectorIndexCore(nonBlocking, out wouldBlock). In non-blocking mode it usesTryAcquireSharedLock/TryAcquireExclusiveLockand reports contention (wouldBlock) instead of spin-waiting, and likewise reports contention rather than spinning on a pending index drop. The blocking path (all existing network callers such asVSIM) is byte-for-byte unchanged.VectorManager.Quantization.cs— the worker loop drains the channel and processes each request through a synchronousTryProcessQuantizationRequesthelper (all ref-struct /Span/ native interop stays off theawaitpath). On contention the helper returnsfalseand the async loopawaitsTask.Yield()(thenTask.Delay(1)after a few spins) before retrying.VectorManager.cs—quantizationTasksseeded withTask.CompletedTaskin the constructor soDispose'sTask.WhenAlldrain is always safe.Is the exclusive lock the cause? It is an amplifier/coupler, not the root. The root anti-pattern is waiting by spinning on a pool thread: the
ProcessorCount-scale quantization workers were the dominant amplifier (proven in the dump), converting one blocked disk-read-under-lock intoProcessorCountbusy-spinners that each pin a pool thread. The exclusive lock is necessary but not sufficient — fixing the waiters (this change) breaks the cycle. A deeper alternative would fix the holder (not hold the exclusive lock across the blocking disk read during recreate); that is larger and left as a follow-up.Validation
Garnet.serverand the vectorset test project build clean on net8.0 and net10.0, 0 warnings (TreatWarningsAsErrorson).ConcurrentVaddToSpilledSetMakesProgress: 8 concurrentVADDworkers hammer one set whose records spill to the object log and must keep making forward progress; the historical deadlock stalls all workers permanently and fails the test. Passes on net10.0 Debug (~62 s).dotnet format --verify-no-changesclean on all changed files.Notes
Scope is intentionally the deadlock only; the object-log flush overflow
NullReferenceExceptionfrom the same gist is handled separately.