Fix concurrent VADD deadlock and object-log flush crash on disk-tiered vector sets

[Ted Hart (TedHartMS)]

Summary

Fixes two distinct concurrency defects reported in gist badrishc/1da9f5175490b3cbd74b93c89f03cb6e: concurrent VADD to a single vector set whose ~8 KB DiskANN records spill to the object log (small --memory/--page + --storage-tier, or the lowMemory test helper) can either deadlock (hang the server) or crash (server-down NullReferenceException on the flush thread). Both are addressed here.

---

Defect 1 — Deadlock (thread-pool starvation)

A full process dump of the reproduced hang proves the cause is .NET thread-pool starvation, not a lock-correctness bug:

• The quantization workers (StartQuantizationTasks) ran as Tasks on thread-pool threads and spin-wait (Thread.Yield()) on the per-set ReadOptimizedLock via ReadVectorIndex.
• Meanwhile a network VADD recreating a disk-tiered index (RecreateIndex, indexPtr == 0) blocks in CompletePending(wait: true) on a pending object-log disk read while holding that same lock exclusively.
• Every VADD with a quant type enqueues a BuildQuantizationTable request, so up to ProcessorCount workers 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 on the lock, 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 concurrent VADD makes progress. Full quantization parallelism and backfill sharding are preserved, and no non-pool background threads are introduced.

• VectorManager.Locking.cs — split ReadVectorIndex into a thin wrapper plus ReadVectorIndexCore(nonBlocking, out wouldBlock). In non-blocking mode it uses TryAcquireSharedLock/TryAcquireExclusiveLock and reports contention (wouldBlock) instead of spin-waiting, and returns wouldBlock rather than spinning on a pending index drop. The blocking path (all existing network callers such as VSIM) is byte-for-byte unchanged.
• VectorManager.Quantization.cs — the dedicated Thread[] workers become an async QuantizationTaskAsync that drains the channel and calls a synchronous TryProcessQuantizationRequest helper (all ref-struct / Span / native interop stays off the await path). On contention the helper returns false and the async loop awaits Task.Yield() (then Task.Delay(1) after a few spins) before retrying.
• VectorManager.cs — quantizationTasks (Task[]) initialized to Task.CompletedTask; Dispose drains the channel and awaits Task.WhenAll(quantizationTasks).

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 into ProcessorCount busy-spinners that each pin a pool thread. The exclusive lock is necessary but not sufficient — fixing the waiters (this change: yield the pool thread instead of spinning) 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 follow-up. The rare case where a quantization worker itself hits needsRecreate still blocks one pool thread under the lock, but that is bounded and self-resolving now that the other workers no longer spin.

---

Defect 2 — Object-log flush NullReferenceException

During an object-log page flush (ObjectAllocatorImpl.WriteAsync), a concurrent Upsert/RMW elides a read-only-but-not-yet-flushed record and frees its overflow byte[] / object slot after the page's inline image was copied. The flush then re-derived the on-disk length from the now-freed objectIdMap slot in LogRecord.SetObjectLogRecordStartPositionAndLength, dereferencing a null OverflowByteArray (get_Length) and taking down the flush thread (and the server). Fires in Debug and Release — a real NRE, not an assert.

Fix

Two parts:

1. Authoritative captured lengths. Derive the on-disk key/value lengths from the overflow instances the flusher already captured under the epoch (the same bytes written to the object log) instead of re-reading the objectIdMap slot. Capturing the overflow also pins the backing byte[] while it is still reachable.

2. Live-Invalid gate (never invalidate a Valid record). After the capture, re-read the live record's RecordInfo. Elision of a read-only record is ordered: it (a) CAS's the record out of the tag chain, (b) SealAndInvalidates it — clearing the Valid bit — and only then (c) frees its heap via OnDispose(Elided). So if the live record now reads Invalid, the flusher persists the disk-image copy as Invalid and skips it; the superseding record is already in the chain, so recovery skips the invalid image and the winner is served. A record that is still Valid is never invalidated by the flush — e.g. a Deleted-but-not-elided tombstone that must remain in the chain so reads/recovery do not fall through to an older version. Because the heap is freed strictly after SealAndInvalidate, a still-Valid record's captured heap is always intact; a Debug.Assert guards this.

