[FumingPower3925]

driver/redis: a cluster or sentinel PubSub follows its subscription to another node. Refs #887 (item 31, quoted: "drivers, redis cluster PubSub: ClusterClient.Subscribe and ClusterClient.SSubscribe bind their PubSub to the client of one node"). Maintainer decision D3, option A: the client re-routes inside, nothing public changes.

Commits: f8fdb86 the tests (they fail on the base), 1951ee6 the change; review round 1: d11531e tests (5 fail on 1951ee6), a63be3e the fixes, 43a9469 test diagnostics; review round 2: 32e4f12 + 840c486 (handshake race, its own pair), 3f2eb6a tests, 3b3de72 fixes, 5978381 a test-only wait; review round 3: 722a9de tests, 0e9bbf1 the flag, 02177f8 a test-only wait (the sentinel test). Round 2's 5978381 was the first test-only wait (the fake counted an SSUBSCRIBE before recording it; one CI failure of TestPubSubSunsubscribeOfAWantedChannelResubscribes887/resp2). api/ is untouched, mage apiCheck says "api/ matches the exported API of 53 packages".

The problem

The PubSub of ClusterClient.Subscribe, ClusterClient.SSubscribe and SentinelClient.Subscribe is bound to the *Client of one node. Its reconnect loop dials that client's pool, that is, that one address, for ever.

