Fix VectorSessionFunctions element grow/shrink corruption (VectorSet Attributes) + tests#1967
Merged
tiagonapoli merged 28 commits intoJul 23, 2026
Merged
Conversation
Add unit tests over VectorSessionFunctions' element sub-store write path that pin the VectorSet Attributes recovery corruption at the unit level (no Docker / CPU-throttle repro needed). The tests drive a dedicated Vector session directly and Upsert a namespaced element value, mirroring VectorManager's DiskANN write callback. Growing a variable-length element in the mutable region hits InPlaceWriter, which copies without honoring the destination capacity: grow overflows (ArgumentException) and shrink leaves stale trailing bytes. The read-only-region cases (fresh InitialWriter record), round-trip, and same-size overwrite pass. Grow(False)/Shrink(False) fail today, demonstrating the bug; the production fix is intentionally not included in this change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Drop the AttributesNamespace constant and its DiskANN-specific comment; the Writer path has no namespace-specific behaviour, so the tests just use an arbitrary single-byte namespace inline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Combine the same-size / grow / shrink Upsert tests into a single multi-stage VectorElementUpsertResizePreservesValue that walks one element through initial write -> equal-size overwrite -> grow -> shrink, verifying the round-trip at each stage, keeping the inReadOnlyRegion parameterization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
InPlaceWriter did newValue.CopyTo(value) against the existing record's value span with no capacity/length handling, so an in-place overwrite of a variable-length element (e.g. the per-element Attributes blob) would overflow the record on a grow (ArgumentException) or leave stale trailing bytes and a wrong content length on a shrink. Mirror the main-store pattern: build the RecordSizeInfo for the new value and call TrySetContentLengths, which resizes/shortens the value in place and returns false when the value cannot grow within the allocated record. On false, return false so Tsavorite falls back to allocating a fresh, correctly-sized record via InitialWriter. Only copy once the length is set. VectorElementUpsertResizePreservesValue now passes in both the mutable (InPlaceWriter) and read-only (InitialWriter) regions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
CopyUpdater and InPlaceUpdater had the same unbounded-copy shape that was fixed in InPlaceWriter: they assumed the new value was the same size as the old one. CopyUpdater (callback branch) did oldValueAligned.CopyTo(newValueAligned) with no bound, overflowing the freshly-allocated destination on a shrink (old larger than new), asserted the contradictory WriteDesiredSize <= oldValueAligned.Length (which fails on a grow), and returned true without setting the content length. Now it carries over only the overlapping prefix of the old value before the callback repopulates the buffer, and returns TrySetContentLengths so the destination length is correct. InPlaceUpdater (callback branch) handed the existing record's value span to the native callback and let it write WriteDesiredSize bytes with no capacity check, overflowing the record on a grow and leaving a stale content length on a shrink. Now it resizes the record via TrySetContentLengths first, returning false (-> Tsavorite RCU to CopyUpdater with a fresh record) when the value cannot grow in place, then re-aligns before invoking the callback. These paths are only reached today with fixed-size DiskANN terms and the fixed-size ContextMetadata, so the change is behavior-preserving for the same-size case; the full Garnet.test.vectorset suite is unchanged (143 passed, 22 skipped, 0 failed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
The RMW data-fill callback is a SuppressGCTransition function pointer that cannot dispatch to a managed method, so the InitialUpdater/CopyUpdater/ InPlaceUpdater resize paths had no unit coverage. Add a DEBUG-only VectorInput.TestCallback that the updaters invoke via a plain Cdecl call when set, routed through a shared InvokeDataCallback helper. Compiled out of Release, so it has zero production footprint. Add VectorElementRmwResizePreservesValue (mutable -> InPlaceUpdater, read-only -> CopyUpdater) exercising create/same-size/grow/shrink and asserting exact length and no residue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Reuse the real Callback pointer and add a DEBUG-only IsTestCallback flag that selects the plain-Cdecl (managed) invocation instead of SuppressGCTransition. Drops the (nint)1 sentinel from the test, which now sets Callback to the real managed fill pointer directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Replace the #if DEBUG guards around VectorElementRmwResizePreservesValue and its helpers with a runtime TestUtils.IgnoreIfNotDebugBuild() call (mirroring IgnoreIfExceptionInjectionDisabled). The test now compiles in Release and is reported as Skipped there, while still running in Debug where the managed IsTestCallback hook is honored. Also tidy comments and switch InvokeDataCallback to a ref parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Condense the multi-line test/helper summaries in RespVectorSetTests and the resize-path comments in VectorSessionFunctions to concise one/two-liners. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
The CopyUpdater destination is freshly allocated at the new value size, so the DiskANN callback fully repopulates it (same contract InitialUpdater relies on). Drop the redundant old-value carry-over copy and just Clear() before the callback, matching InitialUpdater. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
RMW values in the vector store are always fixed-size: DiskANN only issues RMW against Neighbors ((max_degree+1)*u32) and the FSM metadata (BLOCK_SIZE_BYTES), both matched to the size they were first written at, and AlignOrPin always allocates that fixed size + ValueAlignmentBytes. Only Attributes are variable length, and those go through Upsert, not RMW. Drop the RMW grow/shrink handling that this can never exercise: - CopyUpdater carries the current value forward with a plain fixed-size copy (the callback reads it for FSM bit flips / neighbour appends). - InPlaceUpdater modifies the already-correctly-sized record in place instead of resizing it. Remove the now-unused DEBUG-only test callback machinery (VectorInput.IsTestCallback, InvokeDataCallback's DEBUG branch, TestUtils.IgnoreIfNotDebugBuild) and the VectorElementRmwResizePreservesValue test that drove the non-existent resize. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Subsumed by the first stage of VectorElementUpsertResizePreservesValue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Adds same-size, sub-alignment and cross-alignment grow/shrink stages, asserting the expected in-place vs reallocation record path for each. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
tiagonapoli
marked this pull request as ready for review
July 22, 2026 23:20
Drives grow/shrink by 1,2,3,4,5,20 bytes each way against a fixed base, deriving the expected in-place vs reallocate record path from the tracked 8-byte bucket capacity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a variable-length VectorSet element Upsert corruption/exception issue by ensuring VectorSessionFunctions.InPlaceWriter correctly resizes (or declines in-place growth) before copying the new payload, and adds a regression test that exercises create → same-size overwrite → grow → shrink across both mutable and read-only regions.
Changes:
- Update
VectorSessionFunctions.InPlaceWriterto build/populateRecordSizeInfoand callTrySetContentLengths(...)prior to copying, so grow/shrink updates don’t overflow or leave stale trailing bytes. - Add
VectorElementUpsertResizePreservesValuetest that drives the element sub-store Upsert/Read path and validates exact round-trips after each resize stage (mutable in-place vs read-only reallocation).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/standalone/Garnet.test.vectorset/RespVectorSetTests.cs | Adds a regression test covering element Upsert value resizing (create/overwrite/grow/shrink) in both mutable and read-only regions. |
| libs/server/Storage/Functions/VectorStore/VectorSessionFunctions.cs | Fixes InPlaceWriter to resize/shorten in-place safely via RecordSizeInfo + TrySetContentLengths, falling back to reallocation when needed. |
Comments suppressed due to low confidence (1)
test/standalone/Garnet.test.vectorset/RespVectorSetTests.cs:3984
- If the Read goes pending, calling CompletePending(wait:true) does not update the local
status/input/output. Subsequent asserts will still see the original Pending status (Found will be false) and may miss the updated ReadDesiredSize recorded by the Reader. Use CompletePendingWithOutputs to retrieve the completed Status (and Input/Output) before asserting.
var status = context.Read(elementKey, ref input, ref output);
if (status.IsPending)
_ = context.CompletePending(wait: true);
ClassicAssert.IsTrue(status.Found, "reading a written element must find the record");
ClassicAssert.IsTrue(output.SpanByteAndMemory.IsSpanByte, "the element read must stay on the pinned span");
Drop the 8-byte-bucket capacity model and record-path assertions; just overwrite one element with shorter/longer values (1-10B) and verify each reads back verbatim with no stale bytes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad22029-9339-4e1a-85cc-bb7597125bb6
kevin-montrose
approved these changes
Jul 23, 2026
tiagonapoli
deleted the
tiagonapoli/vectorset-attributes-grow-shrink-tests
branch
July 23, 2026 19:27
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
Fixes a VectorSet element corruption bug in
VectorSessionFunctions.InPlaceWriter, and adds a unit test over the element sub-store write path that pins it.The bug
InPlaceWriterdidnewValue.CopyTo(value)straight into the existing record's value span with no capacity or content-length handling. For a variable-length per-element value (the SETATTR Attributes blob), an in-place overwrite in the mutable region would:ArgumentException: Destination is too short.The base contract computes a
RecordSizeInfoand returnsfalsewhen the new value won't fit, letting Tsavorite allocate a fresh record — the Vector override skipped all of it.The fix
Mirror the main-store pattern: build the
RecordSizeInfofor the new value,PopulateRecordSizeInfo, thenTrySetContentLengths, which resizes/shortens the value in place and returnsfalsewhen it cannot grow within the allocated record. Onfalse, returnfalseso Tsavorite falls back to a fresh, correctly-sized record viaInitialWriter. Only copy once the length is set.Test
VectorElementUpsertResizePreservesValue([Values(false, true)] inReadOnlyRegion)drives a dedicated Vector session directly (mirroringVectorManager's DiskANN write callback) and walks a single element through initial write -> equal-size overwrite -> grow -> shrink, verifying the exact round-trip after each stage. It covers both the mutable region (InPlaceWriter) and the read-only region (freshInitialWriterrecord). PlusVectorElementUpsertRoundTripsas a baseline.Before the fix, the mutable-region case failed at the grow stage; with the fix both regions pass.
Validation
VectorElement*tests: 6/6 pass (both TestFixtures).Garnet.test.vectorsetsuite: 143 passed, 22 skipped, 0 failed — no regressions.Scope note
Only the Upsert/
InPlaceWriterpath is fixed here — that is the path attributes actually use (WriteCallbackUnmanaged->Upsert) and the one the repro hit. The RMW/Updater callback path (InitialUpdater/InPlaceUpdater/CopyUpdater) has the same latent shape but is only invoked by DiskANN with fixed-size data and isn't reachable variable-size through any command, so it is intentionally left unchanged.