From 63351294251e32949c610d6a9d7ab4029e51c69f Mon Sep 17 00:00:00 2001 From: Jiajun Peng Date: Tue, 25 Aug 2026 23:54:41 -0700 Subject: [PATCH 1/3] [Cluster] Prevent MergeSlotMap from crediting a slot to a replica sender MergeSlotMap could 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 entirely. assignToWorkerId therefore keeps its default of the sender and is written in as the owner, while the correction that would have redirected it to the sender's primary sits inside the branch that was skipped. The attribution then propagates rather than settling. On the next gossip from that same replica the receiver 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, and the true owner's claim is rejected for as long as the bogus owner's epoch is greater. It does drain, since the bogus owner is a primary that does not claim the slot and its own gossip triggers the stale-ownership reset. But unowned is exactly this bug's precondition, so the reset hands the slot back into the race, and the loop gates only on the sender's slot state, so the receiver holding the slot OFFLINE does not protect it. Whichever gossip arrives first decides: a primary repairs the slot, a replica puts it back to a wrong owner. Convergence therefore 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, the affected nodes answered with MOVED redirects to nodes that did not own the slot. The guard was added by #1435 to fix a NullReferenceException, since workers[RESERVED_WORKER_ID].Nodeid is null. Skipping the block avoided the dereference but turned a throw into silently crediting the replica. Restore the pre-#1435 invariant that a replica sender can never introduce ownership, while keeping the dereference guarded: check for the reserved owner first and skip the slot. An unowned slot then becomes a safe resting state, claimable only by a primary that actually claims it, so the stale-ownership reset settles in one round instead of feeding the race. Adds four regression tests. The unowned-slot and correction-leak tests fail without this change; the ownership-preservation and planned-failover hand-off tests guard against regressing behavior that already worked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- libs/cluster/Server/ClusterConfig.cs | 12 +- .../Garnet.test.cluster/ClusterConfigTests.cs | 148 ++++++++++++++++++ 2 files changed, 159 insertions(+), 1 deletion(-) diff --git a/libs/cluster/Server/ClusterConfig.cs b/libs/cluster/Server/ClusterConfig.cs index 046e92d8da1..641605bf170 100644 --- a/libs/cluster/Server/ClusterConfig.cs +++ b/libs/cluster/Server/ClusterConfig.cs @@ -1198,8 +1198,18 @@ 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 be + // permanent, because the true owner is afterwards rejected by the config epoch comparison. + // NOTE: this check must precede the node-id comparison, since + // workers[RESERVED_WORKER_ID].Nodeid is null and dereferencing it was the failure fixed by #1435. + 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..429edd00815 100644 --- a/test/cluster/Garnet.test.cluster/ClusterConfigTests.cs +++ b/test/cluster/Garnet.test.cluster/ClusterConfigTests.cs @@ -354,5 +354,153 @@ 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 is permanent: + /// the true owner is afterwards rejected by the config epoch comparison against the bogus owner. + /// 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 From 9728a20e3ab32520a2f9d2a4e02ee48a645c3bc4 Mon Sep 17 00:00:00 2001 From: jiajunpeng-msft <32749342+jiajunpeng-msft@users.noreply.github.com> Date: Thu, 10 Sep 2026 10:30:20 -0700 Subject: [PATCH 2/3] Apply suggestion from @Mathos1432 Co-authored-by: Mathieu Tremblay --- libs/cluster/Server/ClusterConfig.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/libs/cluster/Server/ClusterConfig.cs b/libs/cluster/Server/ClusterConfig.cs index 641605bf170..89ed3a40834 100644 --- a/libs/cluster/Server/ClusterConfig.cs +++ b/libs/cluster/Server/ClusterConfig.cs @@ -1205,8 +1205,6 @@ public ClusterConfig MergeSlotMap(ClusterConfig senderConfig, ILogger logger = n // 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 be // permanent, because the true owner is afterwards rejected by the config epoch comparison. - // NOTE: this check must precede the node-id comparison, since - // workers[RESERVED_WORKER_ID].Nodeid is null and dereferencing it was the failure fixed by #1435. if (currentOwnerId == RESERVED_WORKER_ID) continue; From 43f5630ad3f757465b2ba64680897bcfbb3e28c4 Mon Sep 17 00:00:00 2001 From: Jiajun Peng Date: Fri, 11 Sep 2026 10:17:49 -0700 Subject: [PATCH 3/3] Describe the rejection window accurately in comments The comment on the replica branch and the doc comment on ClusterConfigMergeSlotMapReplicaSenderCannotClaimUnownedSlotTest both called a bad attribution permanent. It is not: the wrongly credited owner is a primary that does not claim the slot, so its own gossip eventually triggers the stale-ownership reset and returns the slot to unowned. The true owner is rejected only while the bogus owner's config epoch remains the higher one. Comments only, no behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- libs/cluster/Server/ClusterConfig.cs | 4 ++-- test/cluster/Garnet.test.cluster/ClusterConfigTests.cs | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/libs/cluster/Server/ClusterConfig.cs b/libs/cluster/Server/ClusterConfig.cs index 946f71871e0..0b8eac849ba 100644 --- a/libs/cluster/Server/ClusterConfig.cs +++ b/libs/cluster/Server/ClusterConfig.cs @@ -1203,8 +1203,8 @@ public ClusterConfig MergeSlotMap(ClusterConfig senderConfig, ILogger logger = n // 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 be - // permanent, because the true owner is afterwards rejected by the config epoch comparison. + // 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; diff --git a/test/cluster/Garnet.test.cluster/ClusterConfigTests.cs b/test/cluster/Garnet.test.cluster/ClusterConfigTests.cs index 429edd00815..e4b11afa9b3 100644 --- a/test/cluster/Garnet.test.cluster/ClusterConfigTests.cs +++ b/test/cluster/Garnet.test.cluster/ClusterConfigTests.cs @@ -404,8 +404,9 @@ private static ClusterConfig CreateReceiverAwareOf(ClusterConfig other, params i } /// - /// A replica sender must not be credited with a slot the receiver holds unowned. Doing so is permanent: - /// the true owner is afterwards rejected by the config epoch comparison against the bogus owner. + /// 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)]