fix(singleflight): give waiting callers the real error of the run they waited on - #1014
Conversation
…y waited on A caller that arrived while fn was already running waited for that run but then always got nil, no matter how the run ended. nil looks like success, which breaks callers that retry until an operation succeeds. The concrete victim is sentinelClient.refreshRetry, which retries refresh() until it returns nil. refresh() is deduplicated, so during a sentinel failover — when refreshes keep failing until a new master is elected — refreshRetry can end up waiting on a refresh started by some other caller. If that refresh fails and refreshRetry is told nil, it stops retrying while the master is still unresolved. If the other caller does not retry either (a periodic topology refresh tick, for example), nobody is left to re-resolve the master: recovery that should finish the moment the election ends instead waits for the next periodic refresh, or never happens. Store the error on a per-run holder written before the channel close, so every waiter reads the outcome of exactly the run it waited on. Cluster callers are compatible: pick/pickMulti/pickMultiCache now surface the real refresh error instead of falling through to ErrNoSlot, and lazyRefresh ignores results entirely.
| // before ch is closed, so any goroutine that observed the close may read err | ||
| // without further synchronization. | ||
| type flight struct { | ||
| err error |
There was a problem hiding this comment.
I wonder if an additional flight struct is needed. Can't the error be embedded in the call struct directly?
There was a problem hiding this comment.
Good question — it was tried, and the error cannot live on call, because call is reused by the next flight.
do() clears the flight before it closes the channel, so the next flight can start while the previous waiters are still waking up. If the error is a field of call, that next flight overwrites it inside this window, and the waiters then read its result instead of their own.
All five current tests still pass with the error moved to call, so the suite does not cover this. A test with 50 waiters, where the next flight starts as soon as the previous one clears its counter, fails at once:
WARNING: DATA RACE
Write at 0x00c00007eda0 by goroutine 60: (*call).do() singleflight.go:70
Previous read by goroutine 29: (*call).Do() singleflight.go:42
--- FAIL: joiner 0 got <nil>, want flight failed
That <nil> is the bug of this PR again: a waiter is told its flight succeeded when it failed.
The data race is a second problem. A waiter reads the error without the lock. That is safe for its own flight, because the write happens before close(ch) and the read happens after <-ch. It is not safe against the next flight's write, which has no such ordering. So an error on call would also need the mutex on every wake, while flight.err needs no synchronization at all.
flight holds only what belongs to one flight. The channel is already per flight and the error has the same lifetime, so they live together, at the cost of one small allocation per refresh.
That 50-waiter test can be added here if you want it, since the current tests clearly do not cover this case.
There was a problem hiding this comment.
hmm, ok. Maybe we should replace the ch channel with a wg sync.WaitGroup to reduce GC overhead.
There was a problem hiding this comment.
IDK.. I was unable to have wg and keep cancellable property. All solutions with the same behavior but fixed issue require 1 extra alloc compared to main branch.
| nobody waits | a waiter joins | |
|---|---|---|
| main today, waiters get nil | 112 B, 1 alloc | 112 B, 1 alloc |
flight{err, ch}, this PR |
136 B, 2 | 136 B, 2 |
flight{err, wg} |
32 B, 1 | not cancellable |
chan error, buffered |
128 B, 2 | 128 B, 2 |
| error box, allocated by the first waiter | 112 B, 1 | 128 B, 2 |
Last one is kinda close perf-wise and only 16 bytes extra alloc. I can push that version if it makes more sense (but it's more complicated than the one here).
There was a problem hiding this comment.
I see. Sorry, I forgot about the cancellation.
…onn (#1015) ## Summary On a failover `_switchTarget` could close the live `mConn`/`rConn` and leave the client pointing at a closed mux. This closes only connections the call itself dialed. ## Impact - After an **ordinary failover** the client loses all connectivity to a *healthy* Redis: every command fails with `ErrClosing` **without even attempting to dial**, and the client holds no usable connection to any node. - Recovery depends on `refreshRetry`, which `switchTargetRetry` spawns on failure. That normally re-dials and repairs `mConn` — 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. - It becomes permanent when it coincides with a concurrent failing refresh: the deduplicated `refresh()` hands the joining `refreshRetry` `nil`, `refreshRetry` treats that as success and exits, and nothing is left to replace the closed `mConn`. That is the singleflight bug fixed in #1014, which is why the two belong together. ## Root cause `_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*, so `mConn`/`rConn` referenced a closed mux and nothing replaced it (the swap is only reached on success). This is reachable in normal operation: a just-demoted master answers `ROLE` with `slave` → `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`/`rConn` and 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 `mConn` means writes fail `-READONLY` until refresh replaces it. That is strictly better than `ErrClosing` on every command including reads, and it is transient either way. ## Tests - `TestSwitchTargetDoesNotCloseReusedConn` — fails against the previous code (closes the connection still referenced by `mConn`), passes with the fix. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **High Risk** > Touches Sentinel connection lifecycle during failover; the prior behavior caused prolonged total client outage (`ErrClosing` on every command). The fix is narrowly scoped but sits on critical Redis connectivity paths. > > **Overview** > Fixes a Sentinel failover bug where `_switchTarget` could **close the live `mConn`/`rConn`** when ROLE validation failed but the target was the **reused** connection for an address the client already held (e.g. demoted master returns `slave` → `errNotMaster`). > > The change tracks whether `target` is reused vs newly dialed and uses **`closeIfOwned`** on 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 **`TestSwitchTargetDoesNotCloseReusedConn`** to lock in that `mConn` must not be closed on that failure path. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit e2e751b. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
The call struct is reused by the next flight, which is why the error lives on the flight rather than on call. Nothing covered that: moving the error to call keeps all the existing tests green while bringing back the nil the waiters used to get. TestSingleFlightJoinerKeepsItsOwnFlightError closes that gap. Fifty waiters join a failing flight while the next one starts as soon as the first clears its counter, so it overlaps with the waiters waking up. With the error on call it fails at once, both as a wrong value and as a data race. The other two drive callers into the moment a flight completes, half of them able to be cancelled and half not, since the two take different branches of Do. One leaves the cancellable callers alone, the other cancels them.
61b005e to
5cb1475
Compare
2 participants
Summary
A deduplicated call handed joined waiters
nileven when the run failed. This gives every waiter the real error of the run it waited on.Impact
nillooks like success, so callers that retry until an operation succeeds stop early on a silent failure.refreshRetryretriesrefresh()until it returnsnil.refresh()is deduplicated, so during a failover it can wait on another caller's failed refresh, be toldnil, and stop retrying while the master is still unresolved — recovery that should finish when the election ends instead waits for the next periodic refresh, or never happens.pick/pickMulti/pickMultiCachemask the real refresh error asErrNoSlot.Root cause
singleflight.Doreturnednilto any caller that joined an in-flight run, regardless of how that run ended.Fix
Store the error on a per-run holder written before the channel is closed, so every waiter reads the outcome of exactly the run it waited on. Cluster callers surface the real error through their existing
return nil, errpath — no call-site changes needed.Tests
TestSingleFlightJoinerReceivesFlightError— a joined waiter now observes the run's error.Note
Medium Risk
Touches core concurrency deduplication used by sentinel failover and cluster slot refresh; behavior change is intentional but affects error propagation on shared in-flight work.
Overview
call.Do/DelayDono longer returnnilto goroutines that joined an in-flight run when that run failed. Waiters now receivefl.errfrom a new per-runflightstruct (error set before the wait channel closes), instead of always gettingnilafter<-ch.This fixes retry-until-success callers (e.g. sentinel
refreshRetry, clusterpickpaths) that could treat a failed shared refresh as success and stop retrying or mask the real error asErrNoSlot.Tests extend
TestSingleFlightto expect all 1000 callers to see the error, and add stress/race coverage for joiners, overlapping flights, and cancellable waiters at completion.Reviewed by Cursor Bugbot for commit 5cb1475. Bugbot is set up for automated code reviews on this repo. Configure here.