• LogRecord.cs — SetObjectLogRecordStartPositionAndLength now takes the captured in keyOverflow, in valueOverflow and uses their lengths as authoritative (no fresh objectIdMap lookup).
• ObjectAllocatorImpl.cs — the WriteAsync flush loop re-reads the live RecordInfo after the capture and SetInvalid()s + skips only records the live chain has already elided (Info.Invalid); a Debug.Assert verifies a Valid record's capture is never empty.
• DiskLogRecord.cs — SerializeHeapObjects hoists the overflow locals and passes them through both call sites.

Point 1 — OnDispose on elision-not-to-freelist (verified). For read-only/flush records (logicalAddress < GetMinRevivifiableAddress()), elision does not transfer to the revivification free list, and OnDispose(Elided) is called — InternalUpsert.cs:374 and InternalRMW.cs:675 — which is what frees the objectIdMap slot. One call-out: Helpers.TryTransferToFreeList (Helpers.cs:130-131) returns false without calling hlog.OnDispose for a Delete-orphan below MinRevivifiable, leaving that key-overflow slot unreclaimed until the page closes (a minor leak-until-close, not a correctness bug, and unrelated to the flush NRE).

---

Tests

test/standalone/Garnet.test.vectorset/ConcurrentVaddDiskSpillTests.cs:

• ConcurrentVaddToSpilledSetMakesProgress — concurrent-disk-spill liveness / deadlock guard (~60 s). 8 concurrent VADD workers to one object-log-spilled set; a sustained no-progress window fails as a deadlock. Passes with the cooperative-Tasks fix across repeated runs; the historical hang stalls all workers permanently.
• ConcurrentVaddSpilledToObjectLogDoesNotCrashFlush — object-log flush-crash guard (~12 s). Reproduces the gist's exact crash config (1m memory, 16k page, storage tier) with 8 concurrent VADD workers each on their own ConnectionMultiplexer (a single shared multiplexer serializes on one RespServerSession and never reproduces the race — the critical repro ingredient). Verified a true guard: without the fix the server flush thread crashes and the run aborts (5/5 runs); with the fix it runs clean (10/10 runs). Temporary instrumentation confirmed the live-Invalid gate actually fires (skipping elided records) during the run, while the "empty capture but live-Valid" assert branch never triggered.

Validation

• dotnet build clean (0 warnings/errors, net8.0 + net10.0) for Garnet.server and the vectorset test project.
• Both fixture tests (ConcurrentVaddToSpilledSetMakesProgress ×3, ConcurrentVaddSpilledToObjectLogDoesNotCrashFlush) pass in Debug and Release; all 14 existing WithQuantizationBackfillAsync quantization cases pass (every quant type) — cooperative workers still build/backfill correctly.
• test.recordops (220 object-allocator tests) green — no regression from the allocator change.
• dotnet format Garnet.slnx --verify-no-changes and Tsavorite.slnx --verify-no-changes clean on all changed files.
• Note: the pre-existing InterruptedVectorSetDelete_{BeforeMark,AfterMark} fault-injection tests are flaky on main as well (~40–50% on both base and this branch; a subsequent VSIM on a partially-deleted set can close the connection). They use NOQUANT, so none of this change's code paths execute in them — the flake is unrelated and not introduced here.

Remaining follow-up (separate, deeper — not fixed here)

Point 2 — read-side race that trips the !recordInfo.Invalid assert. AsyncGetFromDiskCallback (AllocatorBase.cs:2282) asserts !recordInfo.Invalid ("Invalid records should not be in the hash chain for pending IO"). It fires when a Valid-bit-clear RecordInfo is persisted for, or observed on, a chain-reachable record whose disk read is already pending: a read is issued while the record is Valid + in-chain, then the record is elided/invalidated before the IO completes, so the completion callback observes it Invalid. The previous SetInvalid()-on-empty-capture logic could actively manufacture this on disk; the live-Invalid gate no longer invalidates any Valid/in-chain record, removing that flush-induced trigger. A deeper pre-existing variant (read-issued-then-elided) remains and is Debug-only (compiled out of Release, which is why the gist's Release repro never hit it). Left for a focused follow-up with its own Tsavorite-level repro.

Delete-source coherence. InternalDelete.CreateNewRecordDelete (InternalDelete.cs:262-263) frees a read-only source's value then Seal()s it, leaving it Valid + Sealed with a freed value heap. A flush that snapshotted such a record mid-transition would see a Valid live record with an empty capture — the one case the Debug.Assert guards. It did not surface in the concurrent-VADD workload (the assert never fired across all runs, and VADD's supersede path is Upsert/RMW-driven elision, not Delete), but a fully general fix would use epoch-deferred object free. Scoped as follow-up; do not attempt a speculative fix without a Tsavorite-level repro.

