Skip to content

fix(recovery): guard claim fan-out send on context cancellation to unblock Stop - #2118

Merged
AkramBitar merged 1 commit into
mainfrom
fix-2038-recovery-fanout-deadlock
Aug 10, 2026
Merged

fix(recovery): guard claim fan-out send on context cancellation to unblock Stop#2118
AkramBitar merged 1 commit into
mainfrom
fix-2038-recovery-fanout-deadlock

Conversation

@AkramBitar

Copy link
Copy Markdown
Contributor

Fixes #2038

The recovery manager fanned claims out to its worker pool with an unguarded work <- claim send. On shutdown the workers return from their own select on ctx.Done() without draining work, so that send blocked forever: close(work) was never reached, the recovery loop's deferred wg.Done() never ran, and Stop()'s wg.Wait() hung while holding m.mu — wedging every later Start()/Stop().

Changes

  • Extract the fan-out into Manager.fanOut and guard the send on ctx.Done(), mirroring services/cleanup/manager.go.
  • Keep close(work) / workerWG.Wait() on the cancellation path so no worker goroutine leaks, and surface the cancellation as errors.Join(fanOutErr, firstErr) instead of swallowing it.
  • Undispatched claims stay Pending, so the next sweep re-claims them once their lease expires — no work is lost.
  • docs/services/storage/recovery.md: new "Shutdown Behaviour" section documenting the above.

Test

TestManager_StopDuringFanOutDoesNotDeadlock reproduces the issue's numbered failure scenario step by step: 64 claims to a single worker parked inside Recover (fan-out blocked on the unbuffered send), Stop() from a goroutine, worker released only after cancellation so both its select arms are ready, then a guard on Stop() returning and a final Start()/Stop() pair proving the manager is not wedged.

Verified both directions: it fails against the unguarded send (Stop() deadlocked while the sweep was fanning out claims) and passes with the fix under -count=3 -race. gofmt, go vet, and golangci-lint are clean.

@AkramBitar AkramBitar added this to the Q3/26 milestone Aug 3, 2026
@AkramBitar AkramBitar self-assigned this Aug 3, 2026
@AkramBitar
AkramBitar force-pushed the fix-2038-recovery-fanout-deadlock branch from e925b17 to d42163c Compare August 3, 2026 19:53
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

📊 Token Validation Benchmark

Comparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.

Variant Benchmark Params Workers TPS (base → PR) Δ TPS
csp BenchmarkAPIGRPC f=1, nc=4, w=token-validation-service 4 147 → 148 ➖ +0.7%
csp BenchmarkLocalTokenValidation out-tokens=2in-tokens=2 4 115 → 115 ➖ +0.1%
ipa BenchmarkAPIGRPC f=1, nc=4, w=token-validation-service 4 78 → 78 ➖ +0.0%
ipa BenchmarkLocalTokenValidation out-tokens=2in-tokens=2 4 68 → 69 ➖ +0.4%

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

📊 Token Validation Benchmark

Comparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.

Variant Benchmark Params Workers TPS (base → PR) Δ TPS
csp BenchmarkAPIGRPC f=1, nc=4, w=token-validation-service 4 118 → 118 ➖ -0.1%
csp BenchmarkLocalTokenValidation out-tokens=2in-tokens=2 4 104 → 104 ➖ -0.1%
ipa BenchmarkAPIGRPC f=1, nc=4, w=token-validation-service 4 78 → 78 ➖ -0.3%
ipa BenchmarkLocalTokenValidation out-tokens=2in-tokens=2 4 89 → 89 ➖ +0.1%

@AkramBitar
AkramBitar requested review from SaidAltury-ibm and removed request for HayimShaul August 5, 2026 13:57

@atharrva01 atharrva01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @AkramBitar , took a careful read since I have been working in this file recently. The deadlock traces through exactly as you describe: worker returns from its own select on ctx.Done() without draining work, work is unbuffered and fanOut is the only sender, so the send parks with no receiver and close(work) is never reached. Guarding the send looks right to me, and keeping close(work) and workerWG.Wait() on the cancellation path seems like the important detail.

Ran the test at -count=3 -race and it passes. I liked that it drives the failure through the real Start/Stop path rather than reaching for the unexported fan-out.

Three observations below, none of them blocking, and some possibly things you already weighed up.

Comment thread token/services/storage/services/recovery/manager.go
Comment thread token/services/storage/services/recovery/manager.go Outdated
Comment thread token/services/storage/services/recovery/manager.go Outdated

@Effi-S Effi-S left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AkramBitar
AkramBitar force-pushed the fix-2038-recovery-fanout-deadlock branch from d42163c to 6ee7243 Compare August 10, 2026 17:45
@AkramBitar

Copy link
Copy Markdown
Contributor Author

@atharrva01

Thanks a lot for the review. I fixed all the comments. Hope now it is ok.

Regards,
Akram

@AkramBitar
AkramBitar force-pushed the fix-2038-recovery-fanout-deadlock branch 2 times, most recently from 8e3f497 to 44290ee Compare August 10, 2026 18:15
…block Stop

The recovery manager fanned claims out to its worker pool with an unguarded
`work <- claim` send. On shutdown the workers return from their own select on
`ctx.Done()` without draining `work`, so that send blocked forever:
`close(work)` was never reached, the recovery loop's deferred `wg.Done()` never
ran, and `Stop()`'s `wg.Wait()` hung while holding `m.mu`, wedging every later
`Start()`/`Stop()` call.

Extract the fan-out into `Manager.fanOut` and guard the send on `ctx.Done()`,
keeping `close(work)` and `workerWG.Wait()` on the cancellation path so no
worker goroutine leaks, and surface the cancellation instead of swallowing it.
Undispatched claims stay `Pending`, so the next sweep re-claims them once their
lease expires and no work is lost.

`fanOut` also returns the number of claims it dispatched. The sweep summary
counted successes as `len(records)-failures`, which was only correct while the
fan-out either completed or hung; a cancelled fan-out leaves a tail that never
reaches `errCh`, so a partial sweep reported never-attempted claims as
succeeded. Successes are now counted against the dispatched total and a short
dispatch warns on its own.

A sweep aborted by `Stop()` is an ordinary shutdown, so `recoveryLoop` logs a
cancelled sweep at debug level via `logSweepError` and keeps the warning for
genuine failures.

Signed-off-by: AkramBitar <akram@il.ibm.com>
@AkramBitar
AkramBitar force-pushed the fix-2038-recovery-fanout-deadlock branch from 44290ee to ae29395 Compare August 10, 2026 20:04
@AkramBitar
AkramBitar merged commit 5655709 into main Aug 10, 2026
153 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

services/recovery: unguarded fan-out send can deadlock Stop() on shutdown race

3 participants