[FumingPower3925]

Lane L15 DRV-REDIS of v1.6.0. Refs #887, Refs #890. Draft.

Method: the premise of each item was checked on main first (0897add, then 8936e89 after #1127 merged; none of the five changed). Commit 1 holds the tests and compiles on main; commit 2 is the change. Every test below fails on main as an assertion, except the wake source guard (item 890.50), which pins code that is already correct and is checked by mutants.

Item numbers are the position in the issue body counting EVERY checkbox, ticked or not (#887 has 31, #890 has 98). #1099 and #1050 count unchecked boxes only, which gives other numbers (#1050 calls the wake test "item 49"); each row quotes the item's opening words.

item	opening words	premise	result
887.28	"drivers, io_uring dial: a driver's dial fails with celeris/iouring: fd already registered"	TRUE, reproduced on main with the real io_uring engine	fixed, all three drivers' dials
887.29	"drivers, redis sentinel: SentinelClient.watchFailover waits on sc.subPS.Channel()"	TRUE (still so on main)	fixed
887.30	"drivers, redis PubSub reconnect backoff: the reconnect loop's in-loop conn.closed check"	TRUE	fixed
887.31	"drivers, redis cluster PubSub: ClusterClient.Subscribe and ClusterClient.SSubscribe bind their PubSub to the client of one node"	still open on main	NOT touched: decision D3, see "Decision pending"
890.50	"#932 wake test: TestWriteRacingCloseNeverTouchesTheClosedEventfd862 ... catches its defect only through the race detector"	TRUE: mutant WAKEATOMIC passes every behavioural test of the package	source-guard test added (the hook test the issue suggests cannot work, see below)
890.89	"portable worker: an UnregisterConn(fd) that runs while RegisterConn(fd, ...) is still opening the net.Conn is lost"	TRUE, reproduced	fixed

Files: driver/internal/eventloop/{register_wait.go,portable.go} (new helper, portable fix), driver/{redis,memcached,postgres}/conn_unix.go (one line each), driver/redis/{pubsub.go,sentinel.go}, tests. No exported API of a public package changes (driver/internal/... is internal): mage apiCheck says api/ matches the exported API of 53 packages; api/driver/redis.txt is untouched.

Commits

1. 0e27c41 tests (and two declared knobs the tests set, see below).
2. 502a4f4 the change.
3. 5152237 review round 1, tests first (one more declared, unused name: errRegisterWaitBudget).
4. 4340ecd review round 1, the change.
5. 4a8f934 merge of main (f4ae3ad), no conflicts.

Review round 1 (Opus, head 502a4f4) found one major and four minors; all are in "Review round 1" below.

Round-0 tested sha: 502a4f4 (rebased on 8936e89). The head after review round 1 is 4a8f934; its runs are listed under "Review round 1". The mutant and -count=20 runs below were made at 3680b82, the same two commits before the rebase onto #1127 (which changed internal/engine/iouring/worker.go; the driver files and the engine's refusal text at internal/engine/iouring/driver.go:242 are unchanged by it). The full driver suites and the one-worker runs were repeated at 502a4f4.

887.28: the dial

Reproduction first, on main, the real io_uring engine (a celeris.Server with Engine: IOUring, the PubSub rig of pubsub_rig_engines_887_linux_test.go; docker --cpus 2 --security-opt seccomp=unconfined, since docker's default seccomp profile blocks io_uring and the rig skips): TestDialAfterTheCloseOfAConnOnTheSameNumberSucceeds887 has 4 goroutines close a conn and dial the next at once, 2000 dials per run, over a fake redis (ForceRESP2, so the dial is all that is measured).

tree	io_uring workers	runs that fail	dials refused per run of 2000
main (commit 1)	2	10 of 10	107, 65, 217, 54, 119, 195, 30, 27, 75, 76
main (commit 1), after #1127	2	5 of 5	69, 79, 81, 97, 157
main (commit 1)	1 (--ulimit memlock=8388608, a runner's 8 MiB)	5 of 5	16, 1, 266, 95, 112
commit 2	2 and 1	0 failures in -race -count=20 (2 workers) and in a one-worker run	0 of 2000

The standalone loop and the epoll engine were clean on both trees (0 of 2000 in every run; the epoll engine runs onClose before UnregisterConn returns). Log: l15drvredis/logs/c1-dial-reuse-redis-count10.log.

The change: eventloop.RegisterConnWait(ctx, loop, fd, onRecv, onClose) is loop.RegisterConn that, on exactly the io_uring engine's refusal, retries with a pause that grows from 100 us to 5 ms until the number is finalized, ctx is done, or 2 s have passed, and then returns the refusal. The three conn_unix.go dials call it. The 2 s bound is for a close that lingers (SO_LINGER delays that conn's onClose, celeris#735); it is a guess, not a measurement.

• eventloop.IsNumberBusy matches the text (celeris/iouring: prefix, fd already registered suffix): the engine has no sentinel and I did not touch engine files. TestTheIOUringRefusalIsWhatTheDialWaitsOut887 registers one number twice on a real io_uring worker and requires the match, so a reworded engine message turns that test red instead of silently turning the wait off. The Adaptive engine's driverWorkerLoop.RegisterConn returns the inner error unwrapped (internal/adaptive/provider.go:73-76), so the prefix survives there. Any other error (... is already an HTTP connection, a shut-down worker, the epoll fd N is already registered, the standalone loop's ErrAlreadyRegistered, which does not clear by itself) is returned at once.
• Deterministic half, no engine needed: a WorkerLoop wrapper that refuses the first 3 RegisterConn calls with the io_uring text (TestDialWaitsOutAnUnfinalizedNumber887 in redis, memcached, postgres; fail on main with RegisterConn was called 1 times), plus two that pass on main and guard the fix against being too wide or unbounded: another error is not retried, a refusal that never clears gives up with the ctx (300 ms ctx: 302 ms, 44 calls).
• The retry in the PubSub rig (newPubSubAfterReuse, from 887 item 10) is removed, as its comment asked. Honest limit: TestPubSubControlWritesDropsAndCloseInterleave929 passes 20 of 20 on all three engines at one io_uring worker both on commit 1 with the retry removed and on commit 2, so it does not discriminate; the regression test for this item is the dial test above.
• RULE 74: the dial is per connection, not per command. The success path adds one function call (IsNumberBusy(nil) returns at once). No command path is touched; I did not run a benchmark.

887.29: the sentinel that went away

On main, watchFailover waits on the sentinel PubSub's channel, which a conn drop does not close, and the PubSub reconnects to the same address for ever. Three tests, each on two sentinels A and B (A listed first), each fails on main with timed out after 8s waiting for the client to subscribe to the next sentinel after the first went away: A gone for good (listener closed); A accepts every reconnect and hangs up 5 ms later; A hangs up every 200 ms (a conn that is up as often as it is down). After the move B announces a failover and the client must follow it.

The change:

• PubSub.downFor() (unexported, built on the outage state of item 30 below): zero while the conn is up and has been since its replay stayed up for pubsubStableAfter; during an outage the time since it began while no conn is up or a replay is running; once a conn is up again, the time that outage lasted until that conn came up, so a conn that keeps dying soon after its replay shows a figure that grows and a blip that healed does not.
• watchFailover checks it on a ticker and, past sentinelDownGrace (5 s), closes that sentinel's PubSub and client and returns "gave up"; subscribeLoop then calls connectSentinelFrom(index of that sentinel + 1), so a sentinel that accepts and hangs up does not take the subscription back from the next address for ever (mutant SENT-NO-ROTATE fails the two flapping tests).
• subscribeLoop also closes the replaced sentinel's PubSub: until now only its client was closed, and a PubSub's conn is checked out of the pool, so its reconnect loop outlived it. The tests count the (*PubSub).reconnectLoop goroutines of the process before and after (the two changes together; the mutant that drops both fails the gone-for-good and the 5 ms tests).
• The values are captured when the PubSub / SentinelClient is built, so the goroutines never read the package variables the tests set.

887.30: the reconnect backoff

On main the backoff is reset when the replay returns, so a server that hangs up just after the replay is retried at the base delay for ever. TestReconnectBacksOffWhenTheServerHangsUpAfterEveryReplay887 (a fake that closes each conn 10 ms after SUBSCRIBE): main sees 39 SUBSCRIBEs in 2.5 s, gaps 53 to 80 ms; the change sees 6 (gaps 56, 115, 246, 484, 725 ms). TestReconnectBackoffStartsOverAfterTheConnWasStable887: conns 1 to 5 are cut after 10 ms, conn 6 lives 800 ms (stable window 400 ms in the test): on main the gap before SUBSCRIBE 5 is 62 ms; with the change 357 ms, and the reconnect after the stable conn waits about 47 ms (the base delay).

The change: an outage starts at the first drop (or at a drop of a conn that stayed up for pubsubStableAfter, 5 s = the backoff cap) and the backoff attempt continues across reconnect loops within it; Reset runs when an outage starts, not when a replay returns. TestReconnectLoopCountsAConnThatDiesDuringTheReplayAsFailed887 (item 15) still passes unchanged.

890.89: the portable worker

TestUnregisterConnWhileRegisterConnIsOpening890 is written as linearizability: either UnregisterConn returned ErrUnknownFD and the registration stands, fully live (and its own UnregisterConn fires onClose once), or it returned nil and the conn is fully torn down (onClose(nil), the net.Conn closed, the table clean). On main it returns nil and the conn stays registered: onClose never fired, 2 s. The fix: UnregisterConn treats register's placeholder (conn == nil) as an unknown number and leaves it, which is what the Linux workers answer (their RegisterConn inserts in one step). The alternative named by the issue, register re-checking that its placeholder is still in the table and closing the opened conn otherwise, would keep a nil return but then onClose has to fire for a conn the driver never saw registered. I took the first because it needs no new callback contract; redis and memcached Close ignore UnregisterConn's result and postgres' Close tolerates ErrUnknownFD. The shutdownConn comment that documented the bug is rewritten.

890.50: the source guard, and why it is not the suggested hook test

The issue asks for a hook in shutdown between the eventfd's close and the return of w.mu, with a wake that "loaded the number before the close" let through. teardown_hooks_890_linux_test.go already says why that cannot order the WAKEATOMIC shape: FD() returns -1 once the close has run, so a hook after the close never sees a stale number, the race detector reports nothing (an atomic and a syscall), and the window of the mutant is between two statements of its own wake, where the production code has no hook and, by design, no window.

TestWakeGoesThroughSignalOnly890 (wake_source_guard_890_test.go, no build tag, parses the package's non-test files, so it runs on darwin and windows too) requires that every use of the worker's wakeFD field is a method call allowed to its function (newWorker builds it, run calls FD, wake calls Signal, shutdown calls Close), that nothing reads the field bare (a copy or method value escapes WakeFD's lock), that (*worker).wake is exactly the statement w.wakeFD.Signal(), and that none of the four allowed uses has disappeared (so the scan cannot go empty).

Mutants of loop_linux.go (logs/mutants-wake-guard-c1.log; each restored, RESTORED_OK):

mutant	guard
WAKEATOMIC: wake = unix.Write(w.wakeFD.FD(), ...)	red: wake uses wakeFD.FD
WAKE-STALE-CACHED-NUMBER: an efdCache field written in newWorker, wake writes it	red
EXTRA-STATEMENT-IN-WAKE	red
COPY-OF-THE-FIELD (keep := w.wakeFD in run)	red

And WAKEATOMIC against the existing tests, 10 runs -race (logs/wakeatomic-existing-tests.log): TestAWakeAfterShutdownClosedTheEventfdWritesNothing890 10 of 10 PASS (does not catch it), TestWriteRacingCloseNeverTouchesTheClosedEventfd862 1 of 10 FAIL, the guard 10 of 10 FAIL. The two comments that said the shape was covered only by the probabilistic oracle now point at the guard. The guard is a source pin: whoever changes the wake design must update its allow-list, and its messages say so.

Failing on main (commit 1, l15drvredis/logs/c1-*.log, -race)

test	on main
TestDialAfterTheCloseOfAConnOnTheSameNumberSucceeds887/iouring	FAIL, 27 to 266 of 2000 refused
TestDialWaitsOutAnUnfinalizedNumber887 (redis, memcached, postgres)	FAIL: RegisterConn was called 1 times
TestSentinelMovesOn...887 (3 tests)	FAIL after 8 s
TestReconnectBacksOff...887, TestReconnectBackoffStartsOver...887	FAIL: 39 SUBSCRIBEs, gap 62 ms
TestUnregisterConnWhileRegisterConnIsOpening890	FAIL: onClose never fired
TestWakeGoesThroughSignalOnly890	PASS on main (pins correct code); red under the mutants above
TestDialGivesUp..., TestDialDoesNotRetryOtherRegisterErrors887, TestIsNumberBusy887 / TestRegisterConnWait887 (eventloop, commit 2)	pass on main / new with the helper: boundary guards of the fix

Commit 1 declares two package variables with no production reader, sentinelDownGrace and pubsubStableAfter, so the tests can shorten them and still compile on main (the precedent is #1050's commit d07fd64, which declared hooks nothing called).

Mutants of the change (logs/mutants-fix.log; each red test named; restored after)

mutant	red
NO-RETRY (helper returns the first error)	dial tests of redis, memcached, postgres, real-engine dial test, helper tests
RETRY-EVERY-ERROR	TestDialDoesNotRetryOtherRegisterErrors887 (redis, memcached), TestIsNumberBusy887
IGNORE-CTX	TestDialGivesUpWhenTheNumberStaysRegistered887, helper test
IOURING-TEXT-DRIFT (matcher expects another text)	real-engine dial test, TestTheIOUringRefusalIsWhatTheDialWaitsOut887, TestIsNumberBusy887
SENT-NEVER-GIVES-UP	all 3 sentinel tests
SENT-NO-ROTATE	the two flapping tests
SENT-DOWNFOR-NAIVE ("conn up now" means healthy)	the 200 ms flapping test only
SENT-KEEPS-OLD-PUBSUB (neither close site)	the gone-for-good and the 5 ms test (goroutine check)
BACKOFF-RESET-ON-REPLAY (main's behaviour)	both backoff tests
BACKOFF-NEVER-RESETS	...StartsOverAfterTheConnWasStable887
PORTABLE-UNREGISTER-DELETES-PLACEHOLDER	TestUnregisterConnWhileRegisterConnIsOpening890

Local results (docker golang:1.27.3, --cpus 2, one Go process at a time; a redis:7-alpine, postgres:17-alpine, memcached:1.6-alpine container each, --cpus 1, on a private network, removed after)

• At 502a4f4: go test -race -count=1 ./driver/... and go test -count=1 ./driver/... (without -race, so the !race alloc guards run): all five packages ok. TESTING_STRICT_ALLOC_BUDGETS=1 go test -run TestAllocBudgets ./driver/redis ./driver/memcached: ok.
• Conformance against live servers with -race (tip before the rebase): -tags redis ./test/conformance/redis (CI's -skip for the three password tests) 48 PASS 0 SKIP; -tags postgres 48 PASS 0 SKIP; -tags memcached 24 PASS 0 SKIP. Not run: the other server versions, valkey, sentinel and cluster jobs against real servers (there is no such test in the tree for item 29; the fakes are the fixtures), Windows and darwin runtime (GOOS=darwin|windows go vet ./driver/... clean).
• -race -count=20: the backoff, dial and pin tests (redis package, 116 s), the portable test (eventloop), and the three sentinel tests (151 s): ok.
• One worker (--ulimit memlock=8388608, the rig logs 1 workers): dial tests and pin test pass; 929 test 20 of 20 on all three engines.
• golangci-lint v2.14 run ./driver/...: 0 issues. mage apiCheck: ok.
• The new tests add about 6 s to the driver/redis package under -race (18.7 s to 24.8 s). The new io_uring subtests and TestTheIOUringRefusalIsWhatTheDialWaitsOut887 skip on a host without io_uring like the existing rig tests (the CI Unit job's SKIP tally and its alone-rerun apply as they do to those); the other new tests need no external service.

Release notes

• driver/redis, driver/memcached, driver/postgres: on the io_uring engine a dial no longer fails with celeris/iouring: fd already registered when the new socket takes the number of a conn that was just closed; the dial waits (up to 2 s, or its ctx) for the old conn to be finalized.
• driver/redis: SentinelClient gives up a sentinel whose subscription has been down or flapping for 5 s and subscribes to the next address of SentinelAddrs; the replaced sentinel's PubSub is closed (its reconnect goroutine used to outlive it). After a give-up the next sentinel is tried first, so the preference for the first address returns only when the next one fails.
• driver/redis: a PubSub whose server hangs up soon after every replay backs off (50 ms doubling to 5 s) instead of retrying at 50 ms for ever; the backoff starts over once a conn has stayed up for 5 s.
• Internal: the portable event loop's UnregisterConn answers ErrUnknownFD for a number whose RegisterConn is still opening the connection, instead of losing the registration.

Overlap with the follow-ups of #1117 (L11, not fixed here)

None of the four touches a file this PR changes: F1 (releaseOnExit / pollSyncMulti) is in loop_linux.go and conn.go, F2 in the execDirect helpers, F3 in the memcached/redis bridge users, F4 (releaseCmd not deferred) in client.go, commands.go, pool.go. This PR adds register_wait.go, one line in each conn_unix.go, and edits portable.go, pubsub.go, sentinel.go; it edits two comments (not code) in eventloop tests. Item 887.28 is the issue's own sibling of F4 in spirit (pool churn) but shares no code.

Findings, not fixed here

• A failover announced while the client was between sentinels (or during the 5 s grace) is not re-discovered: after a give-up the client subscribes to the next sentinel and hears only new +switch-master messages; it does not ask SENTINEL get-master-addr-by-name again, and the master is swapped only if it differs. Not a regression: on main the client stayed on the dead sentinel for ever. Fix shape (reviewer): after a reconnect that follows a give-up, run SENTINEL get-master-addr-by-name and swap the master client if the address differs.
• The engine has no sentinel error for the refusal; the text match should become errors.Is once an engine lane adds engine.ErrFDBusy (or similar) to internal/engine/iouring/driver.go:242. Not done: engine files.
• With more than one worker the dial could try another worker instead of waiting (workerHint = fd % NumWorkers puts the new socket on the stale worker); the wait is needed for one worker anyway, so this is not done.
• The alternative to waiting: when the number is refused, move the socket to a free number (F_DUPFD_CLOEXEC from fd+1, os.NewFile, close the original, register the duplicate). It works on one worker and on any thread, including the engine worker (see the review-round item 2 below), but changes which descriptor the driver owns and needs its own review. Not done. The engine-side option the issue names (the io_uring RegisterConn re-keys a closing conn) is an engine change.
• The driver/postgres ctx handling of doStartup waited about 30 s after a 1.5 s ctx expired while the server stayed silent (seen while writing the postgres dial test, which now uses a server that hangs up). Not investigated.

Decision pending: 887.31 (cluster PubSub, maintainer decision D3). Nothing is implemented.

The issue text offers: (a) have the cluster constructors re-route the PubSub on repeated reconnect failure (re-resolve the owner of the slot) and/or (b) expose a health signal on PubSub (a Down() channel or a counter of failed reconnects).

• Option A, re-route inside: ClusterClient.Subscribe / SSubscribe give the PubSub a private re-route hook; when it fires, the PubSub re-resolves (any live node for Subscribe, the slot owner for SSubscribe), swaps ps.client under ctlMu, and replays. api/driver/redis.txt: no change. Messages in the swap gap are lost (already documented as at-most-once).
• Option B, a public signal: a new method on *PubSub, for example method (*PubSub) Down() <-chan struct{} or method (*PubSub) Reconnects() uint64, as one added line in api/driver/redis.txt. Permanent API for v1.x, and it leaves the re-routing to every caller.
• Option C, both.

What A can and cannot detect (checked, review round 1 m4). This PR's unexported PubSub.downFor is a trigger for a node that is gone: a dropped conn, or a conn that keeps dying. It cannot see a slot move, because a slot move does not drop the conn. I ran it on redis:7-alpine, two cluster nodes, --cpus 1, on a private network, removed afterwards: an SSUBSCRIBE foo client (slot 12182) on node A, then the slot moved to B (CLUSTER SETSLOT ... IMPORTING on B, MIGRATING on A, NODE on both). The client received sunsubscribe / foo / 0 and stayed connected to A (CLIENT LIST shows it, ssub=0). The PubSub's push handling takes sunsubscribe as the acknowledgement of its own SUnsubscribe and ignores it (state.go:656,676), while ps.shardSubs still lists the channel, so today the subscription is silently dead after a slot move and a reconnect would replay the SSUBSCRIBE to the same node. So A needs two triggers: downFor for a gone node, and a server-initiated sunsubscribe (one the PubSub did not ask for) of a channel still in shardSubs for a slot move, after which it re-resolves the owner and re-sends SSUBSCRIBE there. A is therefore not just a re-routing of the existing reconnect: the second trigger needs the push path to tell its own unsubscribe from the server's. The plain Subscribe (SUBSCRIBE is cluster-wide) only needs the first trigger.

Recommendation, unchanged but now stated with its cost: A, B only if a user asks. A needs no exported surface and removes the silent dead subscription for both a gone node and a slot move; B would freeze a signal shape before anyone used the unexported one. The residual case, the whole cluster down, retries with backoff and nothing a caller could do differently is lost. The risk of A is in SSubscribe: the slot-map refresh and the unsolicited-sunsubscribe detection.

Review round 1 (Opus, head 502a4f4: REQUEST-CHANGES, one major)

Verified good by the reviewer: 887.28 off the worker (main 122, 209, 212 of 2000 refused, head 0 of 2000), 887.30 (at most 1 reconnect loop across 645 forced reconnects), 890.89 (no new leak), the 11 mutants, -race ./driver/..., windows/darwin vet, the 11 required checks.

M1 (major), fixed. A new data race (887.29): watchFailover closed the given-up sentinel's client outside sentinelMu while the sentinel was still listed, so a user's SentinelClient.Close in that window closed the same *Client; redisPool.Close reads and writes p.provider with no lock and calls eventloop.Release on the shared standalone loop, so two closers could drop a reference other clients hold. Three changes:

• redisPool.Close runs its body once (sync.Once); a second concurrent caller returns at once.
• The give-up takes the sentinel out of sentinelConns under sentinelMu (takeSentinel) and closes it only if it was still listed; whoever takes a sentinel out of the list, or declines to list it, closes it, so Close and the give-up never close the same client.
• Folded in, the pre-existing leak (item m2): subscribeLoop looked at no closed flag before it listed the sentinel it had just connected; under sentinelMu it now closes that sentinel instead of listing it when Close has run.

Tests (commit 3, driver/redis/sentinel_close_race_887_linux_test.go), each in separate -race processes, 8 runs per test (logs/r1-red-on-502a4f4.log, logs/r1-green-on-fix.log):

test	502a4f4	4340ecd
TestClientCloseTwiceConcurrently887 (two goroutines Client.Close, a bystander PubSub on the shared loop)	8 of 8 FAIL, 2 race reports each	8 of 8 ok, 0 reports
TestSentinelCloseVsGiveUp887 (the reviewer's probe: both sentinels hang up every 2 ms, grace 20 ms, 40 x New/Close)	6 of 8 FAIL (1, 1, 1, 2, 1, 1 reports), 2 ok	8 of 8 ok, 0 reports
TestSentinelCloseDuringSubscribeLeaksNothing887 (Close at a random 0-400 us, 300 iterations)	FAIL, 25 of 300 left a sentinel open (the reviewer measured 106 of 300 and 98 of 300 on main)	ok, 0 of 300

Mutants (logs/mutants-r1.log): Close not idempotent: TestClientCloseTwiceConcurrently887 red 3 of 3. The closed check before the list append removed: ...LeaksNothing887 red 3 of 3. The give-up closing without owning (takeSentinel bypassed) with the Once kept: TestSentinelCloseVsGiveUp887 stays green 6 of 6, because the Once makes the second close harmless: the two layers overlap, and the test is red only on the original 502a4f4 where neither exists.

Item 2 (minor, 887.28): the dial wait on the worker it waits for. A dial that runs on the io_uring worker that must finalize the number sleeps on itself (the reviewer: 2.0 to 6.0 s frozen, then the same refusal; main failed in milliseconds). Documented in the godoc of RegisterConnWait; not guarded, because the engine gives no cheap, reliable way to know the caller is on its worker. Reachability: with WithEngine on io_uring, epoll or adaptive all three drivers dial their command connections as direct net.Conns and never call RegisterConn (redis client.go:62-67, postgres pool.go:226-244, memcached client.go:52,67). The only public path left is redis Client.Subscribe called from a handler (the PubSub pool registers with the engine), and only with ForceRESP2 and no password or DB; with the default config that call already blocks on main (the HELLO handshake waits on the worker it occupies). F_DUPFD is the alternative, listed under Findings. The same commit wraps the returned error (nit n1): the refusal together with ctx.Err() or the budget (errors.Is sees both; tests-first in commit 3).

Item 3 (minor, 890.50): guard bypass, fixed. The reviewer's mutant (a copy of the eventfd number kept in a new field by newWorker and written by enqueueFlush) left wakeFD untouched and passed. The guard now also requires that efd in newWorker appears only as an argument of wakefd.New, unix.EpollCtl (including the int32(efd) of its event) or unix.Close, and that the only raw unix|syscall .Write*/.Pwrite* call in the package is flushLocked's on c.fd. Mutants (logs/mutants-wake-guard-r1.log, 6 of 6 red): WAKEATOMIC, EXTRA-STATEMENT-IN-WAKE, COPY-OF-THE-FIELD, WAKE-STALE-CACHED-NUMBER, the reviewer's BYPASS (red with newWorker uses the eventfd number efd outside ... and enqueueFlush calls unix.Write(w.efdNum, ...)), and NUMBER-STORED-IN-ANOTHER-FIELD. Known limit, in the test's comment: it pins syntactic shapes; a number reaching a producer by another route (a global, an integer returned by a helper) is not seen, and the probabilistic oracle of the 862 test is the backstop.

Item 4 (minor): the 887.31 paragraph, corrected above after running Redis 7 (a slot move sends sunsubscribe and keeps the conn).

Item 5: a failover missed between sentinels, listed under Findings with the reviewer's fix shape.

Runs on the new head (4a8f934 for the full run, the tests at 4340ecd; main merged is f4ae3ad): the new and changed tests -race -count=10 ok (redis 63 s, eventloop 21 s, the three sentinel move-on tests 76 s); the timing tests (TestSentinelMovesOn, TestReconnectBack, TestSentinelClose*, TestClientCloseTwice, TestDialAfter) at --cpus 1 -race -count=3 ok (59 s); go test -race -count=1 ./driver/... ok on 4340ecd and on 4a8f934; GOOS=darwin and GOOS=windows go vet ./driver/... clean; golangci-lint v2.14 0 issues; mage apiCheck ok.

Release-note addition (reviewer nit n2): a PubSub blip within 5 s of recovering from a long outage now waits up to the 5 s cap before reconnecting instead of 50 ms, because the backoff runs over the whole outage.

For the reviewer to attack

• IsNumberBusy is a string match on an engine message; the pin test is the only protection. Does the Adaptive engine ever wrap (I read provider.go:73-76: no)?
• RegisterConnWait holds the dialing goroutine up to 2 s under Pool.Acquire; is 2 s right, and should it be a Config knob (not added: public API)?
• Sentinel behaviour changes: the 5 s grace; the rotation of the preference order after a give-up; downFor's blip-versus-flap semantics (is "time the outage lasted until the conn came up" the right figure for a conn that is up but unstable?); the sentinel's own PubSub is now closed on replacement.
• PubSub backoff: the attempt counter now persists across reconnect loops of one outage and ps.backoff.Attempts() is no longer reset on success; tests that replace ps.backoff still pass. Can onConnDrop's episode decision and the loop's defer re-check ever start two loops, or start a loop whose attempt was written by a loop that has not yet returned?
• Portable UnregisterConn on a placeholder returns ErrUnknownFD: a caller that unregisters its own number while its own RegisterConn of it is still opening gets "unknown" and the registration stands. Is that the contract you want, or the deferred-teardown alternative?
• The source guard is strict by design; check that it does not pin more of loop_linux.go than the property (a legitimate refactor of run that moves the FD() call to a helper must update the allow-list).
• Timing tests (backoff 2.5 s windows, the 8 s sentinel waits, the 600 ms grace) were stable in 20 of 20 -race runs at --cpus 2; I did not run them at --cpus 1 or under a loaded CI runner.

[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 92.38095% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
driver/redis/sentinel.go	86.04%	6 Missing ⚠️
driver/redis/pubsub.go	93.33%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!