Skip to content

Fixes for Vector Set RENAMEs in AOF replay - #2080

Merged
kevin-montrose merged 14 commits into
mainfrom
users/kmontrose/vectorSetRenameAOFFixes
Aug 20, 2026
Merged

kevin-montrose merged 14 commits into
mainfrom
users/kmontrose/vectorSetRenameAOFFixes

Conversation

@kevin-montrose

Copy link
Copy Markdown
Contributor

A number of issues exist if Vector Sets are RENAME'd during AOF replay.

Fundamentally RENAME is a little weird as it is not a single operation against the Tsavorite log, but rather a bundle of gets, deletes, and sets covered by a transaction. Related fix #2071 .

This PR fixes these by:

  • Correctly handling the SetFlags version of VADD
  • Waiting for concurrent VADDs and various cleanup tasks to complete before and after AOF recovery starts
  • Special casing RENAME's associated UnifiedStoreStringUpsert op in the AOF
    • This is necessary because the context and indexPtr actually stored in the value are not necessarily correct during replay and need to be refreshed with current values

Copilot AI balanced review requested due to automatic review settings August 19, 2026 14:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes Vector Set rename handling during AOF recovery and coordinates background vector operations around replay.

Changes:

  • Records rename metadata and reconstructs Vector Set indexes during replay.
  • Adds recovery quiescence waits and Vector Set reconciliation.
  • Adds AOF rename recovery tests.

Reviewed changes

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

Show a summary per file
File Description
test/standalone/Garnet.test.vectorset/VectorSetOverwriteTests.cs Uses the expanded quiescence wait.
test/standalone/Garnet.test.vectorset/RespVectorSetTests.cs Tests rename recovery scenarios.
libs/storage/Tsavorite/cs/src/core/ClientSession/ManageClientSessions.cs Removes blank lines.
libs/server/StoreWrapper.cs Reorders vector recovery around AOF replay.
libs/server/Storage/Session/UnifiedStore/UnifiedStoreOps.cs Records rename keys and record type.
libs/server/Storage/Functions/UnifiedStore/VarLenInputMethods.cs Restores record type during replay.
libs/server/Storage/Functions/UnifiedStore/UpsertMethods.cs Separates rename metadata from expiration.
libs/server/Resp/Vector/VectorManager.Replication.cs Handles flag replay and rename copying.
libs/server/Resp/Vector/VectorManager.cs Formatting-only change.
libs/server/Resp/Vector/VectorManager.Cleanup.cs Expands background-work waiting.
libs/server/Databases/SingleDatabaseManager.cs Waits for vector replay and cleanup.
libs/server/Databases/MultiDatabaseManager.cs Adds per-database vector quiescence.
libs/server/Databases/IDatabaseManager.cs Updates recovery-order documentation.
libs/server/AOF/AofProcessor.cs Special-cases Vector Set rename upserts.
libs/cluster/Server/Replication/ReplicationManager.cs Reconciles vectors before cluster AOF replay.

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

Comment thread libs/server/Storage/Session/UnifiedStore/UnifiedStoreOps.cs
Comment thread libs/server/Resp/Vector/VectorManager.Cleanup.cs Outdated
Comment thread libs/server/AOF/AofProcessor.cs Outdated
Comment thread libs/server/AOF/AofProcessor.cs Outdated
@kevin-montrose
kevin-montrose merged commit 57c5215 into main Aug 20, 2026
227 checks passed
@kevin-montrose
kevin-montrose deleted the users/kmontrose/vectorSetRenameAOFFixes branch August 20, 2026 21:12
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Aug 30, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

ReconcileRecoveredState inferred that any context "in use but not recovered"
was garbage, but recoveredIndexes was populated only from checkpoint-snapshot
reads, so index records introduced by AOF replay were invisible and their
contexts were reclaimed while live. Recovery now scans the store for live index
records, so the census it reasons about covers every index regardless of how it
arrived. The scan collects into a fresh map instead of merging into the
snapshot-derived one, so an index the snapshot reported but AOF replay then
deleted no longer keeps its context reserved. Two follow-on defects are fixed
with it: contextMetadatas is grown to cover contexts named by index records in
blocks allocated after the last metadata write, previously an
IndexOutOfRangeException that crashed GarnetServer.Start; and a recovered index
whose context is not marked in use has that reservation restored, since the
index record is written before the metadata that reserves its context and a
recovery boundary between the two would otherwise hand the context out twice.

Test fixes
----------

StackExchange.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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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. Each node is now disposed behind its own
bounded wait and the loop continues past both stalls and exceptions, rethrowing
afterwards so the real fault is still reported. 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, which is the 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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                396/396
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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.

IterateLookupSnapshot's documented return value was wrong and is corrected:
ScanLookup returns true only when it stops early on a filled record budget, so
a scan that walks the whole range returns false. Reading it as "completed"
would silently disable cleanup entirely.

Superseded upstream during rebase
---------------------------------

Three fixes from the original version of this branch were landed independently
on main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

Co-authored-by: Copilot <[email protected]>
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Aug 30, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

ReconcileRecoveredState inferred that any context "in use but not recovered"
was garbage, but recoveredIndexes was populated only from checkpoint-snapshot
reads, so index records introduced by AOF replay were invisible and their
contexts were reclaimed while live. Recovery now scans the store for live index
records, so the census it reasons about covers every index regardless of how it
arrived. The scan collects into a fresh map instead of merging into the
snapshot-derived one, so an index the snapshot reported but AOF replay then
deleted no longer keeps its context reserved. Two follow-on defects are fixed
with it: contextMetadatas is grown to cover contexts named by index records in
blocks allocated after the last metadata write, previously an
IndexOutOfRangeException that crashed GarnetServer.Start; and a recovered index
whose context is not marked in use has that reservation restored, since the
index record is written before the metadata that reserves its context and a
recovery boundary between the two would otherwise hand the context out twice.

Test fixes
----------

StackExchange.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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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. Each node is now disposed behind its own
bounded wait and the loop continues past both stalls and exceptions, rethrowing
afterwards so the real fault is still reported. 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, which is the 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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                396/396
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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.

IterateLookupSnapshot's documented return value was wrong and is corrected:
ScanLookup returns true only when it stops early on a filled record budget, so
a scan that walks the whole range returns false, and the value cannot be used
to tell success from an early exit. The recovery scan added above therefore
discards it, and the corrected documentation explains why.

Superseded upstream during rebase
---------------------------------

Three fixes from the original version of this branch were landed independently
on main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

Co-authored-by: Copilot <[email protected]>
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Aug 30, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

