fix(test): make waitForSentinelClusterStable robust to disconnected r… (#3830)

* fix(test): make waitForSentinelClusterStable robust to disconnected replicas

CI on master sporadically fails the universal_test.go spec
"should connect to failover servers on slaves when readonly Options is ok"
with `Expected master to equal slave`. The flake reproduces only when the
random spec order puts the sentinel-failover Describe block before the
universal_test spec.

Root cause is in waitForSentinelClusterStable's post-conditions, not in
production code:

  1. It only checks `len(replicas) >= 2`. Production
     (parseReplicaAddrs in sentinel.go) filters out replicas flagged
     `s_down`, `o_down`, or `disconnected`. Post-failover, replicas
     often linger as `disconnected` for several seconds after Sentinel
     agrees on the master/replica counts, so the stabilization check
     can pass while production sees zero usable replicas.

  2. It then waits a fixed 10s. With no link between that sleep and
     the actual precondition (a FailoverClient(ReadOnly:true) finding
     a routable replica), the test gambles that 10s is enough on every
     CI host. When it isn't, RandomReplicaAddr silently returns the
     master and the spec asserts on the wrong role.

Two changes:

  - Add countUsableReplicas, mirroring parseReplicaAddrs's filter, and
    require each sentinel to report at least 2 usable (non-down,
    non-disconnected) replicas with consistent counts across sentinels.

  - Replace the 10s sleep with a bounded Eventually that opens the same
    kind of client the failing spec uses (UniversalClient + MasterName +
    ReadOnly:true) and confirms ROLE returns "slave". This is the
    deterministic equivalent of waiting "long enough" — it returns as
    soon as the actual precondition holds, and bounds at 30s if the
    cluster never stabilizes.

No production code changes; the flake is entirely in test harness
post-conditions. The new probe uses only public API and existing
package-level fixtures (sentinelName, sentinelAddrs).

* fix(test): harden sentinel ReadOnly spec against silent master fallback

CI on master sporadically fails the universal_test.go spec
"should connect to failover servers on slaves when readonly Options is ok"
with `Expected master to equal slave`. Two random-spec-ordering bug
paths combine to produce this:

  1. sentinel_test.go has three failover-causing specs across three
     Describe blocks ("Sentinel", "NewFailoverClusterClient", and
     "Sentinel Failover with Conns"). Only the third runs Ordered/Serial
     with an AfterAll that calls waitForSentinelClusterStable. The
     first two can land right before the universal_test spec under any
     random spec ordering and leave the cluster mid-recovery.

  2. FailoverClient(ReadOnly:true) routes via RandomReplicaAddr, which
     silently falls back to the master when Sentinel reports zero
     usable replicas. parseReplicaAddrs in sentinel.go filters out
     replicas flagged s_down, o_down, or disconnected — and freshly
     promoted/demoted replicas often linger as "disconnected" for
     several seconds after Sentinel agrees on master/replica counts.

Two test-only changes (no production code touched):

  a) waitForSentinelClusterStable: previously only checked
     len(replicas) >= 2, which can pass while every replica is
     filtered out by production code. Now mirrors parseReplicaAddrs's
     filter via countUsableReplicas and requires each sentinel to
     report >= 2 usable replicas with consistent counts across
     sentinels. The fixed 10s sleep is replaced with a bounded
     Eventually that opens the same UniversalClient(ReadOnly:true)
     the failing spec uses and confirms ROLE returns "slave" — the
     deterministic equivalent of "wait long enough".

  b) universal_test.go spec: defense in depth. Since the failover
     specs in (1) don't all share an AfterAll, the spec itself
     retries client creation + ROLE in a bounded Eventually. Each
     iteration closes the old client and creates a new one so
     RandomReplicaAddr is re-evaluated against current Sentinel
     state. Bounded at 30s so a genuinely broken setup still fails.

The (a) fix keeps the post-failover state clean for any downstream
test; the (b) fix keeps this specific spec correct regardless of
ordering. Together they make the spec deterministic across all 1414!
possible random spec orderings.

* fix(test): make ReadOnly SET assertion atomic with the ROLE probe

Previous attempt fixed the ROLE check ("Expected master to equal slave")
by retrying client creation inside Eventually until ROLE returned
"slave". CI surfaced a second failure mode in the same spec:

  [FAILED] Expected an error to have occurred. Got: <nil>: nil
  universal_test.go:110

