Make auth provider token caches thread-safe (#1595)#1708
Open
tarekgh wants to merge 2 commits into
Open
Conversation
ClientOAuthProvider and IdentityAssertionGrantProvider lazily populated mutable cache fields during 401 handling with no synchronization. Under concurrent in-flight requests on a shared handler this caused redundant token exchanges and races on the cache fields. Coalesce token acquisition in both providers behind a SemaphoreSlim so only one caller performs the exchange or refresh while others await and reuse the result. Valid cached tokens still take a lock-free fast path. Thread the token that caused a 401 through so a stale-but-unexpired token still forces a real refresh. Guard InvalidateCache and document the concurrency contract on ITokenCache. Adds a deterministic concurrency test verifying a single exchange runs for multiple concurrent callers. Copilot-Session: 334e4d21-b783-46ae-8dc6-3c3b5defbe37
Contributor
Author
|
CC @jeffhandley |
) The post-lock cache re-check only reused a freshly cached token when the challenge carried a token (usedAccessToken is not null). During a cold-start connect the client can issue concurrent requests that all send no token, so each saw a null usedAccessToken, skipped the re-check and ran a full interactive authorization, producing redundant token exchanges (observed in CI as two authorization requests on a single connect). When no token was sent, any valid token now present in the cache was obtained by another caller and is safe to reuse, so drop the null guard. The remaining checks still prevent replaying a rejected token (the cached token must differ from the one that produced the challenge) and still run the step-up flow for 403 insufficient_scope challenges. Copilot-Session: 334e4d21-b783-46ae-8dc6-3c3b5defbe37
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
1 participant
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 #1595.
Problem
ClientOAuthProviderandIdentityAssertionGrantProviderlazily populated mutable cache fields (tokens, server metadata, resolved endpoints) during 401 handling with no synchronization. When the same provider handles concurrent in-flight requests, this caused redundant token exchanges and races on the cache fields.Fix
SemaphoreSlimso only one caller performs the exchange/refresh while others await and reuse the result.InvalidateCacheand document the concurrency contract onITokenCache.Tests