Fix root causes behind recurring Garnet .NET CI flakes

[Badrish Chandramouli (badrishc)]

Triaged the last 60 Garnet .NET CI runs on main (39 failed, 68 failed-job logs) and bucketed every failure by root cause.

CI runners have 2 cores, so each failure was reproduced locally under taskset -c 0-1 — that is why none of these reproduce on a high-core dev machine. Every change addresses the underlying defect rather than relaxing a timeout, adding a retry, or weakening an assertion.

---

Product fixes

Replication teardown disposes a client session that a worker is still using

This is the single largest source of CI flakiness: across the failed-job logs it accounts for the great majority of cluster failures, and each occurrence takes down up to 26 consecutive tests in the same job.

GarnetClientSession documents that it "expects mono-threaded client access, i.e., no concurrent invocations of API by client". AofSyncDriver.Dispose() violated this: it disposed each AOF sync task's session before waiting for that task's worker to quiesce.

When the disposing thread wins the race, the worker has just published its send frame (SendResponse nulls responseObject before sending) and has not yet reacquired one, so the disposer's ReturnResponseObject() returns nothing. The worker then calls GetResponseObject(): LightConcurrentStack.TryPop samples disposed == false on an empty stack, releases the latch, and the caller allocates a fresh GarnetSaeaBuffer renting from the replication pool. The disposer disposes the stack in that gap, the worker dies at its next Throttle() check without returning the frame, and its own Dispose() returns immediately because the disposed counter is already past 1. The buffer is stranded.

LimitedFixedBufferPool.Dispose() then spin-waits forever for totalReferences to reach zero, so ReplicationManager.Dispose() never returns and the node never releases its port. Every later test in the fixture fails on Failed to connect within 30 seconds. The CI logs show precisely this, in order:

LimitedFixedBufferPool.Dispose blocked with 1 unreturned references (poolOwner=Replication)
  Unreturned buffer: ownerType=Replication, bufferType=SaeaSendBuffer, size=67108864
^Timed out waiting for DisposeCluster^
... then 26 x "Waiting for Port 7702 to become available" / "Failed to connect within 30 seconds"

67108864 is 2 << AofPageSizeBits at the default 32 MB AOF page size, i.e. aofSyncSendBufferSize — the stranded frame is an AOF sync buffer, which is why these failures concentrate in the sharded-log fixture.

The fix is to correct the ordering, not to make the sender thread-safe (that would paper over the contract violation and put synchronization on the send path):

• AofSyncTask.RunAofSyncTaskAsync disposes its client before leaving the active worker monitor, so a drained monitor implies every client was torn down by the one thread that used it and its buffer is back in the pool.
• AofSyncDriver.Dispose() breaks the connections, waits for the workers, and only then disposes the tasks. The task dispose is kept so a client that was connected without a running worker still returns its buffer exactly as before, and moving it after the wait also makes the AOF iterator disposal race-free.
• GarnetClientSession.CloseConnection() closes only the socket — enough to fail a pending send and unblock a parked worker, without touching the buffer that worker owns.

Confined to teardown; no hot-path changes.

CloseConnection() is what makes the wait terminate, so it must not be possible to lose it. It could be: it reads the socket field, and a worker inside ConnectAsync has not published that field yet. The window is not narrow — the field is written in the continuation of socket = await ConnectSendSocketAsync(...), which on a loaded 2-core runner can be delayed arbitrarily. ConnectSendSocketAsync honours the token, but nothing after it does on the non-TLS path: NetworkHandler.StartAsync returns immediately when TLS is off, and the AUTH / CLIENT SETINFO / CLIENT SETNAME exchanges that follow await a reply with no cancellation token. A worker that lost the break rents its send buffer and then parks forever on a peer that is itself shutting down — the same hang, one step earlier.

So it is a handshake: CloseConnection() records the request before closing, and the connecting thread rechecks it immediately after publishing the socket. Either the closer observes the socket or the connector observes the request. The recheck sits before the network handler is constructed and before the send buffer is rented, so the throw path allocates nothing. Both sides carry a StoreLoad fence — publishing the socket and reading the request are a store then a load of a different location, which an acquire load does not order, so a fence on only one side would still lose the break.