ReconcileRecoveredState inferred that any context "in use but not recovered"
was garbage, but recoveredIndexes was populated only from checkpoint-snapshot
reads, so index records introduced by AOF replay were invisible and their
contexts were reclaimed while live. Recovery now scans the store for live index
records, so the census it reasons about covers every index regardless of how it
arrived. The scan collects into a fresh map instead of merging into the
snapshot-derived one, so an index the snapshot reported but AOF replay then
deleted no longer keeps its context reserved. Two follow-on defects are fixed
with it: contextMetadatas is grown to cover contexts named by index records in
blocks allocated after the last metadata write, previously an
IndexOutOfRangeException that crashed GarnetServer.Start; and a recovered index
whose context is not marked in use has that reservation restored, since the
index record is written before the metadata that reserves its context and a
recovery boundary between the two would otherwise hand the context out twice.

Test fixes
----------

StackExchange.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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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. Each node is now disposed behind its own
bounded wait and the loop continues past both stalls and exceptions, rethrowing
afterwards so the real fault is still reported. 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, which is the 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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                396/396
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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.

IterateLookupSnapshot's documented return value was wrong and is corrected:
ScanLookup returns true only when it stops early on a filled record budget, so
a scan that walks the whole range returns false, and the value cannot be used
to tell success from an early exit. The recovery scan added above therefore
discards it, and the corrected documentation explains why.

Superseded upstream during rebase
---------------------------------

Three fixes from the original version of this branch were landed independently
on main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

Co-authored-by: Copilot <[email protected]>
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 8, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

StackExchange.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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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. Each node is now disposed behind its own
bounded wait and the loop continues past both stalls and exceptions, rethrowing
afterwards so the real fault is still reported. 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, which is the 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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                397/397
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 8, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

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 - therefore 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, which is
why these runs show up 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 releases parked waiters as the first step of disposal, before the drain that
would otherwise block on them. The release is epoch-based rather than a timeout
so no arbitrary interval is introduced, and it is one-shot - waiters that park
after the call sample the new epoch and block normally - so it does not leak
across server instances sharing the process-global injection state. Both the
helper method and its call site are [Conditional("DEBUG")] and compile away in
Release. ExceptionInjectionShutdownTests covers this: without the fix
GarnetServer.Dispose() never returns and the test fails on its 30 s bound with a
message naming the cause; with it, disposal completes in under a second.

StackExchange.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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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. Each node is now disposed behind its own
bounded wait and the loop continues past both stalls and exceptions, rethrowing
afterwards so the real fault is still reported. 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, which is the 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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                398/398
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 8, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

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 - therefore 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, which is
why these runs show up 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 that were
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 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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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, which is the same
abandoned port by another route. Each dispose gets a dedicated thread 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, which is the
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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                400/400
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 9, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

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 - therefore 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, which is
why these runs show up 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 that were
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 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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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, which is the same
abandoned port by another route. Each dispose gets a dedicated thread 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, which is the
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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                400/400
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

ClusterVectorSetTests.VectorSetMigrateManyBySlot: poll on any failed read
-------------------------------------------------------------------------
This test failed three times across recent CI runs (windows net8.0 Debug and
windows net10.0 Debug) with "Expected: 3, But was: 1".

ClusterTestUtils.Execute catches every exception and returns the message as a
single-element reply, so a failed read is indistinguishable from data by type.
The retry loop that reads the migrated keys back from the new replica treated a
reply as transient only when its text began with "Key has MOVED to ", and hit a
hard ClassicAssert.AreEqual(3, length) for anything else - on the very first
poll, before the replica had converged.

Probing that window directly (reading the new replica immediately after
MigrateSlots) shows three distinct transient replies, only the first of which
the old guard recognised:

  "Key has MOVED to Endpoint ... but CommandFlags.NoRedirect was specified"
  "CLUSTERDOWN Hash slot not served"
  a nil reply, which casts to a null array and throws NullReferenceException

Replaying the original guard in that window fails 2/2 tests with exactly the CI
error; the new guard passes 5 runs of 2/2.

A successful VSIM ... WITHSCORES WITHATTRIBS hit is always three elements, so a
null or single-element reply always means the read did not land. The loop now
treats all of them as "poll again", captures the last response for the failure
message, and no longer asserts inside the loop - a caught NUnit assertion is
still recorded against the test result even when the next poll would resolve it,
which the single-slot variant of this test already documents.

The element/score/attribute assertions are unchanged; they now run once after
the loop, so a genuinely wrong reply still fails with the full NUnit diff, and a
persistent error still fails with the response text instead of a bare length
mismatch. The polling window matches the 15s the single-slot variant already
uses for the same convergence event, and the previously absent sleep between
polls stops the loop from spinning on the two cores it shares with the nodes it
is waiting for.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 9, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

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 - therefore 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, which is
why these runs show up 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 that were
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 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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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, which is the same
abandoned port by another route. Each dispose gets a dedicated thread 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, which is the
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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                400/400
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

ClusterVectorSetTests.VectorSetMigrateManyBySlot: poll on any failed read
-------------------------------------------------------------------------
This test failed three times across recent CI runs (windows net8.0 Debug and
windows net10.0 Debug) with "Expected: 3, But was: 1".

ClusterTestUtils.Execute catches every exception and returns the message as a
single-element reply, so a failed read is indistinguishable from data by type.
The retry loop that reads the migrated keys back from the new replica treated a
reply as transient only when its text began with "Key has MOVED to ", and hit a
hard ClassicAssert.AreEqual(3, length) for anything else - on the very first
poll, before the replica had converged.

Probing that window directly (reading the new replica immediately after
MigrateSlots) shows three distinct transient replies, only the first of which
the old guard recognised:

  "Key has MOVED to Endpoint ... but CommandFlags.NoRedirect was specified"
  "CLUSTERDOWN Hash slot not served"
  a nil reply, which casts to a null array and throws NullReferenceException

Replaying the original guard in that window fails 2/2 tests with exactly the CI
error; the new guard passes 5 runs of 2/2.

A successful VSIM ... WITHSCORES WITHATTRIBS hit is always three elements, so a
null or single-element reply always means the read did not land. The loop now
treats all of them as "poll again", captures the last response for the failure
message, and no longer asserts inside the loop - a caught NUnit assertion is
still recorded against the test result even when the next poll would resolve it,
which the single-slot variant of this test already documents.

The element/score/attribute assertions are unchanged; they now run once after
the loop, so a genuinely wrong reply still fails with the full NUnit diff, and a
persistent error still fails with the response text instead of a bare length
mismatch. The polling window matches the 15s the single-slot variant already
uses for the same convergence event, and the previously absent sleep between
polls stops the loop from spinning on the two cores it shares with the nodes it
is waiting for.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 9, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

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 - therefore 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, which is
why these runs show up 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 that were
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 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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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, which is the same
abandoned port by another route. Each dispose gets a dedicated thread 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, which is the
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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                400/400
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

ClusterVectorSetTests.VectorSetMigrateManyBySlot: poll on any failed read
-------------------------------------------------------------------------
This test failed three times across recent CI runs (windows net8.0 Debug and
windows net10.0 Debug) with "Expected: 3, But was: 1".

