Fix VectorSet recreate livelock from synthetic replication RMW on tombstoned key by tiagonapoli · Pull Request #1934 · microsoft/garnet

tiagonapoli · 2026-07-15T02:57:01Z

Problem

On the primary, a single VectorSet key can spin forever in the recreate loop and peg all cores. The log shows RECREATE ctx=0 dims=0 indexPtr=0 with no preceding INITIAL-CREATE — i.e. a fully-zeroed 56-byte index record that reads back as OK, so NeedsRecreate (which only checks indexPtr == 0) returns true forever and RecreateIndex(dims: 0) can never produce a valid pointer.

Root cause

A VADD is phrased as a read once the index exists, so after a successful add VectorManager.ReplicateVectorSetAdd issues a synthetic VADDAppendLogArg RMW on the index key purely to get the write into the AOF. That synthetic arg is only meaningful as a CopyUpdater on an already-existing record.

NeedInitialUpdate had no VADD/VREM case, so these CU-only synthetic args (VADDAppendLogArg, VREMAppendLogArg, RecreateIndexArg, VADDSetFlagsArg, Migrate*) fell through to default -> return true. VADD holds only a shared vector lock across add + replicate, and UNLINK's raw main-store DELETE takes no vector lock, so a concurrent UNLINK can tombstone the key in between. The synthetic RMW then lands on an absent key: NeedInitialUpdate returns true, InitialUpdater's no-op branch leaves the pre-sized 56-byte record zeroed, and the tombstoned key is resurrected as a phantom recreate-flagged record → infinite recreate loop → CPU storm.

Fix

Add explicit VADD/VREM cases to NeedInitialUpdate that refuse to create a record for the CU-only synthetic args (the key must already exist, matching the existing RIPROMOTE/RIRESTORE precedent). A genuine VADD create carries arg1 == 0 and still creates normally.

Testing

Adds a deterministic two-connection race reproducer (in RespVectorSetTests) that exercises the real command paths rather than calling the internal replicate method directly:

A test-only hook (VectorManager.OnBeforeSyntheticReplicationRmw) fires inside the synthetic-replication window of ReplicateVectorSetAdd/Remove — after the vector add/remove, before the append-log RMW. It is a single null-branch on every non-test path.
Connection 1 runs VADD (the hook parks it in that window); connection 2 UNLINKs the key; then the hook is released so the synthetic RMW lands on the tombstoned key. The test asserts the key is not resurrected as a zeroed, recreate-flagged phantom index.

Verified: fails on the unfixed code (key resurrected) and passes with the fix, across both RespVectorSetTests fixtures. Full Garnet.test.vectorset suite green.

…bstoned key NeedInitialUpdate had no VADD/VREM case, so the synthetic CopyUpdater-only args (replication append-log, migration, recreate, set-flags) fell through to `default -> return true`. When a VADD's post-add ReplicateVectorSetAdd RMW raced in after a concurrent UNLINK (a raw main-store DELETE that takes no vector lock) tombstoned the key, InitialUpdater created a pre-sized 56-byte index record and its synthetic-arg branch left it fully zeroed (ctx=0 dims=0 indexPtr=0). NeedsRecreate (which only checks indexPtr==0) then flagged it, and RecreateIndex(dims:0) could never produce a valid pointer, so the recreate loop spun forever, pegging all cores on the primary. Add explicit VADD/VREM cases to NeedInitialUpdate that refuse to create a record for the CU-only synthetic args (the key must already exist, matching the RIPROMOTE/RIRESTORE precedent). A genuine VADD create carries arg1 == 0 and still creates normally. Adds a deterministic standalone regression test that drives the exact create -> UNLINK -> synthetic-replication-RMW interleaving. It fails on the unfixed code and passes with the fix, independent of AOF. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

Copilot

Pull request overview

Fixes a primary-side VectorSet recreate livelock where a synthetic replication RMW (used to append to AOF / replication stream) could accidentally create a zero-initialized index record after a raced delete, causing NeedsRecreate to remain true forever and spinning the recreate loop.

Changes:

Update MainSessionFunctions.NeedInitialUpdate to refuse initial-create for CU-only synthetic VectorSet RMW arguments (VADD/VREM append-log, migrate, recreate, set-flags).
Add a deterministic standalone regression test that reproduces the delete → synthetic-RMW interleaving and asserts the key is not resurrected as a phantom recreate-flagged record.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
test/standalone/Garnet.test.vectorset/VectorRecreateLivelockTests.cs	Adds a regression test covering the raced delete + synthetic replication RMW scenario to prevent resurrecting a zeroed index record.
libs/server/Storage/Functions/MainStore/RMWMethods.cs	Prevents CU-only synthetic VectorSet RMWs from creating new records during `NeedInitialUpdate`, avoiding phantom recreate-flagged index records.

tiagonapoli · 2026-07-15T18:44:32Z

+            // 2. UNLINK the key. This is a raw main-store tombstone that does NOT take the
+            //    vector lock, so in production it can land while a concurrent VADD still holds
+            //    only a shared vector lock and is about to replicate its add.
+            _ = db.KeyDelete(Key);
+            ClassicAssert.IsFalse(db.KeyExists(Key), "key should be gone after UNLINK");


