Clear the Vector Set index pointer on the diskless replication stream-in path - #2043
Merged
Tiago Nápoli (tiagonapoli) merged 16 commits intoAug 10, 2026
Merged
Conversation
Tiago Nápoli (tiagonapoli)
requested review from
Ted Hart (TedHartMS),
Badrish Chandramouli (badrishc),
kevin-montrose and
Vasileios Zois (vazois)
as code owners
August 7, 2026 19:13
Tiago Nápoli (tiagonapoli)
force-pushed
the
tiagonapoli/vs-1-diskless
branch
from
August 7, 2026 19:17
bbcd45d to
fde79a3
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a diskless-replication safety issue for Vector Sets by sanitizing streamed Vector Set index records (clearing the native IndexPtr) and by rebuilding/validating Vector Set context-reservation state after a diskless full sync, preventing replicas from dereferencing foreign pointers or reusing streamed contexts.
Changes:
- Sanitize ingested (recovery + diskless full sync) Vector Set records via
VectorManager.SanitizeAndTrackIngestedRecordIfApplicable, and rebuild reservation state viaVectorManager.ReconcileRecoveredState. - Hook sanitization into
CLUSTER SYNCstream-in and run reconciliation after diskless full sync completion. - Add a dedicated cluster replication test project for Vector Sets and wire it into the solution + CI/nightly matrices.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/cluster/Garnet.test.cluster/Garnet.test.cluster.csproj | Adds IVT for the new Vector Set replication test assembly. |
| test/cluster/Garnet.test.cluster/ClusterTestUtils.cs | Adds cluster-formation and replication helpers used by the new tests. |
| test/cluster/Garnet.test.cluster/ClusterTestContext.cs | Adds a dedicated port assignment and convenience helpers for the new replication tests. |
| test/cluster/Garnet.test.cluster.replication.vectorsets/TestProjectSetup.cs | Sets a unique base port for the new test assembly. |
| test/cluster/Garnet.test.cluster.replication.vectorsets/ReplicationTests/VectorSetReplicationTestBase.cs | Adds shared harness utilities for asserting Vector Set replication correctness and index ownership. |
| test/cluster/Garnet.test.cluster.replication.vectorsets/ReplicationTests/ClusterVectorSetDisklessSyncTests.cs | Adds diskless full-sync / failover / migration / exception-injection coverage for Vector Set replication. |
| test/cluster/Garnet.test.cluster.replication.vectorsets/Garnet.test.cluster.replication.vectorsets.csproj | Introduces the new test project and references required components. |
| libs/server/Storage/Functions/GarnetRecordTriggers.cs | Routes recovery snapshot reads through the new sanitize+track helper. |
| libs/server/Resp/Vector/VectorManager.cs | Adds SanitizeAndTrackIngestedRecordIfApplicable, renames/extends post-recovery logic to ReconcileRecoveredState, and makes recovered bookkeeping reusable. |
| libs/server/Resp/Vector/VectorManager.ContextMetadata.cs | Clears pending recovered state on flush (and should also clear dirty bookkeeping; see comment). |
| libs/server/Properties/AssemblyInfo.cs | Adds IVT for the new test assembly. |
| libs/server/Databases/SingleDatabaseManager.cs | Updates vector-set recovery path to call ReconcileRecoveredState(). |
| libs/server/Databases/MultiDatabaseManager.cs | Updates vector-set recovery path to call ReconcileRecoveredState() across databases. |
| libs/cluster/Session/RespClusterReplicationCommands.cs | Sanitizes streamed records during CLUSTER SYNC ingestion. |
| libs/cluster/Server/Replication/ReplicaOps/ReplicaDisklessSync.cs | Reconciles recovered state after diskless full sync completion (and validates reservation is empty on this path). |
| Garnet.slnx | Registers the new test project in the solution. |
| .github/workflows/nightly.yml | Adds the new test project to the nightly test matrix. |
| .github/workflows/ci.yml | Adds the new test project to the CI test matrix. |
kevin-montrose
requested changes
Aug 7, 2026
kevin-montrose
left a comment
Contributor
There was a problem hiding this comment.
The dirty namespaces Copilot seems correct, plus I think some HasNamespace checks are missing (things might still pass in Release, Debug I would expect assert failures).
…-in path A Vector Set index record persists the primary's native DiskANN handle (IndexPtr) inside its value. Diskless sync streams log records verbatim into the replica's memory, so the replica receives the primary's pointer. The only hook that zeroes that field is GarnetRecordTriggers.OnDiskRead, which by definition never fires for records streamed straight into memory. Since VectorManager.NeedsRecreate is exactly indexPtr == 0, a foreign pointer is indistinguishable from a healthy local one: the lazy Service.RecreateIndex rebuild is skipped and the raw value reaches the P/Invoke unvalidated, faulting the replica with SIGSEGV inside NativeDiskANNMethods.card. NetworkClusterSync now clears the pointer as records are deserialized, so the replica rebuilds its own index on first touch. Also rebuilds the context reservation after a diskless full sync. Such a sync SETs the streamed index and ContextMetadata records straight into the store, bypassing the RMW path that maintains the in-memory context-reservation bitmap. Unlike startup recovery, nothing rebuilt it, so after promotion a fresh Vector Set could be handed a context a streamed set already owns, corrupting both. Streamed records are fed through SanitizeAndTrackIngestedRecordIfApplicable during CLUSTER SYNC, and ReconcileRecoveredState rebuilds the reservation once the stream completes. That call passes requireNoReservedContexts on the diskless path: the rebuild replaces contextMetadatas wholesale, so a context surviving the preceding flush would be dropped without its index cleaned up. Adds Garnet.test.cluster.replication.vectorsets, registered in Garnet.slnx and in the CI and nightly matrices. The harness asserts index ownership and element-level equality, since matching cardinality alone is too weak: an aliased index reports the source's count perfectly well. Covers diskless full sync, async replay, multi-database sync, exception-injected aborted syncs, and FreshVectorSetDoesNotReuseStreamedContextAfterDisklessFullSync for the context-reservation regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b280844-5962-4e83-9a36-c9f9b64639b0
Tiago Nápoli (tiagonapoli)
force-pushed
the
tiagonapoli/vs-1-diskless
branch
from
August 7, 2026 19:35
fde79a3 to
7a8783c
Compare
…cordIfApplicable A record with no namespace reports a single 0 byte rather than an empty span, so the ContextMetadata check must be a positive test for MetadataNamespace. Testing for a non-empty namespace instead routes index records away from ClearIndexPointer, which reintroduces the foreign IndexPtr fault this change fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b280844-5962-4e83-9a36-c9f9b64639b0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b280844-5962-4e83-9a36-c9f9b64639b0
ISourceLogRecord extends IKey, so HasNamespace/NamespaceBytes are available on the generic constraint. This restores the shape used by the recovery path. Note that testing the namespace span for emptiness is not equivalent: a record without a namespace reports a single 0 byte, which IKey documents as reserved. Treating that as 'namespaced' routes index records away from ClearIndexPointer and faults the replica. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b280844-5962-4e83-9a36-c9f9b64639b0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b280844-5962-4e83-9a36-c9f9b64639b0
Also null-guard the VectorManager reconcile on the diskless full sync path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5883e70d-95c1-49ec-981d-ff98afd48a5d
PopulateVectorSet now derives its seed from the test name, key, and call ordinal via FNV-1a, so each vector set still gets distinct data that reproduces across runs without callers maintaining a numbering scheme. String.GetHashCode is randomized per process and cannot be used here. Also folds VectorSetsStayPartitionedAcrossDisklessFullSync into VectorSetReadableOnReplicaAfterDisklessFullSync, which already covers the same sync path and now carries the multi-set cardinality isolation checks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5883e70d-95c1-49ec-981d-ff98afd48a5d
Embeddings are read back from both nodes and compared at runtime rather than regenerated, so the assertions never depend on the vector data being reproducible. Successive Random instances still yield distinct data per vector set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5883e70d-95c1-49ec-981d-ff98afd48a5d
Migration transfers Vector Sets through its own path: the destination reserves contexts via CLUSTER RESERVE VECTOR_SET_CONTEXTS and the index record is remapped with SetContextForMigration before it is sent. SanitizeAndTrackIngestedRecordIfApplicable is only reached from CLUSTER SYNC stream-in and recovery, so no record in that test crossed the code this fixture covers. Vector Set migration is already covered by ClusterVectorSetTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5883e70d-95c1-49ec-981d-ff98afd48a5d
…nc tests The injection flags are global statics, so a test that fails to disable one leaks it into everything that runs after it. ExceptionInjectionHelper.Enabled returns a struct that disables on dispose, replacing the hand written try/finally pairs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5883e70d-95c1-49ec-981d-ff98afd48a5d
added 3 commits
August 10, 2026 09:12
AssertFullyReplicated already waits for the replica AOF offset to catch up, which the replica only publishes after ReconcileRecoveredState completes, so the extra key-visibility poll added nothing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5883e70d-95c1-49ec-981d-ff98afd48a5d
… allocator Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5883e70d-95c1-49ec-981d-ff98afd48a5d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5883e70d-95c1-49ec-981d-ff98afd48a5d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5883e70d-95c1-49ec-981d-ff98afd48a5d
FLUSHALL walks every active database, and a database that was only ever created by SELECT has a VectorManager whose Initialize has not run, so dirtyContextMetadatas is still null and clearing it killed the session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5883e70d-95c1-49ec-981d-ff98afd48a5d
Tiago Nápoli (tiagonapoli)
force-pushed
the
tiagonapoli/vs-1-diskless
branch
from
August 10, 2026 16:48
decc54e to
5c210fc
Compare
kevin-montrose
approved these changes
Aug 10, 2026
Tiago Nápoli (tiagonapoli)
merged commit Aug 10, 2026
312f713
into
microsoft:main
332 of 333 checks passed
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.
Problem
A Vector Set index record persists the primary's native DiskANN handle (
IndexPtr) in its value. Diskless sync streams records verbatim into the replica's memory, so the replica receives the primary's pointer.The only hook that zeroes that field is
GarnetRecordTriggers.OnDiskRead, which never fires for records streamed straight into memory. SinceNeedsRecreateis exactlyindexPtr == 0, a foreign pointer looks healthy: the lazyRecreateIndexrebuild is skipped and the raw value reaches the P/Invoke, faulting the replica with SIGSEGV insideNativeDiskANNMethods.card.Fix
NetworkClusterSyncclears it as records are deserialized, so the replica rebuilds its own index on first touch.ContextMetadatarecords directly, bypassing the RMW path that maintains the in-memory reservation bitmap. Nothing rebuilt it, so after promotion a fresh Vector Set could be handed a context a streamed set already owns, corrupting both. Streamed records now go throughSanitizeAndTrackIngestedRecordIfApplicable, and the reservation is rebuilt viaReconcileRecoveredStateonce the stream completes. ItsrequireNoReservedContextsflag fails loudly if a context survived the preceding flush, since the rebuild replacescontextMetadataswholesale and would otherwise leak that index.FlushGuard.DisposeresetcontextMetadatasbut leftdirtyContextMetadataspopulated, so a concurrentUpdateContextMetadatadrain could index out of range, or re-persist metadata for a context the flush just removed. Now cleared with the rest of the invalidated state.Tests
Adds
Garnet.test.cluster.replication.vectorsets, registered inGarnet.slnxand the CI/nightly matrices.The harness asserts index ownership and element-level equality — matching cardinality alone is too weak, since an aliased index reports the source's count perfectly well. Covers diskless full sync, async replay, multi-database sync,
FreshVectorSetDoesNotReuseStreamedContextAfterDisklessFullSyncfor the reservation regression, and two exception-injection tests for an aborted full sync (recovery after retry, and no orphaned namespaced records left behind).Deferred (separate PRs)
IndexPtrAVE + disk-based reservation gap).