[Cluster] Prevent MergeSlotMap from crediting a slot to a replica sender

[jiajunpeng-msft]

Root Cause

ClusterConfig.MergeSlotMap can credit a hash slot to a node that never claimed it.

When the sender is a replica and the receiver holds the slot unowned, the else if (currentOwnerId != RESERVED_WORKER_ID) guard is false, so the block is skipped:

var assignToWorkerId = GetWorkerIdFromNodeId(senderConfig.LocalNodeId);   // (1) defaults to the SENDER
...
else if (currentOwnerId != RESERVED_WORKER_ID)                            // (2) FALSE when the slot is unowned
{
    if (!workers[currentOwnerId].Nodeid.Equals(senderConfig.LocalNodeId)) continue;
    assignToWorkerId = GetWorkerIdFromNodeId(senderConfig.LocalNodePrimaryId);   // the correction
}
newSlotMap[i]._workerId = assignToWorkerId;                               // (3) writes the SENDER

assignToWorkerId keeps its default from (1), the replica sender, and (3) writes it in as owner. The correction that would have redirected it to the sender's primary lives inside the branch that is skipped in precisely the case where it is needed. A replica sender reaches this path at all because the stale-ownership reset above it is gated on && senderConfig.IsPrimary.

The attribution then propagates rather than settling. On the next gossip from that same replica the receiver now does credit the slot to the sender, so the planned-failover branch is entered and the slot is handed to the replica's primary — a node generally unrelated to the slot, and with no epoch gate on that path. The slot ends up owned by a node that never claimed it, carrying an arbitrary config epoch, and the true owner's claim is rejected for as long as that epoch is the greater one.

It does drain: the bogus owner is a primary that does not claim the slot, so its own gossip triggers the stale-ownership reset, returning the slot to unowned, after which a genuine primary claim wins unconditionally against ConfigEpoch == 0. But unowned is exactly this bug's precondition, so the reset hands the slot straight back into the race — and note the loop gates only on senderSlotMap[i]._state, the sender's view, so the receiver holding the slot OFFLINE does not protect it. Whichever gossip arrives first decides: a primary's claim repairs the slot, a replica's puts it back to a wrong owner. Repair and corruption alternate, and convergence depends on the true owner repeatedly winning that race against every replica that gossips. The same window exists for any node that joins with an empty slot map. On a large cluster this took about a day to settle, and until it did, affected nodes answered with MOVED redirects to nodes that did not own the slot.

This is a regression from #1435 (54286ac0f1), which added the currentOwnerId != RESERVED_WORKER_ID guard. Before it the branch was a plain else, so every replica sender was forced through the node-id check and the correction could not be bypassed — but on an unowned slot workers[RESERVED_WORKER_ID].Nodeid is null, so .Equals(...) threw. Guarding that dereference by skipping the block also skipped the correction, turning a throw into silently crediting the replica.

Description of Change

libs/cluster/Server/ClusterConfig.cs — restore the plain else and check for the reserved owner first, so an unowned slot is skipped rather than assigned:

if (currentOwnerId == RESERVED_WORKER_ID)
    continue;

if (!workers[currentOwnerId].Nodeid.Equals(senderConfig.LocalNodeId))
    continue;

assignToWorkerId = GetWorkerIdFromNodeId(senderConfig.LocalNodePrimaryId);

A replica can still hand off a slot the receiver already credits to that replica, which is the planned-failover case the branch exists for and where its safety comes from the node-id check. An unowned slot offers no such basis, so it is left for its real owner, whose claim always succeeds because workers[RESERVED_WORKER_ID].ConfigEpoch is 0. With that path closed, an unowned slot can only ever be taken by a primary that actually claims it, so the stale-ownership reset settles in one round instead of feeding the race described above. The null dereference #1435 fixed stays guarded, since the reserved check precedes it.

No public API, configuration, or wire-format change.

test/cluster/Garnet.test.cluster/ClusterConfigTests.cs — four regression tests:

Test	Without fix
...ReplicaSenderCannotClaimUnownedSlotTest	fails — replica recorded as owner
...ReplicaHandoffDoesNotLeakToLaterSlotsTest	fails — a later unowned slot inherits an earlier hand-off
...ReplicaSenderCannotStealOwnedSlotTest	passes — guards the "4 nodes A,B,C,D" invariant
...ReplicaSenderHandsOffOwnedSlotToPrimaryTest	passes — planned failover must not regress

The first also asserts Assert.DoesNotThrow, pinning the #1435 NRE so the guard cannot be lost again.

Validation

Garnet.test.cluster 160/160, Garnet.test.cluster.replication 107/107 and Garnet.test.cluster.migrate 56/56 pass on net8.0 Debug. Release build of Garnet.cluster is warning-clean and dotnet format Garnet.slnx --verify-no-changes is clean.

Both new failing tests were confirmed to fail against unpatched main and pass with the change; the two hand-off tests pass in both states.

Issues Fixed

No existing issue — filed directly as a PR. Happy to open a tracking issue if preferred.

Assisted-By: GitHub Copilot

[Copilot]

Pull request overview

Prevents replica gossip from claiming unowned cluster slots while preserving planned-failover handoffs.

Changes:

• Rejects replica ownership updates for unowned slots.
• Adds regression coverage for ownership and handoff scenarios.

Reviewed changes

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

File	Description
libs/cluster/Server/ClusterConfig.cs	Guards unowned slots during replica merges.
test/cluster/Garnet.test.cluster/ClusterConfigTests.cs	Adds four replica merge regression tests.

---

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