fetch: cancelled coalescing waiters no longer retain goroutines (Fixes #53) - #60
Conversation
Every Coalescing.Fetch started a per-caller goroutine that waited inside the singleflight group; a caller whose context was cancelled returned while its goroutine stayed blocked until the flight finished or hit MaxFlight. A cancellation storm on one stalled key retained one goroutine per abandoned caller for the whole flight lifetime (issue #53). Split group.Do into acquire/finish: the leader starts the flight's single worker goroutine, and joiners wait inline on the flight's done channel with their own ctx. A caller that stops waiting now retains nothing; a flight costs one worker no matter how many callers come and go. Two contract corollaries, both documented: - The flight worker's context deadline is exactly the flight's end, and eviction happens a grace margin later (evictGrace), so a ctx-respecting worker's deadline delivery always wins over eviction; the stale path only fires on a worker that ignores its context. - A flight everyone abandoned is no longer respawned at its deadline (nobody is left wanting the bytes); the stale entry is evicted lazily by the next real caller. Regression tests: TestCancelledWaitersRetainNoGoroutines fails on the old code (64 goroutines retained after 64 cancelled callers) and passes now; TestStaleFlightReplacedOnlyByALiveCaller pins the lazy-eviction contract. Fixes #53
|
@copilot please review this PR. Focus points: (1) the new singleflight split ( |
Reviewed. I don’t see a defect requiring code changes.
|
fetch: cancelled coalescing waiters no longer retain goroutines
Fixes #53
What
Coalescing.Fetchstarted one goroutine per caller, and that goroutine waitedinside the singleflight group even after the caller's context was cancelled.
Cancelled callers returned promptly, but their goroutines stayed blocked until
the shared flight finished or hit the 10-minute
MaxFlightbound — acancellation storm on one stalled key retained one goroutine per abandoned
caller.
The singleflight
group.Dois split intoacquire/finish: whoever leads aflight starts its single worker goroutine; every joiner waits inline on
the flight's
donechannel with its ownctx. A caller that stops waiting nowretains no goroutine at all; a flight costs exactly one worker no matter how
many callers join or abandon it.
Triage evidence
Verified real against the reviewed commit
84aac4e:git show 84aac4e:internal/fetch/fetch.go—Fetchallocates a resultchannel and spawns one goroutine per caller; that goroutine blocks in the
non-cancellable
group.Dowait while the outer select returns onctx.Done(). The mechanism exists exactly as the issue describes..work/verify/issue53/repro_test.go): stalled flight(
MaxFlight: time.Minute), 64 callers with 50 ms contexts. All 64 callersreturned on deadline while 64 goroutines remained blocked until the
flight was released — matching the issue's claim precisely.
availability). Not a deliberate simplification — the design docs never claim
per-waiter goroutine retention as a stance, and the fix is cheap and provably
correct.
Fixes
group.Do→group.acquire+group.finish.acquirejoins a live flightor creates/leads a replacement;
finishpublishes the result (writeshappen-before the
doneclose) and deletes only its own map entry, so alate-finishing stale leader still never evicts a replacement — the
TestLateStaleLeaderKeepsReplacementinvariant is unchanged.end, and aflight becomes evictable only after
end + evictGrace(maxFlight)(clamped to[25 ms, 30 s]). This ordering guarantee matters: without it, the waiter's
deadline timer fires epsilon before the ctx-respecting worker delivers, and a
still-waiting leader would evict its own flight and livelock (caught in
self-review:
TestFlightContextBoundsStalledOriginspun 22 s under-coverwith the naive version). The stale path now only ever fires on a worker that
ignores its context.
docs/fetch.md: a flight everyoneabandoned is not respawned at its deadline (nobody is left wanting the
bytes, so firing a fresh origin fetch would be pure waste); the stale entry
is evicted lazily by the next real caller. This replaces the
TestAbandonedWaitersReleasedAtDeadlinebehavior pinned during [Design Gap] fetch: unbounded background flight — one stalled origin poisons the cache key permanently (+ goroutine leak) #4.Regression tests
TestCancelledWaitersRetainNoGoroutines: 64 callers with 20 ms contexts on astalled 1-minute flight. After all callers return, at most 4 goroutines may
remain (the flight's single worker plus slack), and exactly 1 origin call
must have occurred. Verified to fail on the old code ("goroutines
retained after all 64 callers returned = 64, want <= 4").
TestStaleFlightReplacedOnlyByALiveCaller: after the sole caller times out,no replacement flight is led past the deadline (
calls == 1); the next realcaller evicts the stale entry and leads the replacement (
calls == 2).TestStaleFlightEvictedAfterMaxFlight,TestLateStaleLeaderKeepsReplacement,TestJoinerRetrySkippedWhenCallerGone,TestCoalescedResultIsMarked.Verification
All run with the repo toolchain (go1.27.0,
GOTOOLCHAIN=local, workspace-localcaches):
go vet ./...— cleango test ./... -race -count=1— all 13 packages okgo test ./... -cover -count=1— all ok;internal/fetch91.7%go test ./internal/fetch/ -race -count=20 -run 'Flight|Stale|Cancelled|Joiner|Coalescing'— 20 consecutive green rounds on the timing-sensitive tests
gofmt -l internal/ docs/— cleanDocs:
docs/fetch.md§3.5 rewritten for the one-worker-per-flight contract andlazy eviction; the test inventory table replaces
TestAbandonedWaitersReleasedAtDeadlinewith the two new pins.