• A node that is gone: the PubSub retries the dead address for ever, nothing is delivered, nothing tells the caller.
• A slot move (run on redis 7.4.11, and quoted from the redis: dial waits out the io_uring number refusal, sentinel moves on from a lost sentinel, pubsub backoff, portable unregister, wake source guard (v1.6.0, Refs #887 #890) #1129 analysis): the old owner sends sunsubscribe <ch> 0 and keeps the conn. routePush took it for the ack of the PubSub's own SUnsubscribe and dropped it, while shardSubs still lists the channel: the subscription is silently dead.
• A re-SSUBSCRIBE sent to a node that does not own the slot is answered -MOVED, which pubsub mode dropped (empty bridge): silently dead as well.
• Sentinel: after a failover the master's old Client is closed and the user's PubSub retries that closed client for ever.

Design

Full note: evidence/lanes-20261025/L22REDISPUBSUB/design.md (same text as below).

Mechanism. One unexported hook on PubSub: route pubsubRoute (nil for a standalone Client: it dials its own client for ever, as before; the only differences there are that PubSub.Close now cuts its reconnect dial short and its backoff wait is closeCh-first). Every attempt of reconnectLoop calls route.pick(ctx, shard, cur, moveOn) before it takes any lock, then dials acquirePubSub on the returned client. The PubSub remembers which client owns ps.conn (connClient, under mu), so a conn is released to the pool it came from (async.Pool.Discard on a foreign pool would corrupt its counts).

• cluster, regular SUBSCRIBE/PSUBSCRIBE (node-local; the cluster bus delivers every PUBLISH to every node): stay on the node while it works; after a failed dial or replay move to the next node by address, skipping the ones marked failing.
• cluster, sharded: bounded (DialTimeout) refreshTopology, then the owner of the served slot (reached by the smallest channel of that slot); no owner in the table: any node (as the SSubscribe constructor does), which answers -MOVED if wrong and the loop asks again.
• the first attempt of an outage goes back to the node it lost (a blip is the common case); later attempts, or a conn that died soon after its replay (attempt > 0), move on.
• sentinel Subscribe: getClient(), the current master.
• the constructors (ClusterClient.Subscribe/SSubscribe) try the other nodes (or the refreshed owner) when the first dial fails, each node at most once (a stale topology that keeps naming a dead owner is not dialled again).

States and triggers. UP -> REROUTING (one reconnectLoop, reconnecting set) -> UP.

• T1 conn drop: the existing close hook.
• T2 unsolicited sunsubscribe <ch> (RESP3 push or RESP2 array): if ch is still in shardSubs the caller did not remove it (a control call updates the set before it writes), so the server dropped it: reroute. If ch is gone it is the ack of the caller's own call: ignored. Benign false positive: SUnsubscribe(x) then SSubscribe(x) before the ack arrives costs one extra reroute.
• T3 an error reply in pubsub mode on a cluster PubSub: -MOVED for the slot the PubSub serves, or -CLUSTERDOWN: reroute. A -MOVED for another slot answers something else and is ignored (real Redis answers a SUNSUBSCRIBE of a channel it holds no subscription for, in a slot it does not serve, with -MOVED; a second channel of another slot is not served). A -CROSSSLOT or any other error is the caller's mistake and does not loop.

One slot per PubSub. A cluster PubSub serves the slot of its first shard channel (servedSlot, kept while a channel of that slot is tracked). A reroute follows that slot and replays only its channels, so a channel of another slot (documented undefined behaviour) cannot take the first one down with a -CROSSSLOT, as the first head did; a channel of another slot is not served: if another node owns its slot the SSUBSCRIBE is answered -MOVED and it is not subscribed (as on the base); if the same node owns it, the node accepts it and delivers it until the next reroute of any kind, whose replay leaves it out (verified on real Redis; the base sent both slots in one SSUBSCRIBE and got -CROSSSLOT). The move of that slot does not reroute the PubSub (a sunsubscribe acts only for the served slot).

Reports carry their conn. The router callbacks (sunsubscribe, error reply) are made with the router of the conn they came from; the PubSub ignores one from a conn that is no longer ps.conn, and raises the trigger while holding mu, as the install does when it clears pendingReroute. A report that was in flight when the loop replaced the conn cannot start a second reroute.
T2/T3 run on the event-loop worker: they take ps.mu and reconnectingMu only, never ctlMu.

Locking and Close (the #1141 lessons).

• pick and the dial run with no lock held. The install is the existing one, under ctlMu and mu, with the closed check first, so a conn is never installed on a closed PubSub; a conn the loop dialled and could not install is released.
• PubSub.Close cancels the loop's context (ps.ctx): a dial or a topology refresh in flight ends with it.
• Every wait of the loop is closeCh-first (sleep): a select{closeCh:default:} before the timer select, and a closed re-check after it.
• A trigger from a live conn that arrives while a loop runs (reconnecting) is kept (pendingReroute) and the loop's tail starts one more loop (the fix(redis): bind PubSub control writes to their conn and release the dropped conn on reconnect #985 item-19 class: the conn is up, so the conn.closed re-check cannot see it). The flag is cleared at the install, so the second sunsubscribe of the same move (one per channel) does not make a second loop.
• A route error that matches ErrClosed (the ClusterClient or SentinelClient was closed) ends the loop and closes the PubSub: Channel() closes, control calls return ErrClosed. That is "the reconnect loop gives up", already in the PubSub doc.

Backoff reuse. No new budget. The loop is the existing one: ps.backoff (50 ms doubling to 5 s), ps.attempt over the whole outage, reset only after a conn stayed up pubsubStableAfter. A node that flaps, or a stale topology that keeps answering -MOVED, is retried at 50 ms, 100 ms, ... 5 s.

What happens to the user's calls during a reroute. control holds ctlMu across its write; the install and replay hold it too. A call before the install updates the sets (under mu) and writes to the old conn or gets ErrPubSubConnDown; the replay snapshot is taken under mu after the install, so it contains the call either way (an idempotent duplicate SUBSCRIBE at worst). A call after the install writes to the new conn, after the replay. An Unsubscribe during the reroute removes the channel from the set, so the replay omits it.

Ordering and delivery. At most one conn is bound to the PubSub's router at a time: the old conn's router is cleared under mu immediately before the new conn is bound and replayed (before, the old conn was detached after the replay, with a window in which both delivered). So the PubSub itself never delivers one message from two conns; a message published while it switches is lost (a gap), as pub/sub is at-most-once. The gap is bounded by the backoff delay + dial + replay. Order within one conn is kept. One caveat, for a regular cluster SUBSCRIBE only: the cluster sends a PUBLISH to every node over its bus asynchronously, so a message in flight around a switch can in rare cases reach both the old and the new node and be delivered twice (not reproduced: the fake cluster delivers synchronously, and a real one needs bus latency). A sharded channel has one owner and a sentinel PubSub one master, so neither can.

What the caller sees (release note)

• Channel() and Message are unchanged; one channel for the life of the PubSub.
• Gap: messages published while the PubSub switches nodes are lost. The PubSub never delivers one message from two conns; for a regular cluster SUBSCRIBE a message in flight on the cluster bus around the switch can rarely arrive from both nodes.
• Two slots on one PubSub (undefined behaviour, now stated precisely): the first channel's slot is served. A channel of another slot is not subscribed if its node answers -MOVED (no error, as before the PubSub followed its slot); if the same node owns it, it is delivered until the next reroute of any kind and then dropped silently. Neither takes the first channel down on a reroute any more, and the move of such a slot does not rebuild the served subscription.
• A Do whose context ended during a new connection's handshake could hand a stray or empty reply to another request (a bug on main since April, fixed here: see "Handshake request recycled").
• A SUnsubscribe of a channel the PubSub never had, in a slot another node serves, no longer rebuilds the healthy subscription (Redis answers it -MOVED).
• While it has no conn, control calls return ErrPubSubConnDown (existing). Nothing new is exported.
• Re-routing never gives up on its own (the existing loop has no budget; it retries with backoff until PubSub.Close).
• When the ClusterClient or SentinelClient is closed, a PubSub whose conn is down (or goes down afterwards) gives up: Channel() closes, control calls return ErrClosed. A PubSub whose conn stays up after the close (the standalone event loop is refcounted and shared with another Client, or WithEngine) keeps it until PubSub.Close. This is the one semantic call in the PR, a one-branch revert (errors.Is(err, ErrClosed) in reconnectLoop).
• Sentinel: the master PubSub follows a failover once its conn to the old master has dropped.

Other behaviour changes, for a ruling

• A failed PubSub dial marks the node failing (which anyNode uses for Publish and the topology refresh) unless the caller's own context ended; a successful PubSub dial clears the mark only if a failed PubSub dial was the last thing to change it (failFlag, one atomic word: the command path's and the topology refresh's Store replace it, and the clear is a single compare-and-swap on the PubSub's own mark, so it cannot erase a mark that changed after it looked).
• Each sharded reroute attempt runs CLUSTER SLOTS and applyTopology: N calls when N PubSubs get a sunsubscribe together (bounded by the backoff).
• The cluster constructors fall back to other nodes instead of returning the first dial error.
• The reconnect dial is cut short by PubSub.Close (before it ran to its 5 s bound).
• Channel() godoc: no longer says "until the reconnect loop exhausts its retries" (there is no budget); it closes on Close, or when the owning client is closed while the conn is down.
• Not done, by design: SentinelClient does not move a PubSub whose old-master conn is still up after a switch-master (no kick, no registry).

Tests

Method: commit 1 (the tests) compiles on the base and fails; commit 2 is the change. A fake cluster (rpCluster: HELLO, CLUSTER SLOTS with a stale-reply knob, (P/S)SUBSCRIBE, sunsubscribe on a slot move, -MOVED, PUBLISH/SPUBLISH fan-out, kill a node) hosts real PubSub conns, no tag, so they also run in the portable jobs.

• RED on f8fdb86 (base + tests), -race, Linux, --cpus 2: 14 FAIL, 3 PASS (the 3 pass on the base by design: TestClusterPubSubCloseCutsAStalledRerouteDial887, TestClusterPubSubCloseRacesAReroute887, TestStandalonePubSubIgnoresSunsubscribe887, guards that the change must not break). Log: logs/red-final2.log.
• GREEN on 1951ee6: the new tests -race -count=20, Linux, --cpus 2: ok (495 s); go test -race ./driver/redis/... once: ok (71 s); the new tests on darwin natively, -race -count=3: ok.
• Internal-API cells (pubsub_reroute_internal_887_test.go: triggers on RESP3 and RESP2, ack is not a trigger, error kinds, a trigger during the handover, the old conn cut off while the loop is held, a Close between dial and install, sleep closeCh-first) need the route and hooks, so they are in commit 2 and are proven by mutants.
• Mutants (each applied alone, tree restored after; logs/mutants-head.log, run on 1951ee6): NO-ROUTE, RELEASE-VIA-HOME-CLIENT (Close), RELEASE-OLD-VIA-HOME-CLIENT, OLD-ROUTER-NOT-CLEARED, NO-PENDING-REROUTE, PENDING-NOT-CLEARED-AT-INSTALL, SUNSUB-ALWAYS-TRIGGERS, SUNSUB-NEVER-TRIGGERS, MOVED-NOT-A-TRIGGER, CLUSTERDOWN-NOT-A-TRIGGER, ANY-ERROR-TRIGGERS, SLEEP-RANDOM-TIE, CLOSE-DOES-NOT-CANCEL-DIAL, NO-GIVE-UP-ON-CLOSED-OWNER, FLAPPING-RESETS-BACKOFF, NO-ROTATE-ON-FAILURE, NO-TOPOLOGY-REFRESH-FOR-SHARD, CONSTRUCTOR-NO-FALLBACK, SENTINEL-ROUTE-ABSENT: all red (names of the red tests in the log). INSTALL-ON-CLOSED (removing only the closed check at the install) survives: the closed check before each replay write catches the conn and releases it, so the two are layered; removing both (a two-edit mutant, run by hand) is red in TestClusterRerouteDoesNotInstallAConnOnAClosedPubSub887.
• Real redis (test/conformance/redis/pubsub_reroute_cluster_test.go, tag redis_cluster, runs in the "Redis Cluster" job): SSUBSCRIBE on a 6-node redis 7.4.11 cluster in one container, an empty slot moved with CLUSTER SETSLOT IMPORTING/MIGRATING/NODE, moved back at the end; RED on the base ("no message of after-move arrived within 30 s"), 3 of 3 green on the change. Node loss on the same real cluster (a regular SUBSCRIBE holder, then the owner of a shard channel, SHUTDOWN NOSAVE): re-subscribed on another node after 0.2 s, and on the promoted replica after 6.5 s (a throwaway test, not committed; the destructive tag needs per-node containers).
• Lint golangci-lint v2.14 (also with -tags redis_cluster): 0 issues. GOOS=windows/darwin/linux go vet ./driver/redis/: clean.

The #1140/#1141 sweep (select with a timer and a side effect)

• PubSub.reconnectLoop: fixed (sleep, closeCh first; also time.After leaked a timer per attempt).
• ClusterClient.refreshLoop: the ticker branch ran the refresh (dials nodes) with no look at closed; now it returns first. No test (the period is 60 s and a fixed constant).
• SentinelClient.subscribeLoop: already safe, the loop top checks closed right after the select.
• SentinelClient.watchFailover: the tick branch's side effect is takeSentinel/closeSentinel, which only the taker may close (redis: dial waits out the io_uring number refusal, sentinel moves on from a lost sentinel, pubsub backoff, portable unregister, wake source guard (v1.6.0, Refs #887 #890) #1129), so a tie after Close is harmless; the message branch goes through handleSwitchMaster, which checks closed first.
• SentinelClient.handleSwitchMaster: already closed-checked after the select and before the install (driver/redis: SentinelClient and ClusterClient no longer leak or panic when closed during a failover or a refresh #1141).

Review round 1 (Opus, head 1951ee6: approve with fixes) and what was done

item	fix	test	red on 1951ee6 (d11531e)
F1 closed check before the install survived as a mutant (a PubSub with no channel)	test only	TestClusterRerouteDoesNotInstallOnAClosedPubSubWithNoChannels887	green on both; INSTALL-ON-CLOSED now red
F2 a caller's cancelled ctx marked a healthy node failing	ctx.Err()==nil guard in openPubSub	TestClusterSubscribeWithACancelledContextDoesNotMarkANodeFailing887	red
F2b a PubSub dial cleared the command path's mark	clusterNode.psFailed: clear only what the PubSub path set	TestPubSubDialReportClearsOnlyTheMarkItSet887	red
F3 a -MOVED to a foreign SUnsubscribe rebuilt a healthy subscription	-MOVED acts only for the served slot	TestClusterForeignSUnsubscribeDoesNotRerouteAHealthySubscription887; real: TestClusterForeignSUnsubscribeKeepsTheSubscriptionConn (conn id from CLIENT LIST)	red; real red (conn id 14 -> 16)
F4 a second slot killed both channels	one served slot; the replay sends only its channels	TestClusterSecondSlotOnOnePubSubDoesNotKillTheFirst887; real: TestClusterSecondSlotOnOnePubSubKeepsTheFirstDelivering	red; real red (0 of 3 delivered)
F5 docs	Channel(), onConnDrop, ClusterClient.Subscribe/SSubscribe, this body, design.md
F6 a report in flight from the replaced conn made a second reroute	router callbacks carry the conn's router; ignored if not ps.conn; trigger raised under mu	TestClusterRerouteIgnoresAReportFromTheReplacedConn887 (loop held after the install, the old router reports)	not red on 1951ee6 by construction (the fix changes the callbacks' signature, so the test uses the new one); proven by the NO-CONN-IDENTITY-* mutants
F7 the constructor re-dialled a dead owner	each node at most once	TestClusterSSubscribeTriesADeadOwnerOnce887 (topology refreshes counted)	red (2 refreshes, want <= 1)

Counts: d11531e (previous head + the new tests): 25 pass, 5 fail (F2, F2b, F3, F4, F7), logs/red-r1.log. Head a63be3e: the new and old tests -race -count=20 Linux --cpus 2: ok (548 s); TestClusterRerouteAnswersEveryChannelOfAMoveWithOneLoop887 and the F6 test -race -count=50: ok (80 s); go test -race ./driver/redis/...: ok (74 s). Real redis 7.4.11 (one container, 6 nodes), the three real cells: red on 1951ee6 for F3 and F4, 3 of 3 green on a63be3e (logs/real-r1-red.log, real-r1-green.log). Lint v2.14 (also -tags redis_cluster): 0 issues; GOOS=windows/darwin/linux go vet: clean; mage apiCheck: matches 53 packages.

Mutants on a63be3e (28, each alone, logs/mutants-r1.log), all red: RELEASE-VIA-HOME-CLIENT, RELEASE-OLD-VIA-HOME-CLIENT, OLD-ROUTER-NOT-CLEARED, NO-PENDING-REROUTE, SLEEP-RANDOM-TIE, CLOSE-DOES-NOT-CANCEL-DIAL, NO-GIVE-UP-ON-CLOSED-OWNER, FLAPPING-RESETS-BACKOFF, NO-ROTATE-ON-FAILURE, NO-TOPOLOGY-REFRESH-FOR-SHARD, CONSTRUCTOR-NO-FALLBACK, INSTALL-ON-CLOSED (now dies in the F1 test), PENDING-NOT-CLEARED-AT-INSTALL, SENTINEL-ROUTE-ABSENT, NO-ROUTE, SUNSUB-ALWAYS-TRIGGERS, SUNSUB-NEVER-TRIGGERS, MOVED-NOT-A-TRIGGER, MOVED-ANY-SLOT, CLUSTERDOWN-NOT-A-TRIGGER, ANY-ERROR-TRIGGERS, NO-REPLAY-SLOT-FILTER, NO-CONN-IDENTITY-SUNSUB, NO-CONN-IDENTITY-ERROR, DIAL-CTX-MARKS-FAILING, DIALED-CLEARS-COMMAND-MARK, CONSTRUCTOR-REDIALS-SAME-NODE, SERVED-SLOT-NOT-KEPT. (The earlier INSTALL-AND-REPLAY mutant, two edits, is superseded: the single-edit one is red now.)

Known limitations (to be filed as issues, not fixed here)

• Every sharded reroute attempt runs CLUSTER SLOTS + applyTopology: N PubSubs hit by one slot move make N concurrent refreshes against the same probe node, repeated at each backoff step (no coalescing).
• The reconnect replay sends all channels of the served slot in one SSUBSCRIBE; it is not grouped by slot, so a PubSub that holds several slots serves only one of them (and the base's reconnect had the same -CROSSSLOT).
• Pre-existing: ClusterClient.Subscribe/SSubscribe (and Client.Subscribe) against a node that accepts the TCP connection and never answers HELLO block until the caller's context ends: DialTimeout does not bound the PubSub handshake. The reconnect loop bounds its own dial with a 5 s context, the constructors do not.
• A sentinel PubSub whose conn to the demoted master stays up after a switch-master is not moved (a regular SUBSCRIBE there still receives, PUBLISH is replicated).
• If the first (served) shard channel is removed while a channel of another node's slot remains, the remaining channel is not subscribed until the next reroute of any kind (its -MOVED was ignored when it was added; the replay then serves the re-selected slot; same as the base until then). Removing every channel and adding one of a new slot does follow it.
• applyTopology accepts a CLUSTER SLOTS reply with no parsable range and then deletes every node: a corrupt reply empties the client's node table (the Windows symptom above). It should reject such a reply.

Handshake request recycled (round 2, N1; a bug on main, fixed here, its own commit pair)

redisConn.handshake gave the HELLO request back to the pool (releaseResult) on every failed exec. When the caller's context ended while the HELLO reply was arriving, the reader goroutine had already popped the request and was still filling it in (dispatch, state.go); another Do took the same object from the pool and the late finish woke it with whatever the object held. Proof on main with a plain Client (no PubSub): a Do(PING) on a separate healthy client got an empty reply with a nil error (wrong replies 0/2/4 over 3 runs), and -race reported the data race (handshake -> releaseResult against dispatch). The code is from 5cb0f9b (April). This PR made it reachable in the required macOS -race Portable job (about 1 run in 105: a reroute's topology refresh dials a command conn while PubSub.Close cancels its context), and it is the only mechanism found for the Windows failure below.

Fix (840c486): release only on a *RedisError (the reply was dispatched), as every other caller of exec does; a context or network error leaves the request to the garbage collector. Nothing leaks: req.owned is never set in production code (grep '\.owned': only reset to false at state.go:42, state.go:381, pipeline.go:287), so releaseResult's Reader branch is dead and the change only skips one sync.Pool reuse. Test (32e4f12, linux and darwin only, a stress test meant for -race): 1500 fresh clients whose Do has a 50-350 us context against a fake server that answers HELLO after 0-300 us, while two goroutines check every PING reply of a healthy client. Red on the old code: darwin -race -count=20: 16 of 20 fail, 10 race reports, the checker gets &{Type:simple Str:[]} <nil>; it does not reproduce on Linux (the Linux standalone loop's sync path; not investigated), so the red proof is the darwin run. Green: darwin -race -count=20 x3 and -count=100 (0 races, 51 s), Linux -count=20 (in the full run below).

Release note: a Do whose context ended during the connection handshake could hand a stray or empty reply to another request.

Windows "no cluster nodes available" (round 1)

TestClusterPubSubCloseLeavesNoConnBehind887 failed once on the Windows portable job of a63be3e with celeris-redis: no cluster nodes available from SSubscribe. That text comes only from an empty node table (applyTopology deletes every node a reply does not list), not from the F7 change. The only mechanism found is the handshake race above (a HELLO reply delivered as a CLUSTER SLOTS reply); it was not reproduced, so "found", not "proven". The Windows job passed on 1951ee6 and 43a9469. After the fix the test checks the node table right after NewClusterClient and prints it with the number of CLUSTER SLOTS replies, so a recurrence says where it began. Darwin -race -count=100 of that test after the fix: 0 races (517 s).

Review round 2 (Opus, head 43a9469: request changes, one blocker) and what was done

item	fix	test	red before
N1 blocker: handshake recycled a request the reader still held	release only on *RedisError (840c486)	TestHandshakeWhoseContextEndedDoesNotRecycleItsRequestWhileTheReaderHoldsIt887 (32e4f12)	darwin -race -count=20: 16 fail, 10 race reports; not reproducible on Linux
N2 a sunsubscribe of a slot the PubSub does not serve (the same node owns it) rebuilt the served subscription	act on a sunsubscribe only for the served slot	TestClusterSunsubscribeOfANonServedSlotDoesNotRerouteTheServedOne887; real: TestClusterMoveOfASecondSlotOnTheSameNodeKeepsTheSubscriptionConn	red; real red (conn id 7 -> 9)
N3 godoc/PR said "not subscribed, as on base"	exact wording (SSubscribe godoc, this body, design.md)
N4 served channel removed, another node's slot stays: never subscribed	documented (godoc, Known limitations)
N5 stale psFailed	failing is a failFlag: Store (command path, refresh) takes the node over, a PubSub success clears only a mark a failed PubSub dial was last to set	TestPubSubDialSuccessDoesNotClearAMarkTheCommandPathSetAfterwards887	red
N6 two mutants survived (first-slot memory; pick by the smallest channel of any slot)	tests	TestClusterFirstChannelNotSmallestStaysServed887, TestClusterRerouteFollowsTheServedSlotNotTheSmallestChannel887	green on both heads; both mutants die now
Windows diagnostic	the close test checks the node table after NewClusterClient

Runs on 3b3de72: new and old tests -race -count=20 Linux --cpus 2: ok (633 s); the F6 test and TestClusterRerouteAnswersEveryChannelOfAMoveWithOneLoop887 -count=50: ok (81 s); go test -race ./driver/redis/...: ok (77 s); real redis 7.4.11, four real cells x3: green (N2 red before); darwin -race -count=100 of the handshake test and of the close test: 0 races. Lint v2.14 (also -tags redis_cluster): 0 issues; GOOS=windows/darwin/linux go vet: clean; mage apiCheck: matches 53 packages.

Mutants on 3b3de72 (33, logs/mutants-r2.log), all red: the 28 of round 1 (the sunsubscribe ones now also die in the N2 test) plus SUNSUB-ANY-SLOT, PSFAILED-STALE, SERVED-SLOT-ALWAYS-SMALLEST (the reviewer's M7), PICK-SMALLEST-OF-ANY-SLOT (the reviewer's M10) and HANDSHAKE-RELEASE-ON-ANY-ERROR (red in the handshake test, on darwin -race).

Review round 3 (Opus, head 5978381: approve with fixes, nothing blocking) and what was done

item	change	test	red before
m1 a mutant survived: the sunsubscribe handler reading the cached served slot instead of re-deriving it	test only	TestClusterServedSlotIsReselectedWhenItsChannelsAreGone887 (the reviewer's P1)	green on both; N2-NO-RESELECT dies
m2 failFlag interleavings: a command's Store(true) racing a PubSub success could lose the mark	failFlag is one atomic.Uint32 (bits: failing, set by a failed PubSub dial); Store replaces the word, clearPubSub is one CAS on the PubSub's own mark; no embedded atomic.Bool (its Swap/CompareAndSwap skipped the reset)	TestFailFlagCommandMarkIsNotLostToAConcurrentPubSubSuccess887 (300000 rounds) and TestFailFlagStateMachine887	red on 5978381: 1 to 16 lost marks in 300000 rounds, every run
m3 Known limitations said "never subscribed"	"not subscribed until the next reroute of any kind" (this body, design.md, the SSubscribe godoc)
m4 a deterministic N1 test	handshake releases through releaseHandshakeReq, which runs a nil-checked hook; handshake runs once per connection, putRequest and the command path are untouched	TestHandshakeGivesBackNothingWhenTheContextEnds887 (a server that never answers HELLO), TestHandshakeGivesBackTheRequestOfAnErrorReply887; no -race, any OS, 0.3 s	the old behaviour (release before the error check) fails both, 3 of 3
nit	the stress test is 1000 rounds and says it is meant to run once per CI job (a local -count=20 uses about half of the macOS port range)

The command path's four failing sites are unchanged: no lock, no allocation. There is no redis cluster command benchmark in the repo, so two micro-benchmarks stand in (BenchmarkFailFlagStore887, BenchmarkAnyNode887, darwin arm64, -benchtime=3000000x -count=6, 0 allocs both before and after): anyNode 16.2-17.4 ns before, 16.1-16.3 after; failing.Store(false) 1.9-2.8 ns before, 1.6-2.3 after.

Beyond the list: the sentinel test pushed +switch-master before the SentinelClient's background subscription existed (one failure in 20 stress runs); it now waits for that subscription (02177f8).

Runs on 02177f8: new and old tests -race -count=20 Linux --cpus 2: ok (649 s); go test -race ./driver/redis/...: ok (78 s); lint v2.14 (also -tags redis_cluster): 0 issues; GOOS=windows/darwin/linux go vet: clean; mage apiCheck: matches 53 packages. Mutants (37, logs/mutants-r3.log): all red except FAILFLAG-MARK-KEEPS-OLD, which is equivalent (markPubSub with Or(failing|byPubSub) always yields the same word as Store). New: N2-NO-RESELECT, FAILFLAG-CLEAR-ANY, FAILFLAG-CLEAR-READ-THEN-WRITE (killed by the stress test only), and the existing HANDSHAKE-RELEASE-ON-ANY-ERROR now dies in the two deterministic tests.

Not touched, as asked (to be filed): pipeline.go slab reuse after a context error; memcached defer putRequest before the error check.

[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

❌ Patch coverage is 93.61702% with 15 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
driver/redis/pubsub_route.go	90.78%	7 Missing ⚠️
driver/redis/pubsub.go	96.52%	4 Missing ⚠️
driver/redis/cluster.go	77.77%	2 Missing ⚠️
driver/redis/sentinel.go	66.66%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!