Fix VectorElementKey.IsPinned and drop_index calls while Vector Set in use#1871
Merged
Conversation
…akes VectorElementKey.IsPinned => true actually correctly
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes VectorElementKey.IsPinned correctness for pending IO by ensuring the key’s namespace bytes live in stable native/pinned memory (rather than stack-only lifetime), restoring the prior “namespace stored adjacent to key bytes” approach expected by the DiskANN interop paths.
Changes:
- Updated
VectorElementKeyto carry namespace bytes via aReadOnlySpan<byte>-based constructor (pointer-backed on !NET9) instead of a single storedbyte. - Updated DiskANN callback/batch key materialization to write the namespace into reserved bytes immediately preceding the key data.
- Adjusted vector-set tests to reset the “smashed” length-prefix byte after exercising
VectorReadBatch.GetKey.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/standalone/Garnet.test.vectorset/RespVectorSetTests.cs | Updates tests to call CompletePending so in-place namespace writes don’t permanently corrupt the input buffer; removes an assertion in MakeVectorElementKey. |
| libs/server/Resp/Vector/VectorManager.Migration.cs | Updates migration paths to build VectorElementKey using the new namespace-bytes ctor. |
| libs/server/Resp/Vector/VectorManager.cs | Updates metadata key construction for the new ctor. |
| libs/server/Resp/Vector/VectorManager.ContextMetadata.cs | Updates context-metadata RMW key construction for the new ctor. |
| libs/server/Resp/Vector/VectorManager.Cleanup.cs | Updates cleanup delete key construction for the new ctor. |
| libs/server/Resp/Vector/VectorManager.Callbacks.cs | Writes namespace into reserved bytes before key data; restores “smashed” length prefix on advance/complete; updates key construction for new ctor. |
| libs/cluster/Server/Migration/MigrateSessionCommonUtils.cs | Updates cluster migration read path to build VectorElementKey using namespace bytes. |
… indexes; this isnecessary because an eviction might cause an index to 'need to be dropped' during another operation; subtlety here is that we also need to block recreation until the drop is processed or we can end up with two active DiskANN indexes for the same logical Vector Set; introduces a test from Badrish which originally revealed the break
kevin-montrose
changed the title
VectorElementKey.IsPinnedVectorElementKey.IsPinned and drop_index calls while Vector Set in use
…(I think), but it'd be reasonable to do some cleanup in using callbacks in the future
…sult in a double free
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
badrishc
approved these changes
Jun 15, 2026
…r GetKey() call; necessary for ConditionallyHoistedKey to work
badrishc
approved these changes
Jun 16, 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.
Fixing
VectorElementKey.IsPinnedNamespaceBytesis only pinned, currently, if the stack doesn't unwind - which isn't a safe assumption in the face of pending IO.Either uses the space DiskANN provides before the key (for single read/write ops), or
stackallocs space forVectorReadBatchwhich survives acrossGetKey(...)calls.Fixing
drop_indexCalls While Vector Set In UseEviction (ie. moving a record to disk) can occur on an index key while a Vector Set is in use - today that dies horribly.
This change
GarnetRecordTriggers.OnEvictto request adrop_indexcall rather than perform one - that call happens on a background task, under an exclusive lock (guaranteeing no DiskANN operations in progress), and recreation of the same index is blocked until it occurs.Together these fix the issues found in Badrish's
VADDLowMemoryStorageTierForcesDiskSpilltest, which is also integrated as part of this PR.