fix: serialize Grok token refresh to stop concurrent invalid_grant races - #2
Merged
Conversation
GetAccessToken had no real locking around the actual refresh HTTP call: the mutex was only held long enough to copy fields, then released before the network round trip. When multiple requests arrived right at token expiry, each one read the same stale refresh_token and POSTed it to the provider concurrently. Since OAuth providers commonly rotate (single-use) refresh tokens, only one of those calls succeeds; the rest come back invalid_grant and the losing goroutines returned an error to their callers even though a sibling had just refreshed the credential successfully. Add a dedicated refreshMu that is held for the whole refresh operation, and after acquiring it re-check whether the entry is already fresh so a caller that lost the race for the lock just observes the token a sibling refreshed instead of hitting the provider again. ForceRefresh still always attempts a refresh (used after an upstream 401), but is now serialized against the same mutex so a burst of 401s does not also stampede the token endpoint. Added TestConcurrentGetAccessTokenDedupesRefresh, which simulates refresh token rotation and reliably failed against the old code (5/5 runs) before the fix.
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.
What
auth.Manager.refresh()claimed (via a stale comment) to serialize refreshes, but only held the mutex long enough to copy a few fields, releasing it before the actual HTTP round trip to the token endpoint. Concurrent callers hittingGetAccessTokenright at token expiry would each read the samerefresh_tokenand POST it to the provider at the same time.Why it matters
Most OAuth providers rotate (single-use) refresh tokens: only the first of two concurrent uses of the same refresh_token succeeds, the second comes back
invalid_grant. The goroutine that lost that race returned an error to its caller (token expired and refresh failed) even though a sibling goroutine had, moments earlier, successfully refreshed the credential the Manager now holds. Under any real concurrent load this shows up as spurious 503s from the proxy right at the token-expiry boundary, even though a valid token exists.ForceRefresh(used after an upstream 401) had the identical gap — a burst of 401s across concurrent requests could each trigger a forced refresh concurrently.How
refreshMu sync.Mutexheld for the full duration of the refresh operation (network call + state update), not just the field copy.ForceRefreshalways attempts a refresh (correct — it's called specifically because the current token was rejected), but now goes through the same mutex so it can't stampede the token endpoint either.Verification
New test
TestConcurrentGetAccessTokenDedupesRefreshsimulates a provider that rotates refresh tokens and fires 20 concurrentGetAccessTokencalls right at expiry. Verified it fails reliably (5/5 runs) against the pre-fix code before restoring the fix.Pre-existing, unrelated:
go test ./... -count=3panics ininternal/config(pflag.CommandLineflag re-registration across repeated in-process runs) — reproduces identically on unmodifiedmain, not touched by this change.