Make auth provider token caches thread-safe (#1595)#1708
Merged
tarekgh merged 4 commits intoJul 22, 2026
Merged
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
halter73
reviewed
Jul 21, 2026
added 2 commits
July 21, 2026 13:33
- Remove redundant _scopeAccumulatorLock; _tokenAcquisitionLock already serializes all reads and writes to _accumulatedScopes and _hasAttemptedStepUp. - On a concurrent 403 where a step-up already ran and the challenge adds no new scope, reuse a still-valid token produced by another caller instead of failing. - Capture _cachedTokens into a local on the IdentityAssertionGrantProvider lock-free fast path to avoid a null-deref race with InvalidateCache. - Add a regression test for the concurrent step-up token reuse.
Contributor
Author
|
@halter73 I have addressed all feedback. Please let me know if you have more or we are good to go. Thanks! |
halter73
approved these changes
Jul 22, 2026
halter73
added a commit
that referenced
this pull request
Jul 22, 2026
…sted On a cold start where a durable ITokenCache returns a persisted refresh token but no client ID has been assigned yet (DCR/CIMD clients), GetAccessTokenCoreAsync attempted a token refresh before assigning a client ID, causing CreateTokenRequest -> GetClientIdOrThrow() to throw 'Client ID is not available'. Persist the client registration (client ID, secret, token endpoint auth method) alongside the tokens so a single durable ITokenCache can use the refresh token after a restart, and restore it on a cold start before the refresh attempt. Also guard the refresh block with a client-ID check so the flow falls through to client-ID assignment and the authorization-code flow instead of throwing when no credentials are available. The restore and persistence run inside the provider's _tokenAcquisitionLock (added in #1708), so the shared credential fields are written under the same lock that serializes the rest of the auth state; no new races are introduced. Fixes #1658 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
2 participants
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