diff --git a/libs/cluster/Server/ClusterConfig.cs b/libs/cluster/Server/ClusterConfig.cs
index f85905ed7da..0b8eac849ba 100644
--- a/libs/cluster/Server/ClusterConfig.cs
+++ b/libs/cluster/Server/ClusterConfig.cs
@@ -1198,8 +1198,16 @@ public ClusterConfig MergeSlotMap(ClusterConfig senderConfig, ILogger logger = n
if (senderConfig.LocalNodeConfigEpoch != 0 && workers[currentOwnerId].ConfigEpoch >= senderConfig.LocalNodeConfigEpoch)
continue;
}
- else if (currentOwnerId != RESERVED_WORKER_ID) // Possibly multiple replicas may enter this but only the old primary should succeed in the event of a planned failover.
+ else
{
+ // Sender is a replica. It may only hand off a slot that this node already credits to the
+ // sender itself, which is the planned-failover case described below. An unowned slot gives
+ // no such basis, so leave it alone and let its real owner claim it through the primary path
+ // above; a replica must never introduce ownership. Crediting the replica here would reject
+ // the true owner's claims for as long as the bogus owner's config epoch remains the higher one.
+ if (currentOwnerId == RESERVED_WORKER_ID)
+ continue;
+
// This should guarantee that only the old primary should proceed with re-assigning the slots to the replica that is taking over
// Scenario 4 nodes A,B,C,D for which B,C are replicas of A and B takes over from A,
// then due to delay D will receive a gossip from A,B,C in any order.
diff --git a/test/cluster/Garnet.test.cluster/ClusterConfigTests.cs b/test/cluster/Garnet.test.cluster/ClusterConfigTests.cs
index e7669ad2764..e4b11afa9b3 100644
--- a/test/cluster/Garnet.test.cluster/ClusterConfigTests.cs
+++ b/test/cluster/Garnet.test.cluster/ClusterConfigTests.cs
@@ -354,5 +354,154 @@ public void ClusterConfigMergeSlotMapAccumulatesUpdatedAcrossSlotsTest()
Assert.That(merged.GetNodeIdFromSlot(StaleSlot), Is.Not.EqualTo(senderId),
"stale attribution should be cleared");
}
+
+ ///
+ /// Builds a replica whose slot map credits its own primary with , which is what a
+ /// replica gossips in a healthy cluster, and returns it alongside the two node-ids involved.
+ ///
+ private static (ClusterConfig replica, string replicaId, string primaryId) CreateReplicaSender(
+ long primaryEpoch, long replicaEpoch, params int[] slots)
+ {
+ var primaryId = Generator.CreateHexId();
+ var replicaId = Generator.CreateHexId();
+
+ var primary = new ClusterConfig().InitializeLocalWorker(
+ primaryId, "127.0.0.1", ClusterTestContext.Port + 1,
+ primaryEpoch, Garnet.cluster.NodeRole.PRIMARY, null, "");
+ foreach (var slot in slots)
+ primary = primary.UpdateSlotState(slot, ClusterConfig.LOCAL_WORKER_ID, SlotState.STABLE);
+
+ // The replica learns the slots from its primary, so its own map has them STABLE under the primary.
+ var replica = new ClusterConfig()
+ .InitializeLocalWorker(
+ replicaId, "127.0.0.1", ClusterTestContext.Port + 2,
+ replicaEpoch, Garnet.cluster.NodeRole.REPLICA, primaryId, "")
+ .Merge(primary, []);
+
+ Assert.That(replica.GetNodeIdFromSlot((ushort)slots[0]), Is.EqualTo(primaryId),
+ "precondition: the replica's map should credit its primary with the slot");
+
+ return (replica, replicaId, primaryId);
+ }
+
+ ///
+ /// Creates a primary that knows the replica and its primary, with every supplied slot left unowned.
+ ///
+ private static ClusterConfig CreateReceiverAwareOf(ClusterConfig other, params int[] unownedSlots)
+ {
+ var receiver = new ClusterConfig()
+ .InitializeLocalWorker(
+ Generator.CreateHexId(), "127.0.0.1", ClusterTestContext.Port + 3,
+ configEpoch: 1, Garnet.cluster.NodeRole.PRIMARY, null, "")
+ .Merge(other, []);
+
+ // A node that just started with CleanClusterConfig, or one whose stale attribution was reset by
+ // MergeSlotMap, knows the other workers but holds the slot unowned.
+ foreach (var slot in unownedSlots)
+ receiver = receiver.UpdateSlotState(slot, ClusterConfig.RESERVED_WORKER_ID, SlotState.OFFLINE);
+
+ return receiver;
+ }
+
+ ///
+ /// A replica sender must not be credited with a slot the receiver holds unowned. Doing so blocks the
+ /// true owner, whose claim loses the config epoch comparison against the bogus owner until gossip
+ /// from that owner resets the slot back to unowned.
+ /// Also guards the null dereference of workers[RESERVED_WORKER_ID].Nodeid fixed by #1435.
+ ///
+ [Test, Order(12)]
+ [Category("CLUSTER-CONFIG"), CancelAfter(1000)]
+ public void ClusterConfigMergeSlotMapReplicaSenderCannotClaimUnownedSlotTest()
+ {
+ const int UnownedSlot = 300;
+
+ // The replica's epoch exceeds its primary's, which is what makes a wrong assignment unrecoverable.
+ var (replica, replicaId, primaryId) = CreateReplicaSender(primaryEpoch: 10, replicaEpoch: 30, UnownedSlot);
+ var receiver = CreateReceiverAwareOf(replica, UnownedSlot);
+
+ Assert.That(receiver.GetWorkerIdFromSlot(UnownedSlot), Is.EqualTo(ClusterConfig.RESERVED_WORKER_ID),
+ "precondition: the receiver holds the slot unowned");
+
+ ClusterConfig merged = null;
+ Assert.DoesNotThrow(() => merged = receiver.Merge(replica, []),
+ "workers[RESERVED_WORKER_ID].Nodeid is null, and dereferencing it was the failure fixed by #1435");
+
+ Assert.That(merged.GetNodeIdFromSlot(UnownedSlot), Is.Not.EqualTo(replicaId),
+ "a replica must never be recorded as the owner of a slot");
+ Assert.That(merged.GetWorkerIdFromSlot(UnownedSlot), Is.EqualTo(ClusterConfig.RESERVED_WORKER_ID),
+ "the slot must stay unowned so its real primary can still claim it");
+ Assert.That(merged.GetNodeIdFromSlot(UnownedSlot), Is.Not.EqualTo(primaryId),
+ "the replica's primary has no claim to the slot through a replica's gossip either");
+ }
+
+ ///
+ /// A replica sender must not take a slot away from a node the receiver already credits with it.
+ ///
+ [Test, Order(13)]
+ [Category("CLUSTER-CONFIG"), CancelAfter(1000)]
+ public void ClusterConfigMergeSlotMapReplicaSenderCannotStealOwnedSlotTest()
+ {
+ const int OwnedSlot = 400;
+
+ var (replica, replicaId, primaryId) = CreateReplicaSender(primaryEpoch: 10, replicaEpoch: 30, OwnedSlot);
+ var receiver = CreateReceiverAwareOf(replica, OwnedSlot);
+
+ // The receiver correctly credits the real primary with the slot.
+ receiver = receiver.UpdateSlotState(OwnedSlot, receiver.GetWorkerIdFromNodeId(primaryId), SlotState.STABLE);
+
+ var merged = receiver.Merge(replica, []);
+
+ Assert.That(merged.GetNodeIdFromSlot(OwnedSlot), Is.EqualTo(primaryId),
+ "ownership by the real primary must be left untouched by a replica's gossip");
+ Assert.That(merged.GetNodeIdFromSlot(OwnedSlot), Is.Not.EqualTo(replicaId));
+ }
+
+ ///
+ /// The planned-failover hand-off must keep working: when the receiver still credits the sender with a slot
+ /// and the sender has since been demoted to a replica, the slot moves to the sender's new primary.
+ ///
+ [Test, Order(14)]
+ [Category("CLUSTER-CONFIG"), CancelAfter(1000)]
+ public void ClusterConfigMergeSlotMapReplicaSenderHandsOffOwnedSlotToPrimaryTest()
+ {
+ const int HandoffSlot = 500;
+
+ var (replica, replicaId, primaryId) = CreateReplicaSender(primaryEpoch: 10, replicaEpoch: 30, HandoffSlot);
+ var receiver = CreateReceiverAwareOf(replica, HandoffSlot);
+
+ // The receiver still believes the demoted sender owns the slot.
+ receiver = receiver.UpdateSlotState(HandoffSlot, receiver.GetWorkerIdFromNodeId(replicaId), SlotState.STABLE);
+
+ var merged = receiver.Merge(replica, []);
+
+ Assert.That(merged.GetNodeIdFromSlot(HandoffSlot), Is.EqualTo(primaryId),
+ "the slot should be handed off to the node that took over from the sender");
+ Assert.That(merged.GetState((ushort)HandoffSlot), Is.EqualTo(SlotState.STABLE));
+ }
+
+ ///
+ /// The hand-off correction applies to the slot that earned it and must not carry over to later slots in
+ /// the same merge.
+ ///
+ [Test, Order(15)]
+ [Category("CLUSTER-CONFIG"), CancelAfter(1000)]
+ public void ClusterConfigMergeSlotMapReplicaHandoffDoesNotLeakToLaterSlotsTest()
+ {
+ const int HandoffSlot = 600; // visited first, takes the hand-off correction
+ const int UnownedSlot = 700; // visited later, must be left alone
+
+ var (replica, replicaId, primaryId) =
+ CreateReplicaSender(primaryEpoch: 10, replicaEpoch: 30, HandoffSlot, UnownedSlot);
+ var receiver = CreateReceiverAwareOf(replica, HandoffSlot, UnownedSlot);
+
+ receiver = receiver.UpdateSlotState(HandoffSlot, receiver.GetWorkerIdFromNodeId(replicaId), SlotState.STABLE);
+
+ var merged = receiver.Merge(replica, []);
+
+ Assert.That(merged.GetNodeIdFromSlot(HandoffSlot), Is.EqualTo(primaryId),
+ "precondition: the earlier slot takes the hand-off correction");
+ Assert.That(merged.GetWorkerIdFromSlot(UnownedSlot), Is.EqualTo(ClusterConfig.RESERVED_WORKER_ID),
+ "the later unowned slot must not inherit the correction computed for the earlier slot");
+ }
}
}
\ No newline at end of file