Vector Set recall smoke tests - #1990
Draft
badrishc wants to merge 4 commits into
Draft
Conversation
Adds VectorSetRecallSmokeTests: recall-oriented smoke tests that stress a
DiskANN Vector Set graph across the physical storage configurations that
matter for disk-tiered operation:
* quantization mode (NOQUANT / Q8 / BIN)
* larger-than-memory: records spill to the storage tier during load, or
are evicted to disk after an in-memory build
* read-cache vs copy-reads-to-tail vs neither
* save -> restart -> recover, optionally re-evicting the recovered graph
The invariant under test is physical robustness: a graph that answers
queries well in memory must keep answering them well once the same records
are served from disk or recovered from a checkpoint. Each test builds a
small, well-clustered, deterministic graph (Dim=32, fixed seed, single
connection, sequential VADD), measures recall@10 against a brute-force
ground truth, applies a physical stressor, and asserts recall does not
collapse. Spill is forced and verified with DEBUG FLUSHANDEVICT plus the
store head/tail addresses. The graphs are intentionally tiny so the whole
suite runs in ~30s.
Known-failing cases: the Q8 cases that read from disk (FlushAndEvict,
larger-than-memory load, and recover) currently fail against
diskann-garnet 4.0.0. The per-dimension Q8 quantization table is native
in-memory-only state that is lost when the index is recreated after
eviction/recovery and cannot be rebuilt, so stored codes are decoded with a
missing table and recall collapses (~1.0 -> ~0.03-0.33). NOQUANT and BIN
(table-free) are robust across every configuration. These failures are
deliberate regression guards for that bug, not flaky tests.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Recovering a checkpoint onto a "smaller box" (a much smaller log) must keep the Vector Set working correctly. Reworks RecallSurvivesSaveAndRecover so the `recoverIntoSmallerLog` case recovers into a 64k log (down from 8m) while holding the page size fixed at a valid 16k, so the recovered graph no longer fits in memory and is served from disk. Asserts the records actually spilled and that recall is preserved. Verified: NOQUANT and BIN preserve recall (1.0) when recovered into the smaller log; only Q8 collapses, which is the known quantization-table bug. (An earlier iteration recovered with the lowMemory helper, which forces a 4k page size below the 16k minimum the server enforces for Vector Sets; that is an unsupported geometry and is not what "recover to a smaller log" means.) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
tiagonapoli
reviewed
Jul 29, 2026
| return total / data.Queries.Length; | ||
| } | ||
|
|
||
| private static int[] BruteForceTopK(float[][] vectors, float[] query) |
Collaborator
There was a problem hiding this comment.
nit: we have DiskANNSyntheticRecallTests which has methods duplicated with some here, maybe worth evaluating if it's possible to reuse? Or updating DiskANNSyntheticRecallTests to add fixtures that will also add quantization and spilling to disk?
VectorSessionFunctions.CopyUpdater copies the full existing value into the
new record (so the DiskANN read-modify-write callback can read it back)
before the callback runs. The new record was sized from
GetRMWModifiedFieldInfo to only WriteDesiredSize + ValueAlignmentBytes, which
is smaller than the existing value when a record is rewritten to a smaller
size. In that case oldValueAligned.CopyTo(newValueAligned) overflows the
destination and throws ArgumentException ("Destination is too short"),
crashing the server. This only occurs on the pending (disk) CopyUpdater path
— in-memory updates use InPlaceUpdater, which does not copy — so it surfaces
under heavy eviction (e.g. a small page size / large graph).
Size the destination to max(WriteDesiredSize + ValueAlignmentBytes,
existingValue.Length) so the copy can never overrun the destination,
regardless of whether the update grows or shrinks the record. The common
(growing / same-size) case is unchanged.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ecords" This reverts commit 9a71407. The fix guarded a condition that does not occur. Instrumented investigation (per-(namespace,key) write-size tracking with liveness confirmation, plus a disk-chain-walk namespace/key match check, run isolated and across the full smoke suite at 4 KB pages under CPU stress) established: - DiskANN neighbor/quant/FSM RMWs never change a record's size. Exactly two copy/inplace shapes are produced and each is perfectly constant across a build (NeighborList 68 B inline; the 8192 B overflow record). Zero size changes were ever observed for any key. - The pending (disk) CopyUpdater always matches the correct key AND namespace for DiskANN records (zero mismatches). GarnetKeyComparer's generic path compares NamespaceBytes + KeyBytes and correctly distinguishes the two namespaces, so a small-record RMW never copy-updates a larger record. Given the source value equals WriteDesiredSize + ValueAlignmentBytes for a correctly-matched, size-stable record, oldValueAligned.Length always equals newValueAligned.Length, so the CopyTo cannot overflow in normal operation. max(WriteDesiredSize + ValueAlignmentBytes, value.Length) therefore protects against nothing that happens; worse, if ValueSpan.Length were ever transiently corrupt (large), it would attempt a huge allocation (OOM) instead of failing fast. Any future "Destination is too short" here must be root-caused as a race/corruption, not sized around. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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
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.
What
Recall-oriented smoke tests for Garnet Vector Sets that stress a DiskANN graph across the physical storage configurations that matter for disk-tiered operation. (A CopyUpdater sizing change was briefly included and has since been reverted — see the last section.)
Smoke test coverage
Invariant
Physical robustness: a graph that answers queries well while resident in memory must keep answering them well once the same records are served from disk or recovered from a checkpoint.
Each test builds a small, well-clustered, deterministic graph (Dim=32, 16 clusters, M=16, EF=64, fixed seed, single connection, sequential
VADD), measures recall@10 against a brute-force ground truth, applies a physical stressor, and asserts recall does not collapse (MaxRecallDrop = 0.20). Spill is forced and verified withDEBUG FLUSHANDEVICT+ the store head/tail addresses. Everything is single-threaded and the graphs are intentionally tiny, so the whole suite runs in ~30s.Tests (18 cases)
RecallSurvivesFlushAndEvict(quant, {none/readcache/copytotail})RecallSurvivesLargerThanMemoryLoad(quant)RecallSurvivesSaveAndRecover(quant, recoverIntoSmallerLog)Known-failing cases (deliberate regression guards)
The Q8 cases that read from disk (FlushAndEvict, larger-than-memory load, and recover) currently fail against
diskann-garnet 4.0.0. The per-dimension Q8 quantization table is native in-memory-only state that is lost when the index is recreated after eviction/recovery and cannot be rebuilt, so stored codes are decoded with a missing table and recall collapses (~1.0 to ~0.03-0.33). NOQUANT and BIN (table-free) are robust across every configuration.Current result against
main(diskann 4.0.0): 12 pass / 6 fail, where the 6 failures are exactly the Q8 disk cases:These are intentional regression guards for the Q8 disk-recall bug - they should start passing once the quantization table is persisted with (or rebuildable for) a recreated/recovered index. Opening as draft since these Q8 cases are expected red until that fix lands.
CopyUpdater "Destination is too short" — investigated, fix reverted
An earlier commit sized the
CopyUpdaterdestination tomax(WriteDesiredSize + ValueAlignmentBytes, existingValue.Length)to stop a rareArgumentException("Destination is too short") seen during a BIN larger-than-memory build at 4 KB pages. That commit has been reverted after root-cause investigation showed it guards a condition that does not occur.Instrumented runs (per-
(namespace,key)write-size tracking with liveness confirmation, plus a disk-chain-walk namespace/key match check, run isolated and across the full smoke suite at 4 KB pages under CPU stress) established:GarnetKeyComparer's generic path comparesNamespaceBytes + KeyBytes, so a small-record RMW never copy-updates a larger record.Because the source value equals
WriteDesiredSize + ValueAlignmentBytesfor a correctly-matched, size-stable record,oldValueAligned.Lengthalways equalsnewValueAligned.Lengthand theCopyTocannot overflow in normal operation. Themax(...)sizing therefore protects against nothing that happens; worse, ifValueSpan.Lengthwere ever transiently corrupt (large) it would attempt a huge allocation (OOM) rather than fail fast. The original crash was most plausibly the separate index-record recreate path (already correctly sized in HEAD viaVectorManager.IndexSize) or an extremely rare eviction/pending-IO race; either way it must be root-caused as a race/corruption, not sized around.Notes
Garnet.test.vectorsetconventions (TestBase,TestUtils.CreateGarnetServer,VADD/VSIMvia StackExchange.Redis, binary int element ids).Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com