registry: detach the token singleflight from the leader's context - #59
Merged
Conversation
The shared token exchange ran on the elected leader's request context, so cancelling that one caller failed the whole concurrent cold-auth cohort with context.Canceled — even followers whose contexts were still live. A short-lived first request could thus fail every concurrent pull of the same repository. Run the exchange on a bounded background flight context (1 minute) exactly like fetch.Coalescing: each caller's own context now bounds only how long that caller waits, the flight completes for the remaining waiters (and warms the cache even if none remain), and a stalled token endpoint cannot pin the cache key past the bound. Fixes #51
Collaborator
Author
|
@copilot please review this PR. Focus points:
Please do NOT push commits to this branch; post findings as review comments only. |
Contributor
Reviewed c6b086e. Findings: none.
No commits were pushed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #51
What
AuthTransport.fetchTokenSharedran the shared token exchange on the elected leader's request context. Cancelling that one caller failed the entire concurrent cold-authentication cohort withcontext.Canceled, even for followers whose contexts were still live — one short-lived first request could fail every concurrent pull of the same repository.The exchange now runs on a bounded background flight context (1 minute), exactly like
fetch.Coalescing:Triage evidence
Verified authentic against the exact commit the issue names (
84aac4e, current main):ctxis the leader's request context; followers wait onc.doneand return the sharedc.err, so a leader cancellation is published to every live follower. Reproduced deterministically before the fix:Classification: implementation defect — the deviation is from a contract the repo itself already established.
fetch.Coalescing(internal/fetch/fetch.go:457-463) runs shared flights on a bounded background context precisely so "one caller's cancellation does not abort it"; the registry singleflight contradicted that stance, anddocs/registry.mdalready cites the Coalescing semantics as the reason credentials cannot ride request contexts.Fixes
fetchTokenShared: the first caller now starts the flight (in a dedicated goroutine) and then waits onc.donelike every follower; each caller's ownctxbounds only its own wait.runTokenExchange: performs the exchange oncontext.WithTimeout(context.Background(), maxTokenExchange)(1 minute), publishes to the cache before removing the in-flight marker (the existing one-critical-section ordering is preserved), and closesdoneonly after the result fields are written and the lock is released — waiters get a happens-before edge on the result, nothing runs undera.mu.docs/registry.md: new contract bullet for the singleflight flight semantics + test-table rows for the new regression tests.No signature changes (
authorizeis the sole caller offetchTokenShared); no new dependencies; main modulego.sumstill empty.Regression tests
internal/registry/issue51_test.go, against a stub token transport that stalls mid-exchange and honors request-context cancellation like a real transport (that fidelity is what makes the tests fail on the old code):TestLeaderCancellationDoesNotPoisonFollowers— the issue's exact scenario: the elected leader is cancelled while the token endpoint is blocked; 4 followers with live contexts joined the same flight must still complete with the flight's token, the cohort must cost exactly one exchange, and the result must be cached. Looped 25 rounds across scheduling windows (same pattern asTestConcurrentColdRequestsShareOneExchange). Fails on the old code in round 0 (follower 0 poisoned ...: context canceled); passes after the fix.TestCancelledFollowerDoesNotAbortFlight— symmetric contract pin: a departing follower must not cancel the shared exchange either; the leader still completes and exactly one exchange occurred.Verification
All run in the fix worktree with the repo-pinned toolchain (go1.22.12):
And the named regression demonstrated failing pre-fix, passing post-fix under
-race.