Addressed in 326586e — the test issues a DEL via KeyDelete, which takes the same lock-free raw main-store delete path as UNLINK (both bypass the vector lock). Reworded the comments to describe it accurately rather than claiming UNLINK.

tiagonapoli · 2026-07-15T18:44:34Z

+                    // record. If the key was concurrently deleted (e.g. a raced UNLINK, which
+                    // takes no vector lock), creating a record here via InitialUpdater leaves a
+                    // zeroed 56-byte index that NeedsRecreate flags forever, livelocking the
+                    // recreate loop. Refuse to create — the key must already exist, as with
+                    // RIPROMOTE/RIRESTORE. A genuine VADD create carries arg1 == 0 and still creates.


Addressed in 326586e — dropped the hard-coded 56-byte literals from the comments and describe the index record generically (the size is defined by a constant).

The previous regression test drove the bug by calling the internal ReplicateVectorSetAdd directly. Replace it with a deterministic end-to-end race that exercises the real command paths: - Add a test-only hook (VectorManager.OnBeforeSyntheticReplicationRmw) fired inside the synthetic-replication window of ReplicateVectorSetAdd/Remove, after the vector add/remove but before the append-log RMW. It is a single null-branch on every non-test path. - The new test (in RespVectorSetTests) runs VADD on one connection, which the hook parks in that window; UNLINKs the key on a second connection; then releases the VADD so its synthetic RMW lands on the tombstoned key. It asserts the key is not resurrected as a zeroed, recreate-flagged phantom index. Verified: fails on the unfixed code (key resurrected) and passes with the NeedInitialUpdate fix, across both RespVectorSetTests fixtures. Removes the standalone VectorRecreateLivelockTests file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

Replace the bespoke `VectorManager.OnBeforeSyntheticReplicationRmw` static Action with the codebase's established fault-injection framework: - Add ExceptionInjectionType.VectorSet_Pause_Before_Synthetic_Replication_Rmw. - ReplicateVectorSetAdd/Remove now call ExceptionInjectionHelper.WaitOnClear at the synthetic-replication window (DEBUG-only), matching the existing pause points (e.g. RangeIndex_Migration_Receive_Pause_In_ProcessRecord). - The race test arms it with EnableException and clears it with DisableException, mirroring the InterruptedVectorSetDelete tests, and is skipped in non-DEBUG. Behaviour is unchanged: the test still fails on the unfixed code (tombstoned key resurrected) and passes with the NeedInitialUpdate fix, across both fixtures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

… spin) Switch both the VADD and VREM replication pause points to ExceptionInjectionHelper.ResetAndWaitAsync, which parks on a TaskCompletionSource and signals arrival by auto-clearing the injection flag, instead of the busy-spin WaitOnClear. Rewrite the race test to await WaitOnClearAsync for arrival detection rather than polling KeyExists with Thread.Yield, so there is no busy spin on either side of the handshake. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

tiagonapoli · 2026-07-15T04:43:12Z

