From e2e751b180385a36052ea2a600aabc42d01fdc7b Mon Sep 17 00:00:00 2001 From: FZambia Date: Mon, 20 Jul 2026 17:05:29 +0300 Subject: [PATCH] fix(sentinel): do not close a connection still referenced by mConn/rConn _switchTarget reuses the live mConn/rConn as target when Sentinel reports the address the client already holds. On the ROLE failure paths it then called target.Close() and returned without swapping, leaving mConn/rConn pointing at a closed mux. Every subsequent command fails with ErrClosing without attempting to dial, and nothing replaces it because the swap is only reached on success. Reachable in ordinary operation: a just-demoted master answers ROLE with 'slave' -> errNotMaster -> the live connection is closed out from under every caller. Observed as minutes-long total outages against a healthy Redis, with the client holding zero TCP connections to any node. Close only connections this call dialled. --- sentinel.go | 31 ++++++++++++++++++-- sentinel_switchtarget_test.go | 54 +++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 3 deletions(-) create mode 100644 sentinel_switchtarget_test.go diff --git a/sentinel.go b/sentinel.go index ca0f3a3e..b3f605d4 100644 --- a/sentinel.go +++ b/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 } diff --git a/sentinel_switchtarget_test.go b/sentinel_switchtarget_test.go new file mode 100644 index 00000000..327b3b5b --- /dev/null +++ b/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()) + } +}