Repository navigation
Update DiskANN FFI - #1992
Merged
Merged
Update DiskANN FFI#1992
Conversation
This was referenced Jul 29, 2026
…allback is specified, violates documented contract to do so (and corrupts quantizer data in practice)
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiple unresolved critical and moderate correctness, memory-safety, resource-leak, and protocol issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 9
Open (16)
What changed in this PR
Updates Garnet’s DiskANN FFI and adds vector neighbor and random-member support.
Changes:
- Extends callbacks and native signatures for logging, filtering, continuations, neighbors, and random members.
- Implements
VLINKSandVRANDMEMBERacross storage, API, RESP, and tests. - Updates alignment, quantization, migration, documentation, and DiskANN package dependencies.
| File | Summary and findings |
|---|---|
website/docs/dev/vector-sets.md |
Documents the expanded DiskANN ABI. Nit (1 vote): build_quant_table is incorrectly documented as returning void instead of bool.Nit (1 vote): dataLengthLength should be dataLength.Nit (1 vote): Correct the whcih typo.Nit (1 vote): search_exploration_factor should be documented as uint, not int.Nit (1 vote): Replace the obsolete QuantizationRequested enum name with SuccessStartTraining. |
website/docs/commands/vector-sets.md |
Removes the preview warning. Nit (4 votes): Add VLINKS and VRANDMEMBER syntax/replies and update support tables and checklists. |
test/standalone/Garnet.test.vectorset/VectorSetRecallSmokeTests.cs |
Adjusts recall test load. No unresolved issue identified. |
test/standalone/Garnet.test.vectorset/RespVectorSetTests.cs |
Adds continuation, VLINKS, and VRANDMEMBER tests.Nit (1 vote): Use nameof(VLINKSContinueSearchAsync) for the continuation test key. |
test/standalone/Garnet.test.vectorset/ConcurrentVaddDiskSpillTests.cs |
Expands quantizer coverage. Moderate (1 vote): Reset the reused done counter between test cases to avoid masking failures. |
test/standalone/Garnet.test.extensions/DiskANN/DiskANNServiceTests.cs |
Updates FFI integration tests. No unresolved issue identified. |
libs/server/Storage/Session/MainStore/VectorStoreOps.cs |
Adds neighbor and random-member operations. Moderate (4 votes): A valid element with zero neighbors is incorrectly returned as NOTFOUND.Moderate (2 votes): Handle int.MinValue before applying Math.Abs to VRANDMEMBER count. |
libs/server/Storage/Functions/VectorStore/VectorSessionFunctions.cs |
Updates vector callback handling. Critical (3 votes): Overflow-backed values must remain pinned while native/filter callbacks use their pointers. Critical (1 vote): Restore guaranteed four-byte alignment for callback-backed RMW records. Critical (1 vote): Copy only WriteDesiredSize bytes into the resized callback destination. |
libs/server/Resp/Vector/VectorManager.Quantization.cs |
Handles quantization backfill failures and shutdown. No unresolved issue identified. |
libs/server/Resp/Vector/VectorManager.Migration.cs |
Updates migrated index recreation callbacks. No unresolved issue identified. |
libs/server/Resp/Vector/VectorManager.Locking.cs |
Updates index creation callbacks. No unresolved issue identified. |
libs/server/Resp/Vector/VectorManager.Filter.cs |
Changes filtering to receive attribute data directly. No unresolved issue identified. |
libs/server/Resp/Vector/VectorManager.cs |
Adds continuation, neighbor, and random-member logic. Critical (1 vote): Directly assigning Memory leaves a stale stack-backed SpanByte, risking writes beyond the stack allocation.Moderate (4 votes): Dispose each previous continuation ID buffer before replacing it. Moderate (4 votes): Free the previous pinned GCHandle on every duplicate-sampling retry.Critical (3 votes): Read ID length prefixes as 32-bit values, not 16-bit values. Moderate (3 votes): Dispose the old pooled owner when growing the result buffer. Moderate (1 vote): Recompute the retry write cursor instead of double-advancing remainingIds.Critical (1 vote): Fill the removed suffix after duplicate compaction rather than overwriting valid copied IDs. |
libs/server/Resp/Vector/VectorManager.Callbacks.cs |
Updates DiskANN callback plumbing. Moderate (4 votes): Pass the computed value length including record overhead to InitialIORecordSize. |
libs/server/Resp/Vector/RespServerSessionVectors.cs |
Adds VLINKS and VRANDMEMBER responses.Critical (4 votes): The WITHSCORES response emits an invalid aggregate; emit one two-item member/score array.Nit (1 vote): Update and regenerate VLINKS command metadata.Nit (1 vote): Update and regenerate VRANDMEMBER command metadata.Critical (2 votes): RESP2 no-score results incorrectly wrap each ID in a nested array. Critical (1 vote): Size the random_members output buffer for the requested count, prefixes, and variable-length IDs. |
libs/server/Resp/Vector/DiskANNService.cs |
Updates native DiskANN wrappers and ABI declarations. No unresolved issue identified. |
libs/server/InputHeader.cs |
Removes obsolete alignment state. No unresolved issue identified. |
libs/server/API/IGarnetApi.cs |
Updates vector API contracts. No unresolved issue identified. |
libs/server/API/GarnetWatchApi.cs |
Updates watched API forwarding. No unresolved issue identified. |
libs/server/API/GarnetApi.cs |
Updates standard API forwarding. No unresolved issue identified. |
libs/cluster/Server/Migration/MigrateOperation.cs |
Removes obsolete migration alignment setup. No unresolved issue identified. |
Directory.Packages.props |
Updates diskann-garnet to 5.0.3. No unresolved issue identified. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ecause we're only ever going to use 1-byte of the 4-bytes we have allocated, but still best to be correct here
…hey need to be passed to a callback
kevin-montrose
requested review from
Badrish Chandramouli (badrishc) and
Tiago Nápoli (tiagonapoli)
September 23, 2026 14:48
Badrish Chandramouli (badrishc)
approved these changes
Sep 24, 2026
x@01 (x-at-01)
added a commit
to webc-fork/garnet
that referenced
this pull request
Oct 4, 2026
…lude TestBase directly for VS builds (microsoft#2173)
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.
Making what are (hopefully) the final big FFI changes before stabilization.
nint logCallbacktocreate_indexlogCallbackis:LogCallbackUnmanaged(ulong context, nint logMessage, nuint logMessageLength)contextlow bits are used to enrich log line on the Garnet sidelogMessageis utf8 encoded textlogMessageLengthis the byte count oflogMessagevalueLengthHinttoreadCallbackActiveReadGeometryestimates on the Garnet side - allowing DiskANN to own record size informationoverflow_results(ulong context, nint index, nint continuation, nint output_ids, nuint output_ids_len, nint output_distances, nuint output_distances_len, nint new_continuation)search_xxx(...)function can't fit all of the results into the providedoutput_idsbuffercontinuationcontinuation- should be null if an error is encounteredbyte random_members(ulong context, nint index, uint count, nint output_ids, nuint output_ids_len)VRANDMEMBERcountrandom (external) ids and places them inoutput_ids, length prefixedint search_neighbors(ulong context, nint index, nint id_data, nuint id_len, nint output_ids, nuint output_ids_len, nint output_distances, nuint output_distances_len, nint continuation)VLINKSwhich is basically "give me the neighbors of an element + distances to each"continuationwill be set and search can continue withcontinue_search(...)filter_callbackto accept data in place rather than the parsed internal idbackfill_quant_vectorsdataCallbacksare 4-byte aligned