ARM64. SealAndInvalidate and ObjectIdMap.Free are plain (non-interlocked) stores; the gate's capture-before-info-read ordering is guaranteed by x64 TSO. ARM64 would need an explicit barrier — documented follow-up.

Other ReadVectorIndex callers. Migration and replication replay also call ReadVectorIndex and share the same latent spin-acquire pattern, but they were not part of the reproduced workload (which piles ProcessorCount quantization workers onto one hot set). Scope is intentionally limited to the proven quantization amplifier; making those callers cooperative too is a straightforward follow-up if a repro ever implicates them.

[Copilot]

🟡 Not ready to approve

A few small but concrete correctness/diagnostic issues remain in the modified quantization code (non-volatile cross-thread counter reads and a misleading exception message).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR addresses a deadlock scenario during concurrent VADD on disk-tiered vector sets by preventing quantization work from consuming .NET thread-pool threads, thereby avoiding thread-pool starvation of disk-IO completion work.

Changes:

• Replaces thread-pool Task-based quantization workers with dedicated background Thread workers draining the quantization channel.
• Updates VectorManager lifecycle to shut down quantization via channel completion + thread Join() rather than Task.WhenAll.
• Adds regression tests covering both the mechanism (quantization must not run on thread-pool threads) and liveness (concurrent VADD makes progress under spill conditions).

File summaries

File	Description
test/standalone/Garnet.test.vectorset/ConcurrentVaddDiskSpillTests.cs	Adds targeted regression tests for the deadlock mechanism and for concurrent VADD liveness under object-log spill.
libs/server/Resp/Vector/VectorManager.Quantization.cs	Moves quantization workers from Task to dedicated Thread workers and adds a test hook for thread-pool detection.
libs/server/Resp/Vector/VectorManager.cs	Allocates/join-stops quantization worker threads during VectorManager initialization/disposal.

Review details

Suppressed comments (3)

libs/server/Resp/Vector/VectorManager.Quantization.cs:61

• QuantizationRequestsProcessed is written from worker threads (via Interlocked.Increment) but read from other threads without a volatile read. This can lead to stale reads (and test loops that miss progress) and the doc comment still refers to "tasks" even though this is now thread-based.

This issue also appears on line 62 of the same file.

        /// <summary>
        /// For testing purposes, the number of <see cref="QuantizationStep.BuildQuantizationTable"/> requests processed by <see cref="StartQuantizationTasks"/> tasks.
        /// </summary>
        internal int QuantizationRequestsProcessed => quantizationRequestsProcessed;

libs/server/Resp/Vector/VectorManager.Quantization.cs:65

• QuantizationBackfillsProcessed is updated by worker threads but read elsewhere without a volatile read. This can produce stale reads (and flaky progress detection in tests), and the doc comment still mentions "tasks" after the move to dedicated threads.

        /// <summary>
        /// For testing purposes, the number of <see cref="QuantizationStep.BackfillQuantizedVectors"/> requests processed by <see cref="StartQuantizationTasks"/> tasks.
        /// </summary>
        internal int QuantizationBackfillsProcessed => quantizationBackfillsProcessed;

libs/server/Resp/Vector/VectorManager.Quantization.cs:99

• The exception message refers to a "cleanup session" but this is the quantization worker loop; this makes diagnostics confusing if initialization fails.

                    if (session.activeDbId != self.dbId && !session.TrySwitchActiveDatabaseSession(self.dbId))
                    {
                        throw new GarnetException($"Could not switch VectorManager cleanup session to {self.dbId}, initialization failed");
                    }

• Files reviewed: 3/3 changed files
• Comments generated: 0
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

[Ted Hart (TedHartMS)]

Superseded by #2064, which extracts just the concurrent-VADD deadlock fix (cooperative thread-pool quantization workers) and its regression test against a clean main base.

The object-log flush overflow NullReferenceException work that was bundled here is intentionally left out of #2064. It is retained on branch tedhartms-tedhar-vector-overflow as reference, to be reworked on top of the no-copy tedhar/aof-chunk effort (the no-copy change alters the DiskLogRecord/overflow implementation enough that porting this version isn't worthwhile).