fix(sentinel): do not close a connection still referenced by mConn/rConn

sentinel.go

@@ -441,6 +441,18 @@ func (c *sentinelClient) _switchTarget(addr string, isMaster bool) (err error) {
 	var (
 		target conn
 		opt    *ClientOption
+		// reused reports that target is the connection still referenced by
+		// mConn/rConn rather than one we just dialed. Closing a reused
+		// connection on a failure path leaves mConn/rConn pointing at a CLOSED
+		// mux: every subsequent command then fails with ErrClosing without even
+		// attempting to dial, and nothing ever replaces it, because the swap
+		// below is only reached on success.
+		//
+		// This is reachable in ordinary operation: when Sentinel reports the
+		// same address the client already holds, target is reused; a
+		// just-demoted master answers ROLE with "slave" -> errNotMaster -> the
+		// live mConn is closed out from under every caller.
+		reused bool
 	)
 
 	if isMaster {
@@ -449,6 +461,8 @@ func (c *sentinelClient) _switchTarget(addr string, isMaster bool) (err error) {
 			target = c.mConn.Load().(conn)
 			if target.Error() != nil {
 				target = nil
+			} else {
+				reused = true
 			}
 		}
 	} else {
@@ -457,6 +471,8 @@ func (c *sentinelClient) _switchTarget(addr string, isMaster bool) (err error) {
 			target = c.rConn.Load().(conn)
 			if target.Error() != nil {
 				target = nil
+			} else {
+				reused = true
 			}
 		}
 	}
@@ -468,15 +484,24 @@ func (c *sentinelClient) _switchTarget(addr string, isMaster bool) (err error) {
 		}
 	}
 
+	// closeIfOwned closes only a connection this call created. A reused one is
+	// still referenced by mConn/rConn and must outlive the failure; the caller
+	// retries the refresh and replaces it once a real master is found.
+	closeIfOwned := func() {
+		if !reused {
+			target.Close()
+		}
+	}
+
 	resp, err := target.Do(context.Background(), cmds.RoleCmd).ToArray()
 	if err != nil {
-		target.Close()
+		closeIfOwned()
 		return err
 	}
 
 	if isMaster {
 		if resp[0].string() != "master" {
-			target.Close()
+			closeIfOwned()
 			return errNotMaster
 		}
 
@@ -489,7 +514,7 @@ func (c *sentinelClient) _switchTarget(addr string, isMaster bool) (err error) {
 		}
 	} else {
 		if resp[0].string() != "slave" {
-			target.Close()
+			closeIfOwned()
 			return errNotSlave
 		}
 

sentinel_switchtarget_test.go

@@ -0,0 +1,54 @@
+package rueidis
+
+import (
+	"testing"
+)
+
+// TestSwitchTargetDoesNotCloseReusedConn pins the invariant that a connection
+// still referenced by mConn must never be closed on a failure path.
+//
+// Reachable in ordinary operation: Sentinel reports the address the client
+// already holds, so _switchTarget reuses the live mConn as `target`; the node
+// has just been demoted, so ROLE answers "slave"; the old code then called
+// target.Close() and returned errNotMaster WITHOUT swapping. mConn was left
+// pointing at a closed mux, so every later command failed with ErrClosing and
+// never even attempted to dial — observed in production as minutes of total
+// outage against a perfectly healthy Redis.
+func TestSwitchTargetDoesNotCloseReusedConn(t *testing.T) {
+	defer ShouldNotLeak(SetupLeakDetection())
+
+	closed := false
+	// Reports "slave": the demoted-master case.
+	demoted := &mockConn{
+		DoFn: func(cmd Completed) RedisResult {
+			return RedisResult{val: slicemsg('*', []RedisMessage{strmsg('+', "slave")})}
+		},
+		CloseFn: func() { closed = true },
+		ErrorFn: func() error {
+			if closed {
+				return ErrClosing
+			}
+			return nil
+		},
+	}
+
+	c := &sentinelClient{
+		mOpt:   &ClientOption{},
+		connFn: func(dst string, opt *ClientOption) conn { return demoted },
+	}
+	c.mConn.Store(conn(demoted))
+	c.mAddr.Store("127.0.0.1:6379")
+
+	// Same address the client already holds -> the reuse path.
+	if err := c._switchTarget("127.0.0.1:6379", true); err != errNotMaster {
+		t.Fatalf("expected errNotMaster, got %v", err)
+	}
+
+	if closed {
+		t.Fatal("_switchTarget closed the connection still referenced by mConn — " +
+			"mConn now points at a closed mux and every command will fail with ErrClosing")
+	}
+	if got := c.mConn.Load().(conn); got.Error() != nil {
+		t.Fatalf("mConn must remain usable after a failed switch, got error %v", got.Error())
+	}
+}