ClusterTestUtils.Execute catches every exception and returns the message as a
single-element reply, so a failed read is indistinguishable from data by type.
The retry loop that reads the migrated keys back from the new replica treated a
reply as transient only when its text began with "Key has MOVED to ", and hit a
hard ClassicAssert.AreEqual(3, length) for anything else - on the very first
poll, before the replica had converged.

Probing that window directly (reading the new replica immediately after
MigrateSlots) shows three distinct transient replies, only the first of which
the old guard recognised:

  "Key has MOVED to Endpoint ... but CommandFlags.NoRedirect was specified"
  "CLUSTERDOWN Hash slot not served"
  a nil reply, which casts to a null array and throws NullReferenceException

Replaying the original guard in that window fails 2/2 tests with exactly the CI
error; the new guard passes 5 runs of 2/2.

A successful VSIM ... WITHSCORES WITHATTRIBS hit is always three elements, so a
null or single-element reply always means the read did not land. The loop now
treats all of them as "poll again", captures the last response for the failure
message, and no longer asserts inside the loop - a caught NUnit assertion is
still recorded against the test result even when the next poll would resolve it,
which the single-slot variant of this test already documents.

The element/score/attribute assertions are unchanged; they now run once after
the loop, so a genuinely wrong reply still fails with the full NUnit diff, and a
persistent error still fails with the response text instead of a bare length
mismatch. The polling window matches the 15s the single-slot variant already
uses for the same convergence event, and the previously absent sleep between
polls stops the loop from spinning on the two cores it shares with the nodes it
is waiting for.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 10, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

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 - therefore 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, which is
why these runs show up 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 that were
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 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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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, which is the same
abandoned port by another route. Each dispose gets a dedicated thread 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, which is the
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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                400/400
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

ClusterVectorSetTests.VectorSetMigrateManyBySlot: poll on any failed read
-------------------------------------------------------------------------
This test failed three times across recent CI runs (windows net8.0 Debug and
windows net10.0 Debug) with "Expected: 3, But was: 1".

ClusterTestUtils.Execute catches every exception and returns the message as a
single-element reply, so a failed read is indistinguishable from data by type.
The retry loop that reads the migrated keys back from the new replica treated a
reply as transient only when its text began with "Key has MOVED to ", and hit a
hard ClassicAssert.AreEqual(3, length) for anything else - on the very first
poll, before the replica had converged.

Probing that window directly (reading the new replica immediately after
MigrateSlots) shows three distinct transient replies, only the first of which
the old guard recognised:

  "Key has MOVED to Endpoint ... but CommandFlags.NoRedirect was specified"
  "CLUSTERDOWN Hash slot not served"
  a nil reply, which casts to a null array and throws NullReferenceException

Replaying the original guard in that window fails 2/2 tests with exactly the CI
error; the new guard passes 5 runs of 2/2.

A successful VSIM ... WITHSCORES WITHATTRIBS hit is always three elements, so a
null or single-element reply always means the read did not land. The loop now
treats all of them as "poll again", captures the last response for the failure
message, and no longer asserts inside the loop - a caught NUnit assertion is
still recorded against the test result even when the next poll would resolve it,
which the single-slot variant of this test already documents.

The element/score/attribute assertions are unchanged; they now run once after
the loop, so a genuinely wrong reply still fails with the full NUnit diff, and a
persistent error still fails with the response text instead of a bare length
mismatch. The polling window matches the 15s the single-slot variant already
uses for the same convergence event, and the previously absent sleep between
polls stops the loop from spinning on the two cores it shares with the nodes it
is waiting for.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 11, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

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 - therefore 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, which is
why these runs show up 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 that were
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 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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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, which is the same
abandoned port by another route. Each dispose gets a dedicated thread 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, which is the
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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                400/400
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

ClusterVectorSetTests.VectorSetMigrateManyBySlot: poll on any failed read
-------------------------------------------------------------------------
This test failed three times across recent CI runs (windows net8.0 Debug and
windows net10.0 Debug) with "Expected: 3, But was: 1".

ClusterTestUtils.Execute catches every exception and returns the message as a
single-element reply, so a failed read is indistinguishable from data by type.
The retry loop that reads the migrated keys back from the new replica treated a
reply as transient only when its text began with "Key has MOVED to ", and hit a
hard ClassicAssert.AreEqual(3, length) for anything else - on the very first
poll, before the replica had converged.

Probing that window directly (reading the new replica immediately after
MigrateSlots) shows three distinct transient replies, only the first of which
the old guard recognised:

  "Key has MOVED to Endpoint ... but CommandFlags.NoRedirect was specified"
  "CLUSTERDOWN Hash slot not served"
  a nil reply, which casts to a null array and throws NullReferenceException

Replaying the original guard in that window fails 2/2 tests with exactly the CI
error; the new guard passes 5 runs of 2/2.

A successful VSIM ... WITHSCORES WITHATTRIBS hit is always three elements, so a
null or single-element reply always means the read did not land. The loop now
treats all of them as "poll again", captures the last response for the failure
message, and no longer asserts inside the loop - a caught NUnit assertion is
still recorded against the test result even when the next poll would resolve it,
which the single-slot variant of this test already documents.

The element/score/attribute assertions are unchanged; they now run once after
the loop, so a genuinely wrong reply still fails with the full NUnit diff, and a
persistent error still fails with the response text instead of a bare length
mismatch. The polling window matches the 15s the single-slot variant already
uses for the same convergence event, and the previously absent sleep between
polls stops the loop from spinning on the two cores it shares with the nodes it
is waiting for.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 12, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

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 - therefore 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, which is
why these runs show up 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 that were
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 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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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, which is the same
abandoned port by another route. Each dispose gets a dedicated thread 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, which is the
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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                400/400
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

ClusterVectorSetTests.VectorSetMigrateManyBySlot: poll on any failed read
-------------------------------------------------------------------------
This test failed three times across recent CI runs (windows net8.0 Debug and
windows net10.0 Debug) with "Expected: 3, But was: 1".

ClusterTestUtils.Execute catches every exception and returns the message as a
single-element reply, so a failed read is indistinguishable from data by type.
The retry loop that reads the migrated keys back from the new replica treated a
reply as transient only when its text began with "Key has MOVED to ", and hit a
hard ClassicAssert.AreEqual(3, length) for anything else - on the very first
poll, before the replica had converged.

Probing that window directly (reading the new replica immediately after
MigrateSlots) shows three distinct transient replies, only the first of which
the old guard recognised:

  "Key has MOVED to Endpoint ... but CommandFlags.NoRedirect was specified"
  "CLUSTERDOWN Hash slot not served"
  a nil reply, which casts to a null array and throws NullReferenceException

Replaying the original guard in that window fails 2/2 tests with exactly the CI
error; the new guard passes 5 runs of 2/2.

