Fix VectorSet recreate livelock from synthetic replication RMW on tombstoned key#1934
Open
tiagonapoli wants to merge 10 commits into
Open
Fix VectorSet recreate livelock from synthetic replication RMW on tombstoned key#1934tiagonapoli wants to merge 10 commits into
tiagonapoli wants to merge 10 commits into
Conversation
…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
Contributor
There was a problem hiding this comment.
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.NeedInitialUpdateto 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. |
Comment on lines
+83
to
+87
| // 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"); |
Collaborator
Author
There was a problem hiding this comment.
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.
Comment on lines
+54
to
+58
| // 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. |
Collaborator
Author
There was a problem hiding this comment.
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
commented
Jul 15, 2026
| // 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 |
Collaborator
Author
There was a problem hiding this comment.
@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
Contributor
There was a problem hiding this comment.
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
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
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=0with no precedingINITIAL-CREATE— i.e. a fully-zeroed 56-byte index record that reads back asOK, soNeedsRecreate(which only checksindexPtr == 0) returns true forever andRecreateIndex(dims: 0)can never produce a valid pointer.Root cause
A
VADDis phrased as a read once the index exists, so after a successful addVectorManager.ReplicateVectorSetAddissues a syntheticVADDAppendLogArgRMW 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.NeedInitialUpdatehad noVADD/VREMcase, so these CU-only synthetic args (VADDAppendLogArg,VREMAppendLogArg,RecreateIndexArg,VADDSetFlagsArg,Migrate*) fell through todefault -> return true.VADDholds only a shared vector lock across add + replicate, andUNLINK's raw main-storeDELETEtakes no vector lock, so a concurrentUNLINKcan tombstone the key in between. The synthetic RMW then lands on an absent key:NeedInitialUpdatereturnstrue,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/VREMcases toNeedInitialUpdatethat refuse to create a record for the CU-only synthetic args (the key must already exist, matching the existingRIPROMOTE/RIRESTOREprecedent). A genuineVADDcreate carriesarg1 == 0and 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:VectorManager.OnBeforeSyntheticReplicationRmw) fires inside the synthetic-replication window ofReplicateVectorSetAdd/Remove— after the vector add/remove, before the append-log RMW. It is a single null-branch on every non-test path.VADD(the hook parks it in that window); connection 2UNLINKs 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
RespVectorSetTestsfixtures. FullGarnet.test.vectorsetsuite green.