The spec's intent was: ReadOnly client lands on a replica, and a write
against that replica returns READONLY. The new failure means the write
succeeded — there's a second path back to the master:

  SET on replica → READONLY error
   → shouldRetry returns true for READONLY (error.go)
   → conn discarded, retried via masterReplicaDialer
   → RandomReplicaAddr called again, may silently fall back to master
     when no usable replica is reported (same root cause as the original
     flake — Sentinel sees 2 replicas but both are "disconnected")
   → SET hits the master, succeeds, err is nil
   → Expect(err).To(HaveOccurred()) fails

Two changes to make the spec atomic and bypass-proof:

  - Set MaxRetries: -1 on the test client. A READONLY reply now
    surfaces as the first/only SET error instead of triggering a
    retry-dial that may pick the master. This isolates what the test
    is actually asserting: a single round-trip to whatever node the
    dialer picked.

  - Move the SET assertion inside the Eventually closure together
    with the ROLE check. If either step lands on the master (because
    RandomReplicaAddr fell back at dial time), the closure returns an
    error, Eventually closes the client and retries with a fresh one
    so the dialer re-queries Sentinel. The full assertion is therefore
    atomic — both ROLE and SET observe the same dial.

Error messages from the closure name the precise failure mode so a
genuinely broken setup (timeout after 30s) reports something more
useful than "expected slave, got master".

No production code changes; the retry-on-READONLY behaviour is correct
for normal failover scenarios and stays untouched.

---------

Co-authored-by: ofekshenawa <[email protected]>

sentinel_test.go

@@ -695,6 +695,31 @@ func compareSlices(t *testing.T, a, b []string, name string) {
 	}
 }
 