Ablation, modelling the acquire-then-abandon sequence directly against GarnetTcpNetworkSender: with a foreign thread disposing the sender while the worker holds a rented frame, LimitedFixedBufferPool.Dispose() never completes; with the owning thread returning its own frame it drains immediately.

Validated on 2 cores (taskset -c 0-1): Garnet.test.cluster.multilog 105 passed / 12 skipped / 0 failed, with no DisposeCluster timeout, no buffer-pool dispose diagnostic and no leaked epochs.

Slot migration hands a Vector Set a context that is already in use locally

Vector Set contexts are allocated independently per node and index creation is not AOF-logged. On slot migration the destination reserved a context from its own free list and stamped it into the AOF migrate records. Replay adopted that context verbatim, guarding only on IsMigrating and never on IsInUse. ContextMetadata.MarkInUse only asserted — compiled out in Release, which is the configuration the failing CI job runs — so two Vector Sets silently shared one namespace and destroyed each other's data.

Migrated records are now remapped onto a context that is free locally, and the mapping is held for the duration of the migration. Element records are provably transmitted, applied, and acknowledged before the index key is sent (the Task.WhenAll barrier in MigrateSessionSlots.CreateAndRunMigrateTasksAsync), so the mapping can be released when the index key arrives.

A/B over 10 iterations: 12 context collisions, 748 records destroyed, 5 exceptions before — none after.

A checkpoint between an index record and its context metadata leaks the context

A Vector Set's index record is written before the context metadata that reserves its context, and that metadata is only flushed once index creation succeeds. A checkpoint taken between the two captures a live index record whose context is not marked in use, so the free list hands the same context to the next Vector Set created — the same corruption class as the migration defect above.

ReconcileRecoveredState now restores the reservation for every recovered index whose context is free. The hash slot the reservation needs is computed in RecoveredVectorSetIndexKey, which widens recoveredIndexes from byte to ushort to carry it.

VectorSetRecoveredContextReservationTests covers this: with the fix the new Vector Set gets its own context; without it recovery reports the context as free and the next VADD is handed the identical context, failing the test.

The migration remap outlived rebuilds of the array it is keyed on

The remap above is in-memory state keyed on contextMetadatas, and two paths rebuild that array underneath it: FLUSHDB/FLUSHALL through FlushGuard.Dispose, and recovery through ReconcileRecoveredState. Neither cleared the remap, so a cached entry could steer the remaining records of a migration into a context that had since been freed and handed to another Vector Set.

Both paths now clear it. Recovery already treats a context still marked migrating as a failed migration and marks it for cleanup, so forcing the retried migration to re-resolve is the intended behaviour rather than merely the safe one.

---

Test fixes

ClientKillTestAsync asserts an asynchronous disconnect synchronously

The test issued one fire-and-forget Ping and immediately asserted IsConnected was false. GarnetClient.IsConnected reads Socket.Connected, which .NET defines as the state as of the last completed I/O, and TCP guarantees the first send after a remote FIN still succeeds into the kernel buffer — only the next operation observes the reset. The assertion could therefore only pass by winning a race against the client's receive loop asynchronously marking the session disposed, which it loses on a loaded 2-core runner.

A standalone socket experiment confirms the mechanism deterministically: 200 ms after the peer closes, Connected is still True; the first send succeeds; the second throws and only then does Connected go false.

It now pings until the disconnect surfaces, bounded at 10 s. The test keeps its full power: if CLIENT KILL genuinely failed to kill the connection the pings keep succeeding, IsConnected stays true, and the assertion still fires.

A stranded injection point hangs the entire test process

A thread parked in ExceptionInjectionHelper.ResetAndWaitAsync is only released by EnableException, but the cleanup a test runs on its way out is DisableException. Any test that leaves between a waiter arriving and being re-enabled — an assertion failing, or a wait for the arrival timing out on a slow 2-core runner — strands that waiter permanently.

The waiter is a server thread holding a pooled network buffer, so LimitedFixedBufferPool.Dispose spins forever on a reference that is never returned and the entire test process hangs: the job produces no results at all rather than one failing test. That is why these runs appear in CI as a bare timeout with no reportable failure.

The cluster call sites already bound this with WaitAsync(timeout, token); the four Vector Set call sites use the unbounded synchronous ResetAndWait.

GarnetServer now suspends parking for the duration of InternalDispose, which releases anyone already parked and stops anyone new from parking. Covering arrivals matters because disposal closes listeners before it drains handlers, so a request already in flight can reach a still-armed injection point after shutdown has begun and strand itself there — releasing only the waiters already parked leaves the identical hang one moment later.

The suspension is a count rather than a flag, so concurrent shutdowns are independent, and it is unwound in a finally so it cannot leak into a later test sharing this process-wide state (a leaked suspension would silently stop every subsequent injection point from pausing anything). No timeout is introduced. Both the helper methods and their call site are [Conditional("DEBUG")] and compile away in Release.

ExceptionInjectionShutdownTests covers all three properties, and each fails when the corresponding behaviour is removed: disposal completes while a waiter is parked, a caller arriving after shutdown began does not park, and parking still pauses normally once the shutdown that suspended it has finished.

StackExchange.Redis recycles a result box while a pulse is still outstanding

SE.Redis 2.12.8 pools SimpleResultBox in a [ThreadStatic] field and sets the exception outside the lock that ActivateContinuations pulses. When a server error faults the box before the caller reaches Monitor.Wait, the caller skips the wait and recycles the box while the reader thread still has a PulseAll outstanding. The next synchronous call on that thread consumes the stale pulse and returns with neither result nor exception, shifting every subsequent reply on that thread by one — a permanent lag that survives new multiplexers and new servers, because the box is thread-static and NUnit runs a fixture on one thread.

The CI logs show this exactly: LuaScriptTests.Issue1079 loses its exception and Issue1235 later receives Issue1079's error; ScriptLoadErrors pairs with Struct the same way, with counts matching 1:1. RespVectorSetTests.VADDErrors fails identically.

Adds TestUtils.ThrowsRedisException, which runs the failing call on a dedicated thread so the poisoned box never lands on the shared test thread, applied to the two fixtures CI proves are affected. Garnet's wire output was confirmed byte-exact with a command/reply balance counter, so this corrects the client-side defect without hiding any server behaviour.

VectorSetOverwriteTests.SETAsync compacts the whole log twice

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the compactionMaxSegments: 1 used by this fixture the computed compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE compacts the entire log. SETAsync was the only test invoking the ~120 MB helper twice against one server; splitting the KEEPTTL case into its own test takes it from 13 s to 2 s.

One stalled node in DisposeCluster failed the next four tests

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling left the rest of the cluster alive holding its ports; the next four tests then failed with Failed to connect within 30 seconds even though the test that actually broke had passed.

Every node's dispose is now started before any of them is waited on, and each is then waited on under a bounded share of one budget. Starting them inside the wait loop would leave a single node that exhausts the budget with every later node still undisposed — the same abandoned port by another route. Each dispose gets a dedicated thread (TaskCreationOptions.LongRunning) rather than a pool thread, because on a two-core runner the pool injects threads slowly enough that a queued dispose could otherwise sit unstarted behind the ones already blocked.

Task.Wait(TimeSpan) rethrows a faulted dispose as an AggregateException rather than returning, so it is caught per node; without that the throw escaped mid-loop and abandoned every remaining node — the exact cascade this loop exists to prevent.

A standalone probe confirmed it: 1 of 3 healthy nodes disposed before, 3 of 3 after.

The wait is bounded only by the shared budget, with no per-node cap, so a merely slow node cannot fail a teardown that the pre-existing outer guard would have allowed.

Missing gossip-convergence waits

CLUSTER MEET returns before gossip propagates, and MIGRATE/SETSLOT are issued against the source node, so the source must know the target or it replies ERR Unknown endpoint. Adds the missing convergence waits in ClusterMigrateTests, including one that waited in the wrong direction and one that had no wait at all.

Slot re-assignment likewise settles asynchronously after CLUSTER FAILOVER FORCE, so ClusterDivergentReplicasTest now waits for the new primary to observe its own slot ownership before writing to it.

---

Verification

All runs pinned to 2 cores with taskset -c 0-1, Debug, net8.0:

Suite	Result
Garnet.test.vectorset	400 / 400
Garnet.test.scripting	619 passed, 30 skipped, 0 failed
Garnet.test.cluster	156 / 156
Garnet.test.cluster.replication	107 / 107
Garnet.test.cluster.vectorsets	84 / 84
Garnet.test.cluster.replication.vectorsets	7 / 7
Garnet.test.cluster.migrate	56 / 56

Build is clean with 0 warnings, and dotnet format reports no changes for both Garnet.slnx and Tsavorite.slnx.

---

Superseded upstream during rebase

Fixes from the original version of this branch landed independently on main and were dropped in favour of the upstream versions:

• The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and the drain that has to precede it, are both in Fixes for Vector Set RENAMEs in AOF replay #2080. That version is a superset: it also reads the index back and fails loudly rather than silently applying flags to a Vector Set that is not there.
• UpgradeReplicasAsync racing a promoted primary that is still recovering is fixed on main with WaitForFailoverCompleted, which polls the failover state machine directly instead of inferring readiness from slot coverage. The WaitForAllSlotsServedAsync helper written for that call site is dropped with it rather than left unused.

---

Deliberately unchanged

AofSyncDriverStore holds _lock.WriteLock() across syncDriver.Dispose() in TryRemove, TryAddReplicationDriver and TryAddReplicationDrivers, while AofSyncTask.Throttle() takes _lock.ReadLock() via PublishShippedAddress — whose own doc comment says it "must not be called while holding the store lock". That is a lock inversion, but it is strictly pre-existing (the monitor wait was already inside Dispose() before this PR, and the trigger is decided before Dispose() is entered, so the reordering here does not make it more likely). It is also unreachable in these suites: it needs backpressure.Enabled, and AofSyncMaxLagBytes defaults to -1 with nothing in the cluster tests setting it. Filed rather than fixed here — the fix is to hoist the dispose out of the write-locked region, which AofSyncDriverStore.Dispose() already models.

ClusterSRAddReplicaAfterPrimaryCheckpoint and RespMemoryWriterOverflowTests.SortedSetAsync did not reproduce (3/3 and 6/6 locally), so no speculative change was made rather than risk masking a cause that has not been identified.

Vector Set cleanup reclaims a whole context but decides a context is abandoned from the single key QueueCleanups happened to observe during compaction, so a context that lives on under a different key — after RENAME, or after being handed to the next Vector Set created — can be marked while live. Fixing that requires an allocation generation on the context so a stale request can be recognised, which is a design change to context ownership rather than a CI issue, and should be handled separately. Compensating guards were prototyped here and removed: they cannot cover the window in which a context is reserved before its index record is written, and detecting the damage after the fact only converts silent data loss into a different symptom.

[Copilot]

🟡 Changes recommended

Recovery can index beyond rebuilt metadata, waiter release has a lost-release race, and late node disposals may remain unstarted.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes recurring low-core CI flakes across Vector Set recovery/migration, exception injection, and cluster teardown.

Changes:

• Reconciles and remaps Vector Set contexts.
• Hardens shutdown and cluster-test cleanup.
• Isolates affected StackExchange.Redis assertions and reduces expensive tests.

File summaries

File	Description
TestUtils.cs	Adds dedicated-thread Redis exception assertions.
VectorSetRecoveredContextReservationTests.cs	Tests context reservation after recovery.
VectorSetOverwriteTests.cs	Splits costly SET scenarios.
RespVectorSetTests.cs	Uses isolated exception assertions.
ExceptionInjectionShutdownTests.cs	Tests disposal with parked injection waiters.
LuaScriptTests.cs	Uses isolated exception assertions.
ClusterTestContext.cs	Adds bounded per-node disposal handling.
ClusterReplicationBaseTests.cs	Waits for slot ownership convergence.
ClusterMigrateTests.cs	Waits for bidirectional gossip convergence.
VectorManager.Replication.cs	Remaps migrated contexts locally.
VectorManager.cs	Restores recovered context reservations.
VectorManager.ContextMetadata.cs	Clears remaps during flush.
GarnetServer.cs	Releases parked waiters during shutdown.
ExceptionInjectionHelper.cs	Adds epoch-based waiter release.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 3
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Badrish Chandramouli (badrishc)]

Worked through all three from first principles. Two were legitimate and are fixed; one is a pre-existing property of main that this PR neither introduces nor widens.