A successful VSIM ... WITHSCORES WITHATTRIBS hit is always three elements, so a
null or single-element reply always means the read did not land. The loop now
treats all of them as "poll again", captures the last response for the failure
message, and no longer asserts inside the loop - a caught NUnit assertion is
still recorded against the test result even when the next poll would resolve it,
which the single-slot variant of this test already documents.

The element/score/attribute assertions are unchanged; they now run once after
the loop, so a genuinely wrong reply still fails with the full NUnit diff, and a
persistent error still fails with the response text instead of a bare length
mismatch. The polling window matches the 15s the single-slot variant already
uses for the same convergence event, and the previously absent sleep between
polls stops the loop from spinning on the two cores it shares with the nodes it
is waiting for.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 16, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

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 - therefore 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, which is
why these runs show up 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 that were
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 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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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, which is the same
abandoned port by another route. Each dispose gets a dedicated thread 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, which is the
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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                400/400
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

ClusterVectorSetTests.VectorSetMigrateManyBySlot: poll on any failed read
-------------------------------------------------------------------------
This test failed three times across recent CI runs (windows net8.0 Debug and
windows net10.0 Debug) with "Expected: 3, But was: 1".

ClusterTestUtils.Execute catches every exception and returns the message as a
single-element reply, so a failed read is indistinguishable from data by type.
The retry loop that reads the migrated keys back from the new replica treated a
reply as transient only when its text began with "Key has MOVED to ", and hit a
hard ClassicAssert.AreEqual(3, length) for anything else - on the very first
poll, before the replica had converged.

Probing that window directly (reading the new replica immediately after
MigrateSlots) shows three distinct transient replies, only the first of which
the old guard recognised:

  "Key has MOVED to Endpoint ... but CommandFlags.NoRedirect was specified"
  "CLUSTERDOWN Hash slot not served"
  a nil reply, which casts to a null array and throws NullReferenceException

Replaying the original guard in that window fails 2/2 tests with exactly the CI
error; the new guard passes 5 runs of 2/2.

A successful VSIM ... WITHSCORES WITHATTRIBS hit is always three elements, so a
null or single-element reply always means the read did not land. The loop now
treats all of them as "poll again", captures the last response for the failure
message, and no longer asserts inside the loop - a caught NUnit assertion is
still recorded against the test result even when the next poll would resolve it,
which the single-slot variant of this test already documents.

The element/score/attribute assertions are unchanged; they now run once after
the loop, so a genuinely wrong reply still fails with the full NUnit diff, and a
persistent error still fails with the response text instead of a bare length
mismatch. The polling window matches the 15s the single-slot variant already
uses for the same convergence event, and the previously absent sleep between
polls stops the loop from spinning on the two cores it shares with the nodes it
is waiting for.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 17, 2026
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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

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 - therefore 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, which is
why these runs show up 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 that were
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 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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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, which is the same
abandoned port by another route. Each dispose gets a dedicated thread 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, which is the
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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                400/400
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

ClusterVectorSetTests.VectorSetMigrateManyBySlot: poll on any failed read
-------------------------------------------------------------------------
This test failed three times across recent CI runs (windows net8.0 Debug and
windows net10.0 Debug) with "Expected: 3, But was: 1".

ClusterTestUtils.Execute catches every exception and returns the message as a
single-element reply, so a failed read is indistinguishable from data by type.
The retry loop that reads the migrated keys back from the new replica treated a
reply as transient only when its text began with "Key has MOVED to ", and hit a
hard ClassicAssert.AreEqual(3, length) for anything else - on the very first
poll, before the replica had converged.

Probing that window directly (reading the new replica immediately after
MigrateSlots) shows three distinct transient replies, only the first of which
the old guard recognised:

  "Key has MOVED to Endpoint ... but CommandFlags.NoRedirect was specified"
  "CLUSTERDOWN Hash slot not served"
  a nil reply, which casts to a null array and throws NullReferenceException

Replaying the original guard in that window fails 2/2 tests with exactly the CI
error; the new guard passes 5 runs of 2/2.

A successful VSIM ... WITHSCORES WITHATTRIBS hit is always three elements, so a
null or single-element reply always means the read did not land. The loop now
treats all of them as "poll again", captures the last response for the failure
message, and no longer asserts inside the loop - a caught NUnit assertion is
still recorded against the test result even when the next poll would resolve it,
which the single-slot variant of this test already documents.

The element/score/attribute assertions are unchanged; they now run once after
the loop, so a genuinely wrong reply still fails with the full NUnit diff, and a
persistent error still fails with the response text instead of a bare length
mismatch. The polling window matches the 15s the single-slot variant already
uses for the same convergence event, and the previously absent sleep between
polls stops the loop from spinning on the two cores it shares with the nodes it
is waiting for.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Badrish Chandramouli (badrishc) added a commit that referenced this pull request Sep 17, 2026
* Fix root causes behind recurring Garnet .NET CI flakes

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 these
do not reproduce on high-core dev machines. Every change below addresses the
underlying defect rather than relaxing a timeout, adding a retry, or weakening
an assertion.

Product fixes
-------------