+// countUsableReplicas mirrors the filter applied by parseReplicaAddrs in
+// sentinel.go: a replica is usable only when it is not s_down, o_down, or
+// disconnected. Plain len() on Sentinel's Replicas reply is not enough —
+// post-failover, replicas often linger with the "disconnected" flag for
+// several seconds after Sentinel agrees on master/replica counts, and a
+// FailoverClient(ReadOnly:true) created in that window will see an empty
+// usable list and silently fall back to the master (see RandomReplicaAddr
+// in sentinel.go).
+func countUsableReplicas(replicas []map[string]string) int {
+	usable := 0
+	for _, node := range replicas {
+		down := false
+		for _, flag := range strings.Split(node["flags"], ",") {
+			switch flag {
+			case "s_down", "o_down", "disconnected":
+				down = true
+			}
+		}
+		if !down && node["ip"] != "" && node["port"] != "" {
+			usable++
+		}
+	}
+	return usable
+}
+
 func waitForSentinelClusterStable() {
 	sentinel1 := redis.NewSentinelClient(&redis.Options{
 		Addr: ":" + sentinelPort1,
@@ -727,8 +752,11 @@ func waitForSentinelClusterStable() {
 			return false
 		}
 
-		// Also check replicas are consistent and we have at least 2 slaves
-		// (important for ReadOnly tests - if no slaves found, RandomReplicaAddr falls back to master)
+		// Each sentinel must report at least 2 usable (non-down,
+		// non-disconnected) replicas. Just counting replicas isn't enough:
+		// production code filters disconnected replicas out, so a sentinel
+		// reporting "2 replicas, both disconnected" yields 0 usable nodes
+		// and triggers the silent master fallback.
 		replicas1, err1 := sentinel1.Replicas(ctx, sentinelName).Result()
 		replicas2, err2 := sentinel2.Replicas(ctx, sentinelName).Result()
 		replicas3, err3 := sentinel3.Replicas(ctx, sentinelName).Result()
@@ -736,13 +764,39 @@ func waitForSentinelClusterStable() {
 		if err1 != nil || err2 != nil || err3 != nil {
 			return false
 		}
-		if len(replicas1) < 2 || len(replicas2) < 2 || len(replicas3) < 2 {
+		u1, u2, u3 := countUsableReplicas(replicas1), countUsableReplicas(replicas2), countUsableReplicas(replicas3)
+		if u1 < 2 || u2 < 2 || u3 < 2 {
 			return false
 		}
-		return len(replicas1) == len(replicas2) && len(replicas2) == len(replicas3)
+		return u1 == u2 && u2 == u3
 	}, "30s", "1s").Should(BeTrue())
 
-	time.Sleep(10 * time.Second)
+	// End-to-end probe: open the same kind of client the post-failover
+	// ReadOnly spec uses and confirm it actually routes to a replica.
+	// This is the deterministic equivalent of the previous 10-second
+	// time.Sleep — succeeds as soon as the precondition is met, instead of
+	// hoping 10 seconds is long enough for replicas to come out of the
+	// "disconnected" state on every sentinel.
+	Eventually(func() bool {
+		c := redis.NewUniversalClient(&redis.UniversalOptions{
+			MasterName: sentinelName,
+			Addrs:      sentinelAddrs,
+			ReadOnly:   true,
+		})
+		defer c.Close()
+		if err := c.Ping(ctx).Err(); err != nil {
+			return false
+		}
+		role, err := c.Do(ctx, "ROLE").Result()
+		if err != nil {
+			return false
+		}
+		roleSlice, ok := role.([]interface{})
+		if !ok || len(roleSlice) == 0 {
+			return false
+		}
+		return roleSlice[0] == "slave"
+	}, "30s", "500ms").Should(BeTrue())
 }
 
 var _ = Describe("Sentinel Failover with Conns", Serial, Ordered, func() {

universal_test.go

@@ -1,6 +1,8 @@
 package redis_test
 
 import (
+	"fmt"
+
 	. "github.com/bsm/ginkgo/v2"
 	. "github.com/bsm/gomega"
 
@@ -71,23 +73,64 @@ var _ = Describe("UniversalClient", Serial, func() {
 	})
 
 	It("should connect to failover servers on slaves when readonly Options is ok", Label("NonRedisEnterprise"), func() {
-		client = redis.NewUniversalClient(&redis.UniversalOptions{
-			MasterName: sentinelName,
-			Addrs:      sentinelAddrs,
-			ReadOnly:   true,
-		})
-		Expect(client.Ping(ctx).Err()).NotTo(HaveOccurred())
-
-		roleCmd := client.Do(ctx, "ROLE")
-		role, err := roleCmd.Result()
-		Expect(err).NotTo(HaveOccurred())
-
-		roleSlice, ok := role.([]interface{})
-		Expect(ok).To(BeTrue())
-		Expect(roleSlice[0]).To(Equal("slave"))
-
-		err = client.Set(ctx, "somekey", "somevalue", 0).Err()
-		Expect(err).To(HaveOccurred())
+		// Two failure modes combine to make this spec flaky under random
+		// spec ordering when any prior spec triggered a failover:
+		//
+		//  1. FailoverClient(ReadOnly:true) routes via RandomReplicaAddr,
+		//     which silently falls back to the master when Sentinel
+		//     reports zero usable replicas (post-failover replicas often
+		//     linger as "disconnected" for several seconds). Observed
+		//     symptom: ROLE returns "master".
+		//
+		//  2. The SET assertion is similarly bypassed by go-redis's
+		//     retry-on-READONLY logic: SET against a replica returns
+		//     READONLY, shouldRetry returns true, the conn is recycled,
+		//     and the next dial goes through masterReplicaDialer again
+		//     — which can fall back to master via the same path as (1).
+		//     The SET then succeeds against the master. Observed symptom:
+		//     ROLE returns "slave" but SET unexpectedly succeeds.
+		//
+		// Defenses:
+		//
+		//  - Disable client retries (MaxRetries: -1) so a READONLY reply
+		//    surfaces as the first/only SET error instead of triggering
+		//    a fresh dial that may land on the master.
+		//
+		//  - Run the full assertion (connect → ROLE → SET) atomically
+		//    inside a bounded Eventually. If any step lands on the
+		//    master (because RandomReplicaAddr fell back), close and
+		//    retry with a fresh client so RandomReplicaAddr is
+		//    re-evaluated against current Sentinel state.
+		Eventually(func() error {
+			if client != nil {
+				_ = client.Close()
+			}
+			client = redis.NewUniversalClient(&redis.UniversalOptions{
+				MasterName: sentinelName,
+				Addrs:      sentinelAddrs,
+				ReadOnly:   true,
+				MaxRetries: -1,
+			})
+			if err := client.Ping(ctx).Err(); err != nil {
+				return fmt.Errorf("ping: %w", err)
+			}
+			role, err := client.Do(ctx, "ROLE").Result()
+			if err != nil {
+				return fmt.Errorf("ROLE: %w", err)
+			}
+			roleSlice, ok := role.([]interface{})
+			if !ok || len(roleSlice) == 0 {
+				return fmt.Errorf("ROLE returned unexpected shape: %v", role)
+			}
+			firstRole, _ := roleSlice[0].(string)
+			if firstRole != "slave" {
+				return fmt.Errorf("ROLE = %q, want %q (RandomReplicaAddr fell back to master)", firstRole, "slave")
+			}
+			if setErr := client.Set(ctx, "somekey", "somevalue", 0).Err(); setErr == nil {
+				return fmt.Errorf("SET on replica unexpectedly succeeded (likely landed on master)")
+			}
+			return nil
+		}, "30s", "500ms").Should(Succeed())
 	})
 
 	It("should connect to clusters if IsClusterMode is set even if only a single address is provided", Label("NonRedisEnterprise"), func() {