---

1. ExceptionInjectionHelper lost-release race — legitimate, fixed

The diagnosis is right, and tracing it further showed the one-shot release had a second hole with the same fatal outcome. InternalDispose closes listeners before it drains handlers, so a request already in flight can reach a still-armed injection point after the release and park there. Releasing only the waiters that were already parked just moves the hang a moment later.

Both windows have the same shape — a waiter whose release edge has already passed — so rather than move the epoch sample, parking is now suspended for the duration of InternalDispose: it releases anyone already parked and stops anyone new from parking. That closes the read-then-sample window as a consequence, since the suspension is not a one-shot edge that can be missed.

• Count, not a flag, so concurrent shutdowns are independent (DisposeCluster now starts them all at once — see #3).
• Unwound in a finally, so it can't leak into a later test sharing this process-global state. A leaked suspension would silently stop every subsequent injection point from pausing anything, which is why that has its own test.
• No timeout introduced. Both methods and the call site stay [Conditional("DEBUG")].

Ablation — restoring the one-shot semantics and re-running:

Failed ArrivingAtAnInjectionPointDuringShutdownDoesNotPark [30 s]
  a caller reaching an injection point after shutdown began parked instead of
  proceeding, so it would still be holding its pooled buffer when the drain runs
Failed! - Failed: 1, Passed: 2, Total: 3, Duration: 31 s

With the fix: Passed! - Failed: 0, Passed: 3, Total: 3, Duration: 1 s.

---

2. Recovery indexing beyond rebuilt metadata — pre-existing on main, identical exposure

contextIndex comes from ContextMetadata.DecomposeContext(context) inside foreach (var (context, hashSlot) in recoveredIndexes). main already indexes contextMetadatas with that same value, in that same loop iteration:

// origin/main, VectorManager.cs:332-336
foreach (var (context, _) in recoveredIndexes)
{
    var (contextIndex, contextValue) = ContextMetadata.DecomposeContext(context);

    if (contextMetadatas[contextIndex].IsCleaningUp(contextIndex != 0, contextValue))

For a given context, the new block and the pre-existing IsCleaningUp block either both run or neither does, on the identical index. So if this scenario is reachable, main throws IndexOutOfRangeException at line 336 today; the new block only changes which line reports it. The set of indices touched is unchanged.

Growing the array here would therefore not fix the reachable case — main's line would still be there — and it would re-introduce code I removed earlier in this PR after proving it never fires. ContextStep = 8 and DecomposeContext divides by 64 * ContextStep, so contextIndex == 1 requires context ≥ 512; the guard did not fire once across 487 tests, and the IndexOutOfRangeException that originally motivated it was traced to a store scan that has since been deleted. Adding it back would be wallpaper over a condition that is either unreachable or a main bug, and either way not caused by this change.

Worth a separate issue if it can be shown reachable; deliberately out of scope here.

---

3. Late node disposals may remain unstarted — legitimate, fixed

Correct: Task.Run was called inside the wait loop, so a node that consumed the 45 s budget left every later node with its dispose merely queued and unwaited.

All disposes are now started before any of them is waited on, and the budget is then spent waiting on them collectively. Following the thread-pool half of the point: each dispose gets a dedicated thread (TaskCreationOptions.LongRunning) rather than a pool thread, since on a two-core runner the pool injects threads slowly enough that a queued dispose could sit unstarted behind the ones already blocked — the same abandoned port by another route. Dispose closes listeners early, so every node now frees its port even when the budget is blown.

---

Verification

Rebased onto d20d639 first. All suites re-run on 2 cores (taskset -c 0-1), Debug, net8.0 — the injection helper is process-global and DisposeCluster is used by every cluster suite, so none of these were assumed:

Suite	Result
Garnet.test.vectorset	400 / 400
Garnet.test.scripting	619 passed, 30 skipped
Garnet.test.cluster	156 / 156
Garnet.test.cluster.replication	107 / 107
Garnet.test.cluster.vectorsets	84 / 84
Garnet.test.cluster.replication.vectorsets	7 / 7
Garnet.test.cluster.migrate	56 / 56

Build clean with 0 warnings; dotnet format reports no changes.