+                    // zeroed 56-byte index that NeedsRecreate flags forever, livelocking the
+                    // recreate loop. Refuse to create — the key must already exist, as with
+                    // RIPROMOTE/RIRESTORE. A genuine VADD create carries arg1 == 0 and still creates.
+                    if (input.arg1 is VectorManager.VADDAppendLogArg


@kevin-montrose Safe to assume any NeedInitialUpdate with these flags are invalid?
Now that I'm thinking about it, we should actually fail right? Otherwise we might return OK for a VADD that actually didn't complete because of a UNLINK that happened concurrently

The VADD failed to replicate because of a racing delete, so when the delete replicates the vector set disappears. Dropping that write is fine IMO, there's no guarantee (due to replication delay) that a client would observe it.

…n coverage Second primary-side fix for the VectorSet recreate livelock. When a VREM synthetic append-log RMW takes the Tsavorite read-copy-update path (index record aged into the read-only region), CopyUpdater's `case VREM` returned without copying the old 56-byte index value forward, leaving the new record zeroed -> indexPtr==0 -> NeedsRecreate livelock. Copy the value forward, symmetric with the VADD append-log CopyUpdater. Also: - Parametrize the concurrent-UNLINK race regression test over both VADD and VREM (both hold only a shared vector lock while issuing their synthetic replicate RMW, so a raw UNLINK can tombstone the key in the window). - Add VremCopyUpdaterOnReadOnlyIndexMustPreserveIndexValue, a race-free repro driving the CopyUpdater path deterministically via a split-address ShiftReadOnlyAddress. - Replace the DEBUG-only test-hook scaffolding at the two synthetic-replication pause sites with a clean, event-driven ExceptionInjectionHelper.ResetAndWait one-liner (single localized VSTHRD002 suppression). - Gate the race test with #if DEBUG (dominant convention) instead of a runtime Assert.Ignore. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

- Reconcile the DEL/UNLINK narrative in the synthetic-replication race test: the test issues a DEL via KeyDelete, which takes the same lock-free raw main-store delete path as UNLINK (both bypass the vector lock), so the comments now describe it accurately rather than claiming UNLINK. - Drop hard-coded "56-byte" index-record sizes from test comments; describe the record generically since the size is defined by a constant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

Drop the Release/no-busy-spin detail; keep the concise description of what the synchronous pause helper does. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

…lication Documents that when a parked VADD's synthetic append-log RMW lands on a key that was concurrently deleted and re-added as a plain String, the append-log arg is a no-op in the RMW updaters — the String value and type are left intact, with no corruption or recreate livelock. This holds independently of the NeedInitialUpdate guard (the key exists on this path). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

The "concurrent delete, then a new vector set is created before the parked VADD's synthetic RMW runs" interleaving is impossible for the same key: the VADD holds a SHARED vector lock across its synthetic replication RMW, and creating a new index needs an EXCLUSIVE lock, so any new create blocks until the parked VADD releases. The test asserts the only realizable ordering (raced delete -> synthetic RMW on the tombstone -> then a fresh create) leaves a clean index holding only its own element, with no leakage of the raced-away element and no recreate livelock. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

Documents a known replica-side bug: when a primary VADD is raced by a lock-free DEL + String SET before its synthetic append-log RMW, the replica replays a genuine VADD against the now-String key. That trips Debug.Assert(status == GarnetStatus.OK) in ApplyVectorSetAdd (VectorManager.Replication.cs:414) because the "index must exist" invariant is false under a raced concurrent delete. Marked [Ignore] (the assert fires on the replay thread at teardown); orthogonal to the NeedInitialUpdate primary-side fix. Flagged for the Garnet team. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

Positive regression guard for the delete-during-synthetic-replication race in its "UNLINK then fresh vector set" form, observed on a replica. A parked VADD(T1) holds a shared vector lock across its synthetic replicate, so a concurrent fresh create needs an exclusive lock and cannot interleave before T1's replicate; the realizable order is VADD(T1 native) -> UNLINK(T2) -> VAddReplicate(T1, absent key) -> VADD-new-set(T2). Because the NeedInitialUpdate fix makes the synthetic RMW against the absent key a true no-op, it emits no AOF entry, so the replica never resurrects the first element E1. Test asserts primary and replica both converge to {E2}. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

…ctor index When a VADD/VREM emits its synthetic append-log RMW to replicate, the target key can be concurrently UNLINK'd and re-typed (e.g. a raced String SET). The synthetic RMW previously ran as a no-op that still returned Succeeded, emitting an AOF entry the replica replayed as a genuine VADD against a String key - tripping the `index must exist` invariant assert in ApplyVectorSetAdd (and risking the data-loss GarnetException in Release). Guard InPlaceUpdater and CopyUpdater for VADD/VREM: if the record's RecordType is no longer VectorManager.RecordType, set rmwInfo.Action = CancelOperation. Canceling writes nothing to the AOF, so replicas never replay the phantom add; primary and replica both converge to the String value. Un-ignore ReplicaReplaysSyntheticVAddAgainstStringKeyAfterRacedDeleteAsync, which now passes with the primary-side fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

tiagonapoli · 2026-07-15T22:38:31Z

+                    // These synthetic args are only valid as a CopyUpdater on an existing index
+                    // record; encountering them here means the key was concurrently UNLINK'd, so we
+                    // cannot complete the operation. A genuine VADD create carries arg1 == 0.


These are valid as CopyUpdater or InPlaceUpdater, update comment

Replace the chained input.arg1 == ... || comparisons with a single is/or constant pattern for readability. All *Arg members are const, so the pattern compiles and is behaviorally identical. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

Copilot AI review requested due to automatic review settings July 15, 2026 02:57

Copilot started reviewing on behalf of tiagonapoli July 15, 2026 02:57 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Tiago Napoli and others added 3 commits July 14, 2026 20:07

tiagonapoli commented Jul 15, 2026

View reviewed changes

Tiago Napoli and others added 9 commits July 14, 2026 21:45

Trim ResetAndWait doc comment

9519953

Drop the Release/no-busy-spin detail; keep the concise description of what the synchronous pause helper does. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

Drop inline VSTHRD002 justification comment

a7c05b7

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e3efc6-f376-44df-a8c8-9fdc2dd68369

tiagonapoli commented Jul 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix VectorSet recreate livelock from synthetic replication RMW on tombstoned key#1934

Fix VectorSet recreate livelock from synthetic replication RMW on tombstoned key#1934
tiagonapoli wants to merge 14 commits into
mainfrom
tiagonapoli/vectorset-recreate-livelock-fix

tiagonapoli commented Jul 15, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

tiagonapoli Jul 15, 2026

Uh oh!

tiagonapoli Jul 15, 2026

Uh oh!

tiagonapoli Jul 15, 2026

Uh oh!

kevin-montrose Jul 15, 2026

Uh oh!

tiagonapoli Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tiagonapoli commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root cause

Fix

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

tiagonapoli Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

tiagonapoli Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

tiagonapoli Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

kevin-montrose Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

tiagonapoli Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tiagonapoli commented Jul 15, 2026 •

edited

Loading