fix(sentinel): do not close a connection still referenced by mConn/rConn - #1015
Merged
rueian merged 1 commit intoJul 28, 2026
Merged
Conversation
_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.
Contributor
Author
|
@rueian hi! To give you some background for the series of pull requests - those are things I noticed when I was trying to reproduce an issue with PUB/SUB stopped working in Sentinel setup on GKE (using Redis 8.8.0 via Bitnami redis chart 27.0.15). So mostly all the issues I am targeting were caught when I added some chaos into the setup. Clean graceful failover worked correctly, but under some conditions, like killing a pod that manages both Redis and Sentinel instances (Bitnami chart default) – several issues happened. I also plan to open a couple more PRs that fix the failover issues – but they are better to land on top of the initial three. |
rueian
approved these changes
Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On a failover
_switchTargetcould close the livemConn/rConnand leave the client pointing at a closed mux. This closes only connections the call itself dialed.Impact
ErrClosingwithout even attempting to dial, and the client holds no usable connection to any node.refreshRetry, whichswitchTargetRetryspawns on failure. That normally re-dials and repairsmConn— so the usual shape of this is a total outage lasting as long as it takes Sentinel's view to settle, not a permanent one.refresh()hands the joiningrefreshRetrynil,refreshRetrytreats that as success and exits, and nothing is left to replace the closedmConn. That is the singleflight bug fixed in fix(singleflight): give waiting callers the real error of the run they waited on #1014, which is why the two belong together.Root cause
_switchTargetreuses the livemConn/rConnastargetwhen Sentinel reports the address the client already holds. On the ROLE failure paths it then calledtarget.Close()and returned without swapping, somConn/rConnreferenced a closed mux and nothing replaced it (the swap is only reached on success). This is reachable in normal operation: a just-demoted master answersROLEwithslave→errNotMaster→ the live connection is closed out from under every caller.Fix
Close only connections this call dialed. A reused connection is still referenced by
mConn/rConnand must outlive the failure; the caller retries the refresh and replaces it once a real master is found.Note the trade-off: keeping a demoted node as
mConnmeans writes fail-READONLYuntil refresh replaces it. That is strictly better thanErrClosingon every command including reads, and it is transient either way.Tests
TestSwitchTargetDoesNotCloseReusedConn— fails against the previous code (closes the connection still referenced bymConn), passes with the fix.Note
High Risk
Touches Sentinel connection lifecycle during failover; the prior behavior caused prolonged total client outage (
ErrClosingon every command). The fix is narrowly scoped but sits on critical Redis connectivity paths.Overview
Fixes a Sentinel failover bug where
_switchTargetcould close the livemConn/rConnwhen ROLE validation failed but the target was the reused connection for an address the client already held (e.g. demoted master returnsslave→errNotMaster).The change tracks whether
targetis reused vs newly dialed and usescloseIfOwnedon ROLE/command errors and role mismatches so only connections created in this call are closed; reused connections stay open until a successful swap or a later refresh replaces them.Adds
TestSwitchTargetDoesNotCloseReusedConnto lock in thatmConnmust not be closed on that failure path.Reviewed by Cursor Bugbot for commit e2e751b. Bugbot is set up for automated code reviews on this repo. Configure here.