Fix replica left as a sub-replica after a failover

Project: redis/redis | Language: C | Concepts: consistency-models, distributed-systems, error-handling, testing-strategy | Difficulty: senior
Link: https://github.com/redis/redis/pull/15530

The problem

After a manual failover in a Redis cluster, a replica could end up attached to another replica (the old master, now demoted), instead of directly following the new master. This could leave the cluster in a state where a replica is chained under a non-master, and the system would not recover from this state automatically.

Why it's non-trivial

A naive fix might try to re-point replicas whenever a failover occurs, but the actual bug is triggered by the order in which cluster gossip packets arrive. If a replica learns about the failover from the demoted master before hearing from the new master, the slot difference that normally triggers reconfiguration is already gone. The logic that usually corrects such chains (clusterUpdateSlotsConfigWith()) is never called, and the replica remains stuck as a "sub-replica" indefinitely.

The approach

The fix adds a safeguard in clusterProcessPacket() to detect and repair sub-replica chains after any role change, not just when slot differences are observed.

Key code snippets and their purpose:

Early return for stale packets
Prevents further processing of a stale packet after demotion, avoiding inconsistent state:

                       /* Ignore the rest of this stale packet to prevent it from reverting

                        * newer topology changes and creating an invalid replication chain. */

```
                       return 1;
```

Sub-replica safeguard
After handling possible demotions, this block checks if the current node is a replica of another replica (i.e., a sub-replica). If so, it reconfigures the node to follow the grandmaster directly:

   /* Safeguard against sub-replicas: our master may have just been demoted

    * above, or by an earlier packet. We cannot leave this to the same check

    * in clusterUpdateSlotsConfigWith(), which is only reached when the

    * sender claims slots we attribute to someone else: once the demotion

    * handling above hands them to the new master, that difference is gone.

```
    *
```

    * This runs on every packet rather than only on the demotion itself, so

    * it also recovers a node that learned of the demotion before it knew

    * the new master. Only follow a grandmaster we believe is a master, so

    * that a chain that has not settled yet cannot make us resync from a

```
    * node that owns no slots. */
```

   clusterNode *grandmaster = nodeIsSlave(myself) && myself->slaveof ?

                              myself->slaveof->slaveof : NULL;

   if (grandmaster && clusterNodeIsMaster(grandmaster) && grandmaster != myself &&

       !(server.cluster_module_flags & CLUSTER_MODULE_FLAG_NO_REDIRECTION))

```
   {
```
```
       serverLog(LL_NOTICE,
```

                 "I'm a sub-replica! Reconfiguring myself as a replica of grandmaster %.40s (%s)",

                 grandmaster->name, grandmaster->human_nodename);

```
       clusterSetMaster(grandmaster);
```

       /* Save the new config and broadcast to the other nodes. */

       clusterDoBeforeSleep(CLUSTER_TODO_SAVE_CONFIG|

                            CLUSTER_TODO_UPDATE_STATE|

                            CLUSTER_TODO_FSYNC_CONFIG|

                            CLUSTER_TODO_BROADCAST_PONG);

```
   }
```

Regression test
A deterministic test was added to simulate the problematic packet ordering and verify that the sub-replica safeguard correctly repairs the chain:

test "Replica follows a manual failover it learns about from the demoted master" {
```
   ...
```

   # Reproduce the ordering seen in the wild, where the observer processes

   # the old master's demotion before the new master's slot claim.

   R $promoted CLUSTER FORGET $observer_id

   R $promoted DEBUG DROP-CLUSTER-PACKET-FILTER $CLUSTERMSG_TYPE_PING

```
   R $promoted CLUSTER FAILOVER
```
```
   ...
```

   wait_for_log_messages -$observer {"*I'm a sub-replica!*"} $loglines 100 100

```
   ...
```
```
   wait_for_condition 100 100 {
```

       [dict get [cluster_get_myself $observer] slaveof] eq $promoted_id

```
   } else {
```

       fail "observer is still a replica of the old master (sub-replica)"

```
   }
```

From the review thread

ShooterIT:
        if (grandmaster && clusterNodeIsMaster(grandmaster) && grandmaster != myself &&
            !(server.cluster_module_flags & CLUSTER_MODULE_FLAG_NO_REDIRECTION))
grandmaster != myself->slaveof is not necessary, if clusterNodeIsMaster(grandmaster) is true, it means myself->slaveof->slaveof is master, so myself->slaveof will be a replica, so they should be different.

could myself->numslots == 0 happen if myself is a replica? if this happens, it should a bug, right?

shun-lee:
Thanks, applied as suggested.

On the question — agreed, a replica owning slots shouldn't happen in normal operation, so if it does it would be a bug state. In that state this block would hit the serverAssert(myself->numslots == 0) in clusterSetMaster(), but the existing safeguard in clusterUpdateSlotsConfigWith() has the same exposure, so dropping the check here is consistent with current behavior. Seems fine to handle that state separately in a follow-up if needed.

Takeaways

Always consider edge cases in distributed systems where message ordering or loss can produce persistent, invalid topology states.
Safeguards that correct topology should not rely solely on slot differences or other state that can be "consumed" by earlier events; checks should be placed after all possible role transitions.
Deterministic regression tests that simulate real-world failure orderings are essential for validating fixes to distributed system bugs.