Test: forbid failover on empty sub-replicas in replica-migration sub-replica test

[madolson]

Sub-replica reports zero repl offset and rank, and fails to win election requires that node 4, the only replica holding data, wins the election after primary 0 dies, but the server never promised that. The rank and zero-offset delays in clusterHandleReplicaFailover only make an empty replica less likely to win, and with the test's cluster-node-timeout 1000 the whole ordering margin is about 600 ms of wall clock on node 4. Stall node 4 for 2 s across the election window and the empty node 3 or node 7 wins instead, 5 times out of 5, which is what the sanitizer runner produced in CI. This sets cluster-replica-no-failover on nodes 3 and 7 so node 4 winning becomes an invariant instead of a timing margin.

Fixes the shutdown variant tracked upstream as valkey-io/valkey#4672 and, because the change is in the shared test_sub_replica proc, the sigstop variant tracked as valkey-io/valkey#4265. Filed here as #43.

Details

Problem

tests/unit/cluster/replica-migration.tcl:294-303 asserts that after primary 0 is killed, node 4 is the new primary and nodes 3 and 7 are its replicas. Node 4 is the only replica of primary 0 with a non-zero replication offset, so it is the only node that takes the immediate-election path at src/cluster_legacy.c:5992-5997, because myselfIsBestRankedReplica() at src/cluster_legacy.c:165 requires getNodeReplicationOffset(myself) != 0 at src/cluster_legacy.c:167.

Nodes 3 and 7 are penalized but not excluded. src/cluster_legacy.c:5851:

    long long delay = min(server.cluster_node_timeout / 30, 500);

With cluster-node-timeout 1000 (tests/unit/cluster/replica-migration.tcl:356), delay = 33, and an empty rank-1 replica schedules its election at:

5901:        server.cluster->failover_auth_time = now + delay + (delay ? random() % delay : 0);
5909:        server.cluster->failover_auth_time += server.cluster->failover_auth_rank * (delay * 2);
5914:        if (getNodeReplicationOffset(myself) == 0) {
5915:            server.cluster->failover_auth_time += 500;

33 + rand(33) + 66 + 500 = 600 to 630 ms. That is the entire budget. The source itself describes this as probability shaping, not exclusion:

5910:        /* If this is a newly added replica, there is a risk it doesn't know
5911:         * about other replicas yet, so it may think it's the best replica even
5912:         * if there are others with a better replication offsets. Add an extra
5913:         * delay to make it less likely to will win the failover. */

In the CI run (https://github.com/valkey-io/valkey/actions/runs/34727724387/job/103644755446), node 4 started its election 541 ms ahead of node 3 but then logged nothing between 00:26:00.640 and 00:26:01.334, six missed 100 ms cron ticks, so its FAILOVER_AUTH_REQUEST was not flushed until node 3 had already collected both votes.

Reproduction

No sanitizer and no CI machine needed. Stall node 4's event loop across the election window with DEBUG SLEEP from a deferring client, inserted immediately before the if {$type == "shutdown"} block:

        # REPRO-ONLY: stall node 4 (the only replica with data) across the
        # election window, the same way the sanitizer runner did in CI.
        if {[info exists ::env(REPRO43_STALL_MS)]} {
            set stall_rd [valkey_deferring_client -4]
            $stall_rd debug sleep [expr {$::env(REPRO43_STALL_MS) / 1000.0}]
        }

REPRO43_STALL_MS=2000 ./runtest \
  --single unit/cluster/replica-migration \
  --only "Sub-replica reports zero repl offset and rank, and fails to win election - shutdown" \
  --loops 5

2000 ms is the knob. REPRO43_STALL_MS=1000 is 0/5: the stall has to outlive FAIL detection, which cannot happen before cluster_node_timeout = 1000 ms has elapsed, and then node 3's 600 ms delay on top of that.

Before, 5/5 fail:

s -4 role: slave
s -3 role: master
s -7 role: slave
[err]: Sub-replica reports zero repl offset and rank, and fails to win election - shutdown in tests/unit/cluster/replica-migration.tcl
Failover does not happened
s -4 role: slave
s -3 role: master
s -7 role: slave
[err]: Sub-replica reports zero repl offset and rank, and fails to win election - shutdown in tests/unit/cluster/replica-migration.tcl
Failover does not happened
s -4 role: slave
s -3 role: master
s -7 role: slave
[err]: Sub-replica reports zero repl offset and rank, and fails to win election - shutdown in tests/unit/cluster/replica-migration.tcl
Failover does not happened
s -4 role: slave
s -3 role: slave
s -7 role: master
[err]: Sub-replica reports zero repl offset and rank, and fails to win election - shutdown in tests/unit/cluster/replica-migration.tcl
Failover does not happened
s -4 role: slave
s -3 role: master
s -7 role: slave
[err]: Sub-replica reports zero repl offset and rank, and fails to win election - shutdown in tests/unit/cluster/replica-migration.tcl
Failover does not happened

Test Summary: 55 passed, 5 failed

Loop 4 is new information relative to the CI report: node 7, the empty sub-replica, won. Any zero-offset replica of the dead primary can beat node 4, not just node 3.

After, same 2000 ms injection still in place, 5/5 pass:

[ok]: Sub-replica reports zero repl offset and rank, and fails to win election - shutdown (2799 ms)
[ok]: Sub-replica reports zero repl offset and rank, and fails to win election - shutdown (2806 ms)
[ok]: Sub-replica reports zero repl offset and rank, and fails to win election - shutdown (3235 ms)
[ok]: Sub-replica reports zero repl offset and rank, and fails to win election - shutdown (3243 ms)
[ok]: Sub-replica reports zero repl offset and rank, and fails to win election - shutdown (3254 ms)

Test Summary: 60 passed, 0 failed

The injection is not part of this PR.

Fix

clusterHandleReplicaFailover returns immediately when the config is set, at src/cluster_legacy.c:5864, so node 4 is the only node that can ever win the epoch. The two if {[count_log_message -3 "Start of election"] != 0} guards at tests/unit/cluster/replica-migration.tcl:308-314 were already written to tolerate no election happening, so they remain valid; they simply no longer fire.

Decisions a reviewer has to make

1. Is this a product bug? Proposal: no. Promoting an empty replica loses writes and the default configuration permits it. Election safety held in every reproduction: exactly one node won epoch 10, the rest were denied by the one-vote-per-epoch rule at src/cluster_legacy.c:5841-5842. The existence of CLUSTER_REPLICA_NO_FAILOVER_IF_EMPTY on unstable (src/cluster_legacy.c:6266, added by 05a9c29c6, PR #4425) is the acknowledgement that the default path allows exactly this.

2. yes or if-empty? Proposal: yes on this base, if-empty once rebased. This branch is off agents/unstable (596da45a0), which is 66 commits behind upstream/unstable and does not contain 05a9c29c6, so the config there is still a plain bool at src/config.c:3377. On upstream/unstable it is an enum at src/config.c:3539 and if-empty is the more precise value, since it blocks only the zero-offset case. Happy to switch if this is retargeted.

3. Why not raise cluster-node-timeout instead? delay at src/cluster_legacy.c:5851 scales linearly with the timeout, so a proportionally larger stall reinstates the same inversion. It keeps the assertion timing-based and lengthens a test that fa62dce94 (PR #4006) deliberately shortened.

4. Why not loosen the assertion to "some replica won"? The rest of the test would still fail: R 3 get key_977613 at tests/unit/cluster/replica-migration.tcl:338-342 needs the 10 KiB value that only node 4 holds, and it is genuinely lost when an empty replica is promoted.

Out of scope

test_migrated_replica at tests/unit/cluster/replica-migration.tcl:22 and test_nonempty_replica at line 140 carry the same [s -4 role] eq {master} assertion under the same cluster-node-timeout 1000. valkey-io/valkey#3992 is the open upstream issue for the first. The same two lines would fix them; not touched here.

Testing

The before run above is the proof. ./runtest --single unit/cluster/replica-migration with no injection is clean.

This was generated by AI but verified, with love, by a human.