Vector Set contexts are allocated independently per node and index creation is
not AOF-logged, so 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, which is compiled out in Release - 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 (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 and 5 exceptions before, none after.

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 and the two silently share a namespace - 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, and without it recovery reports the context as
free and the next VADD is handed the identical context, failing the test.

The migration 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 only the safe one.

Test fixes
----------

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 - therefore 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, which is
why these runs show up 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 that were
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 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, and
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, and applies
it 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.

DatabaseManagerBase.TakeCheckpointAsync runs compaction inline, and with the
compactionMaxSegments: 1 used by VectorSetOverwriteTests the computed
compactLength is zero, so untilAddress equals readOnlyAddress and every SAVE
compacts the entire log. SETAsync was the only test in the fixture 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. The 3-minute AsyncTimeout added previously
is left in place as defence in depth.

DisposeCluster disposed nodes in a bare loop, so one node throwing or stalling
left the rest of the cluster alive and 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, which is the same
abandoned port by another route. Each dispose gets a dedicated thread 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, which is the
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.

CLUSTER MEET returns before gossip propagates, and MIGRATE and 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:

  Garnet.test.vectorset                400/400
  Garnet.test.scripting                619 passed, 30 skipped, 0 failed
  Garnet.test.cluster.vectorsets       84/84
  Garnet.test.cluster.repl.vectorsets  7/7
  Garnet.test.cluster                  156/156
  Garnet.test.cluster.replication      107/107
  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 were landed independently on
main and have been dropped in favour of the upstream versions:

  - The missing VADDSetFlagsArg branch in HandleVectorSetAddReplication, and
    the drain that has to precede it, are both in #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 in 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
----------------------

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 is being 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.

ClusterVectorSetTests.VectorSetMigrateManyBySlot: poll on any failed read
-------------------------------------------------------------------------
This test failed three times across recent CI runs (windows net8.0 Debug and
windows net10.0 Debug) with "Expected: 3, But was: 1".

ClusterTestUtils.Execute catches every exception and returns the message as a
single-element reply, so a failed read is indistinguishable from data by type.
The retry loop that reads the migrated keys back from the new replica treated a
reply as transient only when its text began with "Key has MOVED to ", and hit a
hard ClassicAssert.AreEqual(3, length) for anything else - on the very first
poll, before the replica had converged.

Probing that window directly (reading the new replica immediately after
MigrateSlots) shows three distinct transient replies, only the first of which
the old guard recognised:

  "Key has MOVED to Endpoint ... but CommandFlags.NoRedirect was specified"
  "CLUSTERDOWN Hash slot not served"
  a nil reply, which casts to a null array and throws NullReferenceException

Replaying the original guard in that window fails 2/2 tests with exactly the CI
error; the new guard passes 5 runs of 2/2.

A successful VSIM ... WITHSCORES WITHATTRIBS hit is always three elements, so a
null or single-element reply always means the read did not land. The loop now
treats all of them as "poll again", captures the last response for the failure
message, and no longer asserts inside the loop - a caught NUnit assertion is
still recorded against the test result even when the next poll would resolve it,
which the single-slot variant of this test already documents.

The element/score/attribute assertions are unchanged; they now run once after
the loop, so a genuinely wrong reply still fails with the full NUnit diff, and a
persistent error still fails with the response text instead of a bare length
mismatch. The polling window matches the 15s the single-slot variant already
uses for the same convergence event, and the previously absent sleep between
polls stops the loop from spinning on the two cores it shares with the nodes it
is waiting for.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Fix replication teardown races that hang server dispose and poison CI fixtures

AofSyncDriver.Dispose() disposed each AOF sync task's GarnetClientSession while
its worker task was still running. GarnetClientSession documents that it
"expects mono-threaded client access, i.e., no concurrent invocations of API by
client", so this is a contract violation, and it is the dominant source of
flakiness in the cluster suites.

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. The disposer's ReturnResponseObject() therefore returns nothing,
and 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 second Dispose() returns immediately
on the disposed counter. The buffer is stranded.

LimitedFixedBufferPool.Dispose() then spin-waits forever for totalReferences to
reach zero, so ReplicationManager.Dispose() never returns, the node never
releases its port, and every later test in the fixture fails on
"Failed to connect within 30 seconds". CI shows exactly this: one unreturned
64 MB Replication SaeaSendBuffer (2 << AofPageSizeBits, i.e. an AOF sync buffer)
logged immediately before "Timed out waiting for DisposeCluster", followed by 26
consecutive failures in the same job.

Fix the ordering rather than making the sender thread-safe, which would paper
over the contract violation and add synchronization to 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 instead of disposing the
  sessions, waits for the workers to quiesce, and only then disposes the tasks.
  The task dispose is kept so a client connected without a running worker still
  returns its buffer, and it also makes the iterator disposal race-free.
- GarnetClientSession.CloseConnection() closes only the socket, which is what
  unblocks a worker parked on a send without touching the buffer it owns.

This is confined to teardown; no hot path changes.

Verified by modelling the acquire-then-abandon sequence directly against
GarnetTcpNetworkSender: when a foreign thread disposes the sender while the
worker holds a rented frame, LimitedFixedBufferPool.Dispose() never completes;
when the owning thread returns its own frame, the pool drains immediately.

Also fix RespTests.ClientKillTestAsync, which asserted IsConnected was false
after a single fire-and-forget Ping. Socket.Connected reflects the last
completed I/O, and TCP guarantees the first send after a remote FIN succeeds
into the kernel buffer, so the assertion could only pass by winning a race with
the client's receive loop. It now pings until the disconnect surfaces, bounded
at 10s, so a CLIENT KILL that genuinely failed still fails the test.

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

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Close the connect-time window where a connection break is lost

AofSyncDriver.Dispose() relies on CloseConnection() to fail a worker's pending
network I/O so the worker exits and the driver's wait for it completes. That
signal could be lost: CloseConnection() reads the socket field, and a worker
inside ConnectAsync has not published it 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 and CLIENT SETNAME exchanges that
follow await a reply with no cancellation token. A worker that lost the break
therefore rents its send buffer and parks forever on a peer that is also shutting
down, so the driver's wait never completes and the pool dispose spins.

Make it a handshake: CloseConnection() records the request before closing, and
the connecting thread rechecks it immediately after publishing the socket, so
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 need a StoreLoad fence for the handshake to hold. Publishing the
socket and reading the request are a store then a load of a different location,
which the acquire load does not order, so without the barrier both sides can read
stale values and the break is lost anyway.

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

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Keep the worker monitor exit unconditional in the AOF sync task

Disposing the client before leaving the monitor means the monitor exit is
now downstream of a call that can throw. A throw there would leave the
worker counted as active forever, so AofSyncDriver.Dispose would block
indefinitely waiting for the monitor to drain -- turning a client teardown
failure into a hang of the whole replication dispose path.

Wrap the client dispose so the exit always runs, and log the failure.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Stop logging routine background-task cancellation as Error/Critical

CommitTaskAsync and CompactionTaskAsync (StoreWrapper) and MainMonitorTaskAsync
(GarnetServerMonitor) caught every exception and logged it at Error/Critical,
including the OperationCanceledException that is raised as part of normal
operation when their own token is cancelled. Their sibling tasks,
AutoCheckpointTask and ObjectCollectTaskAsync, already suppress exactly this
case.

The cancellation is expected on two routine paths: store wrapper disposal, and
SuspendPrimaryOnlyTasksAsync when a node transitions to a replica via
CLUSTER REPLICATE. Cluster tests therefore emitted a burst of Error/Critical
entries on every replica transition and every server teardown.

Measured over the PrimaryUnavailableRecoveryAsync fixture (6 cases, 2 cores):
42 "CompactionTask exception received" plus 36 "MainMonitorTask exception"
entries per run before, 0 after, with identical pass rate and timings.

Since cluster tests log at LogLevel.Error, these were most of what the CI logs
contained, which made genuine failures in this suite very hard to diagnose.
The catch filter is guarded on token.IsCancellationRequested, so a cancellation
originating from any other source is still reported.

* Keep a node gossiping after a single gossip round fails

GossipMainAsync ran InitConnectionsAsync and the configuration broadcast directly
inside the loop's try block, so any exception raised by a round left the loop,
disposed the cluster connection store and decremented the active task count.
TryStartGossipTasks is called once, from ClusterManager.Start, and nothing
supervises or restarts the task, so the node stopped gossiping for the remainder
of the process.

The node keeps answering gossip it receives, so it still looks healthy, and the
only trace is one Warning. Cluster tests log at LogLevel.Error, so in CI this is
completely silent. What follows is a cluster that can no longer converge on any
subsequent configuration change: WaitForSyncAsync polls until the test token
expires and reports "Cancellation Requested", with nothing in the log between the
last setup message and teardown.

A round failing is transient by nature: it covers connection setup, including the
TLS handshake, and a broadcast to every peer, each with its own timeout. Losing
gossip permanently is not a reasonable response to one of those failing.

Failures raised while the round runs are now logged and the loop continues at the
next gossip interval. Cancellation still leaves the loop, and is matched on
OperationCanceledException rather than only TaskCanceledException, so the
cancellation raised while disposing banned worker connections is no longer
reported as an error during shutdown.

ClusterGossipSurvivesFailedRound covers this: it introduces the nodes, fails one
round on every node, and then assigns slots, which can only propagate if the
loops are still running. Reverting to the previous behaviour fails the test after
30s with "node 0 never converged after a gossip round failed".

* Reconnect promptly after a test restarts a node

The shared StackExchange.Redis configuration used by every Garnet test set
ReconnectRetryPolicy to LinearRetry(10s). That policy gates how soon the
multiplexer may retry a dropped connection, so the first command issued after a
test restarts a node blocks until the gate opens.

Measured on ClusterManagementTests.PrimaryUnavailableRecoveryAsync, which
restarts two replicas and then talks to them: a bare PING to the first restarted
node took 10250-10656ms, while the second node - restarted at the same moment
and reached once the gate had already opened - answered in 0-2ms. The server was
serving the whole time; only the client was waiting.

That put the test at 24-25s against its 30s budget on an idle two-core machine,
with a hard-coded 10s workload window accounting for most of the rest. The
remaining slack was smaller than normal CI jitter, which is why it failed
intermittently in two different places depending on where the clock ran out.

Shortening the gate to 250ms drops the same PING to 292-501ms and the test to
13-16s. No timeout was raised, no assertion relaxed, and no retry added; the
dead time was removed. Every test that restarts a node benefits.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Complete the TLS handshake off the accept loop

GarnetServerTcp ran the server-side TLS handshake inline in HandleNewConnection,
which executes on the accept loop. The handshake is several network round trips
driven by the peer and has no deadline, so the loop stayed parked in it: while one
peer negotiated, no other connection was accepted, and a peer that connected without
ever sending a ClientHello stopped the listener permanently.

That serialization is why TLS clusters are the ones that stall in CI. Every node
opens gossip and replication connections to every other node, so one slow handshake
on a small runner holds up the rest and the cluster never reaches the configuration
the test is waiting for - with nothing logged, because the accept loop is not failing,
only waiting.

Start the handler asynchronously instead. Socket setup still happens synchronously on
the accept loop; only the handshake moves to the thread pool, and its outcome is
observed so a failure is logged and the handler disposed exactly as before.

* Report which node disagreed when a cluster never converges

WaitForSyncAsync polled until its deadline and then reported only
"Cancellation Requested", which says nothing about what it was waiting for.
Describe which of the expected nodes the server had not agreed on, and how,
so a cluster that fails to converge is diagnosable from the CI log alone.

The description is built solely on the failure path, so the polling loop is
unchanged.

The MergeSlotMap fix this originally accompanied landed upstream in #2089,
which restructures the same guard and adds wider regression coverage.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Ship the object log up to the address the transferred main log needs

GetLogFileSize reports the byte ranges that cluster full sync copies to a
replica. The main log range ends at flushedLogicalAddress, captured at
PERSISTENCE_CALLBACK, but the main object log range ended at
snapshotStartObjectLogTail, captured earlier at WAIT_FLUSH. The two are not a
matching pair.

ShiftReadOnlyAddress defers its flush through epoch.BumpCurrentEpoch, so the
drain action runs on whichever thread next observes the epoch as safe. When that
lands between WAIT_FLUSH and PERSISTENCE_CALLBACK, flushedLogicalAddress and
hlogEndObjectLogTail both advance while snapshotStartObjectLogTail does not. The
replica then receives main log records whose object bytes were never sent,
ObjectLogReader.Read returns zero and deserialization throws
EndOfStreamException. Local recovery of the same checkpoint succeeds, because
the files there are complete, which is why only replication saw this.

Recovery installs hlogEndObjectLogTail as the object log tail, so that is the
address the transferred range has to reach.

TransferredSnapshotCheckpointRecoversAllObjects copies exactly the reported
ranges and recovers from them, with a page aligned ShiftReadOnlyAddress injected
into the checkpoint window.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Pulse the replica in the time domain its replay side actually uses

The primary emits a tail witness pulse whenever multi-log is on, which is
either several physical sublogs or several replay tasks. It sourced the pulse
value from the sequence number generator, but that generator is only created
when the log is sharded across several physical sublogs. A single physical
sublog replayed by several tasks therefore dereferenced a null generator, and
the NullReferenceException took down the AOF sync task roughly a tail witness
period after it started. Every write after that point stayed on the primary.

Allocating a generator for that configuration would have replaced the crash
with something worse. Records on a single physical sublog carry no sequence
number; the replay side times its virtual sublogs by log address, publishing
the address it has reached. A generator value is a stopwatch reading, so
feeding one into that comparison would push the read consistency frontier
arbitrarily far ahead of the data and hold it there. The pulse now carries the
observed tail, which is the address replay itself publishes once it has
consumed everything at or below it.

The window is why this only ever surfaced under load. The pulse fires once the
first witness period elapses, so a primary that ships its whole payload sooner
never reaches it, and a slower one dies with the replica still empty.

ClusterReplicationSinglePhysicalLogTailWitnessPulseTest leaves the sync task
idle past a witness period before writing anything. Without the change it
reproduces the CI failure exactly, the replica pinned at 64 while the primary
moves on; with it the fixture passes 106 of 106 pinned to two cores.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Cover slot ownership across every gossip ordering

The merge rule that lets a replica take a slot it only relays needs the
receiver to hear from the replica before it hears from the owning primary,
which is a question of ordering rather than of any single merge call. Drive
the real merge and serialization code through the layout the cluster tests
build, over many orderings of the gossip exchanges.

A configuration only goes on the wire when it changed since the last send on
that connection, so the simulation carries the same per-connection caches the
server keeps. That is what makes a divergence permanent rather than transient:
a view the receiver refused is never offered again, and the cluster sits
quiescent exchanging empty pings.

Without the merge fix the simulation stops converging within a handful of
trials, with the true owner holding no slots while its replica holds them all.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Fail a replica sync whose checkpoint recovery failed

Recovering the checkpoint a primary ships is the step that gives a replica the
primary's data. When it threw, the error was logged and swallowed unless
FailOnRecoveryError was set, and the sync carried on to initialize the AOF and
adopt the replication offset the primary sent. The replica then advertised
being caught up while holding none of the primary's data, and nothing anywhere
reported a problem.

FailOnRecoveryError governs whether a standalone server starts despite a bad
local checkpoint, which is a different question from whether a replica may
silently diverge, so the replica path now propagates regardless of it. The
caller already handles this: the sync aborts, the error travels back over
CLUSTER REPLICATE, and the operator sees why. MultiDatabaseManager already
throws unconditionally on this path and needs no change.

ClusterReplicaCheckpointRecoveryFailureAbortsSyncTest fails the recovery
through the existing DEBUG-only injection points. Without the change the
replica answers OK to a sync that recovered nothing, which is the silent data
loss itself.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Give the testhost longer to connect back to vstest

vstest.console launches a testhost process and waits for it to connect back.
The default ninety seconds is occasionally not enough on a loaded two-core
runner, and when it elapses the job fails having run no tests at all, with
nothing to attribute it to beyond "machine slowness".

This governs process startup only. A test that hangs once it is running is
still caught by the blame hang timeout, so nothing about a real stall is
hidden by the longer wait.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Drop the dedicated-thread workaround for SE.Redis result boxes

StackExchange.Redis 2.x parked a synchronous caller on a [ThreadStatic]
result box. A server error reply faulted that box from the connection's
reader thread, and when the fault landed before the caller reached
Monitor.Wait the caller skipped the wait and recycled the box while the
reader thread still had a PulseAll outstanding on it. The next
synchronous call on that thread took the recycled box, consumed the
stale pulse, and returned with neither a result nor an exception,
shifting every subsequent reply on that thread by one.

TestUtils.ThrowsRedisException sidestepped that by running each
expected-to-fail call on its own thread, keeping the recycled box off
the shared test thread. The bump to 3.2.0 fixes the box handling
upstream, so the workaround now only obscures what the tests assert.

Reverts all 178 call sites to ClassicAssert.Throws and deletes the
helper along with the using it needed. LuaScriptTests.cs and
RespVectorSetTests.cs are byte-identical to their pre-workaround state.

Verified rather than assumed: with the helper gone, LuaScriptTests
(123 of the reverted sites) ran 4x at 435/435 and RespVectorSetTests
(the other 55) 164/164, all under taskset -c 0-1 to match the two-core
CI runners - 1,904 executions with no shifted replies.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Recover Vector Sets from the whole log, not just the snapshot

Two independent recovery defects, both of which predate this branch.

A live Vector Set could lose every one of its element records.
ReconcileRecoveredState decides liveness by absence: a context marked
in use in ContextMetadata but missing from recoveredIndexes is treated
as abandoned, and the cleanup task then deletes the set's element
records. But recoveredIndexes was populated only from
OnRecoverySnapshotRead, which the store raises only for records inside
the checkpoint snapshot's address range - ClearBitsOnPage gates on
snapshotFromAddress > 0, and the main-log pass supplies 0. An index
record flushed to the main log before the checkpoint is therefore
recovered but never reported, so its set is judged abandoned. The stub
record survives, which is why the key still exists and VDIM still
answers while VEMB returns nothing at all.

Completing the census from the recovered store is what fixes it, so
liveness is decided against every index record rather than the subset
the snapshot happened to cover. The array of ContextMetadata now spans
recovered metadata and recovered index contexts both, because the
metadata record holding a context can be missing when its index record
is not.

Recovery could also abort outright. A snapshot covers a range, so it
legitimately presents several versions of one metadata record when an
update lands in the checkpoint's fuzzy region and is copied to the
tail. RecoveredContextMetadata assumed at most one and threw. Recovery
swallows that, leaving the server up on a partially recovered store.
ContextMetadata.Version already ordered these versions and simply was
not consulted; newest now wins. Empty versions are kept until
ReconcileRecoveredState trims them, so a newest version that released
its last context still supersedes older non-empty ones.

Both regression tests are deterministic rather than timing luck. The
first fills pages through the product's own flush path and guards on
FlushedUntilAddress > BeginAddress so it cannot silently stop covering
the case. The second choreographs two [Conditional("DEBUG")] barriers
to release a metadata RMW into the new version while the checkpoint is
parked before it captures finalLogicalAddress.

Ablation, taskset -c 0-1: with the fix reverted all four executions
fail, two with "Expected: 8 But was: 0" and two with
"Recovered multiple instances of the same ContextMetadata"; with it
applied all four pass. Full suite 404/404.

No hot path is touched. Both sites run only during recovery, and the
added scan uses the same IterateLookupSnapshot facility the cleanup
task already runs.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Fix hash field expiration on copy update, and make expiry/blocking tests deterministic

HashObject: initialize the expiration span lookup in the copy constructor

HasExpirableItems tests expirationTimes, but IsExpired(ReadOnlySpan<byte>)
reads expirationTimeSpanLookup. InitializeExpirationStructures and
CleanupExpirationStructuresIfEmpty keep the two in sync; the copy constructor
behind Clone() set only expirationTimes, leaving the alternate lookup as a
default struct whose inner dictionary is null.

Clone() is the copy-update path, so any RMW against a hash that carries field
expirations and whose record is immutable - in the read-only region, or read
back from disk - dereferenced a null dictionary and terminated the session.
CanDoHashExpireLTM reproduces this: without the fix it fails with SocketClosed
after the server throws NullReferenceException from HashObject.ContainsKey via
PostCopyUpdater. The path exists only on .NET 9 and above.

Tests: express member expiration as absolute deadlines

Hash field and sorted set member expiry tests set short relative TTLs and then
asserted that a member was still alive, which holds only if less wall-clock
time elapses between two commands than the TTL. On a loaded two-core runner
that bound is not guaranteed, and the member expires before the assertion runs.

MemberExpiry supplies Pending() for members that must stay alive and Imminent()
paired with WaitUntilPast() for members that must expire. Waiting for a deadline
to pass is a lower bound and is unaffected by a slow runner. Assertions are
unchanged or tightened: HEXPIRETIME and HPEXPIRETIME now compare exactly against
the deadline that was set, where before they bracketed a range. The suites also
no longer sleep for fixed multi-second intervals.

CanDoHashCollect additionally sets its pending expirations before arming the
imminent one. HashExpire purges already-expired fields as its first action, so a
stall between the two commands would have dropped the fields that HCOLLECT is
meant to reclaim.

Tests: stop starving the thread pool in MultipleClientsUnblockAndAddTest

The test dispatched 23 work items through Task.Run, each making a blocking
StackExchange.Redis call. With ThreadPool.MinThreads equal to the two available
cores, the pool injected replacement threads at roughly one per second, so the
last task did not start for seven seconds and the commands reached the server
after the ten second BLMPOP deadline. Issuing them through the async API lets
them all reach the server promptly, which is also the interleaving the test
exists to exercise.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Load the test client certificate once instead of per connection

TLS-enabled tests build their SslClientAuthenticationOptions from a factory
delegate that the client invokes once per physical connection, and that factory
imported testcert.pfx from disk every time. Importing a PKCS#12 file is
expensive - on Windows it materializes an on-disk key container - so the import
landed inside the client's 2s connect timeout, repeated on every connection and
every retry. The imported X509Certificate2 was also never disposed, so each
import leaked a key handle for the lifetime of the run.

Import the certificate once and share it. Sharing a single X509Certificate2
across SslStream connections is the normal pattern.

Measured on a TLS cluster test pinned to 2 cores, PKCS#12 imports per test drop
from 7 to 3, with the 4 removed imports all on the client connect path.

This targets ClusterManagementTests.ReplicasRestartAsReplicasAsync and
PrimaryUnavailableRecoveryAsync, the only two TLS tests in that fixture and the
only two that flake there. Those failures reproduce only on the Windows runner,
so this removes measured redundant work from the path that times out rather
than being verified against the failure itself.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Settle background context cleanup before sampling Vector Set slot state

RespVectorSetTests.RenamesAsync reads VectorManager context state straight
after a rename. A context released by the preceding delete or overwrite is
only marked for cleanup by the background cleanup task, so until that task
runs the released context still reads as in-use and the slot reports two live
contexts rather than one.

Wait for that work to quiesce before sampling, using the primitive
VectorSetOverwriteTests already uses for the same reason. The assertion is
unchanged and no less strict.

Captured at the moment of failure, the slot reported 16 namespaces; calling
WaitForQuiescence at that same instant reported 8, and it stayed 8 three
seconds later. The context is released correctly and only the sampling was
early, so there is nothing to fix in VectorManager itself.

Pinned to two cores under CPU load the test failed on iterations 11 and 14 of
two separate 60-run loops. With this change 60 runs are clean.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Address review feedback on comments

Correct the description of the concurrent flush in ObjectLogCheckpointTransferTests:
the test shifts ReadOnlyAddress, so what it models is the ReadOnly flush rather
than eviction driven by LogSizeTracker.

Document recoveredIndexes and recoveredMetadata, and the closeRequested flag.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Fix two migration and recovery defects found in review, and drop two timeout increases

Two contexts could end up sharing one namespace block. A context chosen by the
PRIMARY can be occupied on the node receiving the migration, so that migration is
steered onto a context that is free locally, and the context it lands on is marked
migrating. That mark is also how an interrupted and resumed migration looks, so a
later incoming context whose own value happened to equal that one read it as its
own reservation and adopted it, leaving both migrations writing to one context.
Remembering which local contexts have already been claimed keeps the mapping
injective.

Reconciling recovered state rebuilt the context metadata array from the recovered
metadata alone and defaulted every index that was not represented. A cluster node
with AOF enabled reconciles twice while starting up, once from
RecoverCheckpointAndAOFAsync and again from StoreWrapper, and the first pass
consumes the recovered metadata, so the second had only the index records to go
on. A context that is in use without an index record - a Vector Set that was
deleted but whose data has not finished being cleaned up - was dropped instead of
being marked for cleanup, freeing it for the next Vector Set while its old data was
still present, and the persisted version was reset so later metadata writes were
cancelled as stale. Entries with nothing recovered now keep what is already in
memory.

The multi-slot Vector Set migration test polled without sleeping on the path where
the migration had not finished, so it spun a core flat out for the whole window and
starved the cluster nodes it was waiting on. Sleeping on every path is the fix, and
the window it was given goes back to the five seconds the single-slot variant uses:
on two cores, main fails five runs in ten at five seconds, and sleeping on every
path fails none.

The testhost connection timeout increase goes with it. Ten failed runs, thirty
failed jobs, contain no instance of that timeout elapsing, so there was nothing to
justify raising it.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Take StackExchange.Redis 3.2.1

3.2.0 assumed IBufferWriter<byte>.GetSpan(sizeHint) returns at least sizeHint
bytes.  It guarantees one byte; the hint is advisory.  When RESPite's
CycleBuffer handed back a recycled segment shorter than the hint,
MessageWriter.WriteUnifiedSpan sliced past the end of it and the connection
was torn down with an ArgumentOutOfRangeException, failing the in-flight
command.  That showed up here as intermittent RedisConnectionException on
concurrent VADD.

3.2.1 is StackExchange/StackExchange.Redis#3220, which checks the returned
length and falls back to a piecewise write.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Tighten two comments to describe current behaviour

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Wait for the client to observe a promotion before writing to a new primary

StackExchange.Redis caches a per-endpoint replica flag and rejects a
primary-only command before it leaves the client, refreshing that flag
only on its periodic configuration check. A test that promotes a node
and immediately writes to it therefore races an incidental multiplexer
reconfiguration, and fails with "Command cannot be issued to a replica"
even though the node reports the primary role.

WaitForPrimaryRole now also waits until the multiplexer no longer regards
the node as a replica, forcing it to re-read the topology. The failover
tests use it in place of their own role polls.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Gate the AOF tail witness pulse on a sharded log

The pulse was gated on MultiLogEnabled, which is true when either the log is
sharded across physical sublogs or a single sublog is replayed by several
tasks. The pulse value comes from the sequence number generator, which is
constructed only for a sharded log, so a single sublog with multiple replay
tasks dereferenced a null generator and killed the AOF sync task, stranding
every subsequent write on the primary.

A single physical sublog orders records by log address rather than by
generator sequence number, and every replay task rendezvouses on the same
batch and publishes the same address, so no virtual sublog can lag behind
another for a pulse to relieve. Gate the pulse on AofPhysicalSublogCount
instead, matching how GarnetLog and GarnetAppendOnlyFile already select
between the two ordering schemes.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Use the shared harness configuration in ClusterReplicateFails

ClusterReplicateFails was the only cluster test that hand-built a
connection string, so its client configuration did not match the servers
it creates: the default command map advertises SUBSCRIBE and PUBLISH
against nodes started with pub/sub disabled, and exception detail and
client names were off, leaving a connect failure with no attributable
information.

Route both connections through TestUtils.GetConfig, which trims the
command map to the server's command set, applies the suite's connect
retry policy, and enables exception detail and per-test client names.
The protocol is left unpinned, matching the previous behaviour.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

* Wait for the primary to be known before CLUSTER REPLICATE in tests

CLUSTER REPLICATE resolves the primary's node id against the replica's
own cluster configuration. CLUSTER MEET only makes the node it is sent
to aware of its target; the reverse direction arrives through the gossip
handshake that follows. Tests issue MEET at the primary and REPLICATE at
the replica, so the replica may not yet know the primary and the command
fails with "I don't know about node".

Measured immediately after Meet returns, the replica does not know the
primary in any run; the tests pass only because unrelated setup work
between the two calls usually gives gossip enough time. Issuing REPLICATE
at that point reproduces the failure in every run.

The index-based ClusterReplicate helper now waits for the replica to know
the primary's node id. Tests that assert on the unknown-node error use
the node-id overload and are unaffected.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77

---------

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 30f31d5b-9e53-4111-91a3-e16c94510a77
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants