[FumingPower3925]

driver/redis: two Close races in SentinelClient and ClusterClient, both real (not flakes), found by the review of #1140. Refs #887 #1104

Method: one commit pair per defect so each can be dropped. Commit 1 of each pair adds the test (and a behaviour-neutral seam) and FAILS; commit 2 is the fix. Tests use the package's fake Redis servers, no tag, so they also run in the "Portable fallback" jobs.

	commits
A. SentinelClient: handleSwitchMaster after Close	ac8f75a tests + seam, ff1f376 fix (the retry dial and the install)
B. ClusterClient: refresh / redirect after Close	bfc657b tests + seam, 6a98787 fix
review round: the first dial, and the cluster test assertions	b784352 tests (the entry test is red, the cluster tests are green and are mutant-checked) + the installNode split, 784d60a fix (the entry check)

A. handleSwitchMaster and Close (sentinel.go)

Two defects in one function:

1. After a failed dial the retry waits in select { case <-time.After(delay): case <-s.closeCh: return }. When Close and the timer are both ready, Go picks at random, so the loop dials the master again after Close (at most twice more, maxRetries is 3): a live connection, ROLE round trip included. The same shape as driver/internal/eventloop: RegisterConnWait stops at a done ctx even when its pause has ended too #1140.
2. Worse, and with no select at all: after the loop s.client = newClient had no look at s.closed. dialMaster runs under its own DialTimeout, not closeCh, so a Close that lands while it is in flight (it clears s.client, closes the old one, and returns without waiting for the goroutine) is followed by the install of a live *Client on a closed SentinelClient. Nothing closes it: getClient answers ErrClosed, so it is never used either. A leaked client, its socket and its engine registration, until the process exits (a +switch-master during shutdown).

Fix (commit ff1f376, which covers the dial after a retry pause and the install, not the first dial; that is 784d60a below): after the pause, if s.closed.Load() { return }, so a closed client wins a tie; and under s.mu, before the store, if s.closed.Load() unlock, close newClient and return. Close sets the flag before it takes s.mu, so either handleSwitchMaster sees the flag, or Close takes s.mu after the store and closes the stored client.

Third defect, found by the review of this PR: handleSwitchMaster had no closed check before its FIRST dial. A switch message already queued when Close runs can still be picked by watchFailover's select (message vs closeCh, random when both are ready), so it ran after Close and made one dial (a ROLE request reaches the new master; the client was closed again, so no leak). 784d60a returns at entry when the client is closed. So ff1f376 makes no further dial after a retry pause and installs nothing; the entry check completes "no dial for a closed client".

Seam: sentinelSwitchBackoffBase (var, default the 100 ms that was a literal), the same pattern as sentinelDownGrace, so the test can make the pause end at once.

Tests (sentinel_close_switch_1104_test.go): a master whose reply to ROLE is held lets the test call Close at the exact point, then releases it.

• TestSentinelCloseDuringSwitchMasterDialInstallsNothing1104: Close during the dial, 20 runs: no client installed on the closed SentinelClient and the dialled connection closed.
• TestSentinelSwitchMasterAfterCloseMakesNoDial1104 (b784352): handleSwitchMaster called after Close, the state a lost pick leaves, 50 runs: no ROLE, nothing installed.
• TestSentinelSwitchMasterDoesNotDialAfterClose1104: the master answers slave so the dial fails, Close while it is held, pause 1 ns (a tie), 100 runs: exactly one ROLE.

B. refresh / redirect after Close (cluster.go)

Close calls closeNodes, which leaves c.nodes nil. Two writers took c.mu afterwards and wrote into the nil map, which is a panic that ends the process (in the refresh goroutine):

• refreshTopology: when its CLUSTER SLOTS Do finished before Close and it took c.mu after closeNodes, c.nodes[addr] = n (and the replica one). It dials the node first, and nothing would close it. The window is microseconds, once per 60 s tick or per MOVED-triggered refresh.
• getOrCreateNodeAs (a MOVED/ASK redirect to an address not known yet): dials, then c.nodes[addr] = n. The review named only the refresh; this is the same defect on the same write.

Fix (commit 2): applyTopology returns ErrClosed when c.nodes == nil, before it dials or writes. getOrCreateNodeAs returns ErrClosed before dialling when the client is closed, and, under c.mu, closes the client it dialled and returns ErrClosed if c.nodes is nil (a Close that ran during the dial). The refresh dials under c.mu, so a Close waits for it and closes what it stored.

Seam: refreshTopology is split in two; applyTopology(ctx, v) is its part under c.mu, no behaviour change, so a test runs it right after a Close.

Seam: the part of getOrCreateNodeAs under c.mu is installNode (no behaviour change).

Tests (cluster_close_refresh_1104_test.go): take a real CLUSTER SLOTS reply, Close, then call applyTopology / getOrCreateNode / installNode; 30 runs each: no panic, ErrClosed, and c.nodes still nil (nothing stored); installNode also closes the client it was given, and its two other outcomes (stores a new address, returns the known node and closes the duplicate) are pinned on a live cluster. An earlier version asserted that no connection to the node was made; that could never fail (a Client dials lazily), so it is gone.

Evidence

golang:1.27.3, --cpus 2, 8 MiB memlock, -race.

	result
A, tests alone (ac8f75a), -count=5	5 of 5 FAIL for both tests: installed on the closed client and left open in 20 of 20 runs of every iteration; a further dial after Close in 43 to 59 of 100
B, tests alone (bfc657b), -count=20	20 of 20 FAIL for both tests, 30 of 30 panics per run
entry test alone (b784352), -count=20	20 of 20 FAIL, dialled in 50 of 50 runs each
head, the six tests, -count=20	20 of 20 pass each
head, go test -race ./driver/redis, once	ok

Mutants (each applied to the head, the named test then fails, restored): A without the check before the store: installed in 20 of 20; A without the check after the pause: dialled again in 55 of 100; A without the entry check: dialled in 50 of 50; B without the nil check in applyTopology: 30 of 30 panics; B without the nil check in installNode: 30 of 30 panics (TestClusterInstallNode1104); B with installNode not closing its client on ErrClosed: 30 of 30 left open. Not covered, and not coverable on its own: the early closed check at the top of getOrCreateNodeAs. Removing it leaves every test green, because the node is then dialled (lazily, nothing observable) and installNode answers ErrClosed with the same result; it only saves building a client for a closed cluster. The test comment says so.

Lint: golangci-lint (CI pin v2.14 image) 0 issues; go vet on linux, darwin and windows. The new tests also pass natively on macOS (-race -count=3).

The same shapes elsewhere in the drivers

• driver/redis/pubsub.go reconnect backoff vs closeCh, and sentinel.go subscribeLoop: check closed after the select / at the top of the loop, and subscribeLoop re-checks under sentinelMu before it lists a sentinel; nothing to change.
• driver/redis/cluster.go initNodes/NewClusterClient: writes c.nodes before the client is shared; the failure path calls closeNodes single-threaded.
• driver/redis/cluster.go other readers of c.nodes (anyNode, ForEachNode, measureLatencyLocked): read a nil map, which is safe.
• driver/memcached/cluster.go: nodes is built once and never reassigned; Close stops and waits for the probe loop before it closes the clients, and a probe after the ctx is done fails at once. No instance.
• driver/internal/async/pool.go: Release checks closed and closes the conn, and Acquire checks it on entry; a dial in flight during Close is handed back through Release. No instance.
• driver/postgres: no map or client installed by a background goroutine after Close that I found; its select-on-timer sites are the failAll timeouts listed in driver/internal/eventloop: RegisterConnWait stops at a done ctx even when its pause has ended too #1140. I did not audit every postgres path.

Not covered: only these files were read for the shapes; a path that installs state from a goroutine with no closed check under its lock may still exist.

Release note

Fixed: SentinelClient no longer leaves a live connection behind when it is closed during a failover, and ClusterClient no longer panics when it is closed while a topology refresh or a redirect is in flight.

[coderabbitai]

Caution

Review failed

The pull request is closed.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!