[Cluster] Fix MergeSlotMap discarding stale-ownership resets - #2068
Open
jiajunpeng-msft wants to merge 3 commits into
Open
[Cluster] Fix MergeSlotMap discarding stale-ownership resets#2068jiajunpeng-msft wants to merge 3 commits into
jiajunpeng-msft wants to merge 3 commits into
Conversation
The stale-ownership reset branch in ClusterConfig.MergeSlotMap mutates the working slot map but never sets `updated`, so a gossip merge whose only effect is clearing stale slot attributions is thrown away by the `return updated ? new(newSlotMap, workers) : this` at the end of the method. The receiver therefore keeps crediting a node that no longer claims the slot. Because the merge is epoch-gated on the *recorded* owner (`workers[currentOwnerId].ConfigEpoch >= senderConfig.LocalNodeConfigEpoch`), and that stale owner's epoch keeps advancing for unrelated reasons, the real owner can never win the comparison and reclaim the slot. The divergence is permanent without outside intervention. Also changes the assignment below to accumulate (`|=`) rather than assign, so a later slot needing no change cannot clobber a `true` set earlier in the loop. Adds ClusterConfigMergeSlotMapRetainsStaleOwnershipResetTest, which fails without the fix and passes with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes cluster gossip merges so stale slot-ownership resets are retained and cannot be overwritten by later no-op processing.
Changes:
- Marks stale ownership resets as updates.
- Accumulates updates across all slots.
- Adds a regression test for reset-only merges.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
libs/cluster/Server/ClusterConfig.cs |
Preserves stale slot-map resets during merges. |
test/cluster/Garnet.test.cluster/ClusterConfigTests.cs |
Tests reset-only merge behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // NOTE: this avoids message flooding when sender epoch equals zero | ||
| updated = newSlotMap[i]._workerId != assignToWorkerId || newSlotMap[i]._state != SlotState.STABLE; | ||
| // Accumulate across all slots, including resets handled above. | ||
| updated |= newSlotMap[i]._workerId != assignToWorkerId || newSlotMap[i]._state != SlotState.STABLE; |
Badrish Chandramouli (badrishc)
approved these changes
Aug 18, 2026
The existing ClusterConfigMergeSlotMapRetainsStaleOwnershipResetTest does not exercise the `updated |=` change: its sender config epoch is 20, so the slot the sender genuinely owns is short-circuited by the epoch guard and never reaches the ownership-assignment path. Reverting `|=` to `=` still passed that test. Add ClusterConfigMergeSlotMapAccumulatesUpdatedAcrossSlotsTest, which gives the sender a config epoch of zero so its owned slot bypasses the epoch guard and reaches the assignment with nothing to change. That slot sits at a higher index than the stale-ownership reset, so a plain assignment clears the flag set by the reset and the merge is discarded. Verified the new test fails with `=` and passes with `|=`, while the original test passes either way. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Badrish Chandramouli (badrishc)
requested a review
from Mathieu Tremblay (Mathos1432)
August 18, 2026 20:46
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root Cause
ClusterConfig.MergeSlotMapbuilds a candidate slot map and returns it only if theupdatedflag is set (return updated ? new(newSlotMap, workers) : this;). The stale-ownership reset branch mutatesnewSlotMap— setting the slot toRESERVED_WORKER_ID/SlotState.OFFLINE— but never setsupdated, thencontinues. A gossip merge whose only effect is clearing stale attributions is therefore discarded wholesale, and the receiver keeps crediting a node that no longer claims the slot.This does not self-heal, because the claim path is epoch-gated on the recorded owner (
workers[currentOwnerId].ConfigEpoch >= senderConfig.LocalNodeConfigEpoch). The stale owner's epoch keeps advancing for unrelated reasons, so the true owner — whose epoch was frozen when it claimed the slot — can never win the comparison. The existing comment on the reset branch describes exactly this trap: "the sender will falsely remain the owner and its epoch will be greater than that of the new owner and the new owner will not be able to claim the slot without outside intervention." The reset is the mechanism meant to prevent that, and it is being thrown away.The reset branch (
ed46945fd71, 2024-11-26) predates theupdatedflag (e9d79068491, 2025-01-09, #905), which instrumented only the ownership-assignment path and left the pre-existing reset branch unwired.Description of Change
libs/cluster/Server/ClusterConfig.cs— inMergeSlotMap, setupdated = truein the stale-ownership reset branch, and change the ownership-assignment line fromupdated =toupdated |=. Both are required: the first makes a reset-only merge survive, the second stops a later no-change slot from clobbering it.test/cluster/Garnet.test.cluster/ClusterConfigTests.cs— addedClusterConfigMergeSlotMapRetainsStaleOwnershipResetTest. The receiver believes the sender owns two slots; the sender claims only one and attributes the other to a third node, so the reset is the merge's only effect. Fails without the fix, passes with it.No public API, configuration, or wire-format change, and no behavior change for merges that already had another effect.
What NOT to Do (for future agents)
continues before reaching theupdatedassignment, and sinceSlotState.OFFLINEis0, unclaimed slots are skipped by the!= STABLEguard. The sender must not claim the stale slot.Validation
Garnet.test.clusterpasses on net8.0 Debug (155/155) and net10.0 Release (138 passed, 7 skipped); Release build is warning-clean anddotnet format Garnet.slnx --verify-no-changesis clean.Also verified end-to-end on a 100-node cluster by migrating slots off half the primaries under a live write workload, then collecting the slot map from every node individually. Without the fix the nodes settled into 27 distinct views of slot ownership — stable rather than transient, with no self-recovery — and migrations into any node holding a stale view failed permanently with
ERR Slot <n> is not owned by <node>fromTryPrepareImport, since the target validates the source against its own map. With the fix, all 100 nodes converged to a single view.Issues Fixed
No existing issue — filed directly as a PR. Happy to open a tracking issue if preferred.