feat(mcp): shared OAuth token foundation - challenge, store seam, expiry-aware cache, single-flight refresh - #31275
Merged
tin-berri merged 11 commits intoJun 27, 2026
Conversation
Open
4 tasks
Contributor
Greptile SummaryThis PR adds shared OAuth token infrastructure for MCP outbound credentials. The main changes are:
Confidence Score: 4/5Medium risk: the changes touch authentication error shaping and token caching/refresh semantics, but the foundation is isolated and covered by focused tests. The implementation is not production-wired yet, which limits immediate blast radius, and tests cover challenge mapping, cache expiry, invalidation, outage behavior, refresh coordination, and concurrency paths. No files require follow-up from the completed review.
 T-Rex LogsWhat T-Rex did
Reviews (6): Last reviewed commit: "fix: reread oauth token before refresh" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
tin-berri
force-pushed
the
litellm_mcp_v2_oauth_token_foundation
branch
from
June 25, 2026 04:36
57bbf5a to
8cceb45
Compare
Contributor
Author
tin-berri
changed the title
tin-berri
force-pushed
the
litellm_mcp_v2_oauth_token_foundation
branch
2 times, most recently
from
June 25, 2026 16:40
6b2a061 to
c7cfd84
Compare
Contributor
Author
|
@greptileai re-review — the full |
The unauthorized case becomes a structured Unauthorized (detail + optional WWW-Authenticate header + optional structured body) instead of a bare string, and raise_public emits the header and body when present. This lets a mode reproduce a rich 401 challenge (e.g. BYOK's provisioning prompt) through the generic resolver edge. of_unauthorized's new params are keyword-only and default to None, so existing callers and the summary string are unchanged.
…t flat CredError's unauthorized payload was a pydantic BaseModel, whose base resolves as unknown in this repo's basedpyright (every model in the file trips reportUntypedBaseClass plus an unknown model_config), so the tagged-union case read as unknown and the public edge's challenge access added reportUnknownMemberType errors over the per-rule ceiling. A frozen dataclass is fully typed here, so error.unauthorized resolves directly with no cast or accessor and the per-rule basedpyright counts match base.
…ion_code Lay the foundation for the authorization_code resolver arm: OAuthToken (access_token, expires_at, refresh_token), the OAuthTokenStore Protocol seam, TokenStoreUnavailable for outages, and CachedOAuthTokenStore, an expiry-aware cache that serves a token only while unexpired, caches the "not authorized" None for a default TTL, and propagates a store outage without caching it. Mirrors the BYOK store/cache pattern, adapted for tokens. Refresh and distributed single-flight are deferred to the hardening step.
Add TokenRefresher (a mode-supplied seam: mint a fresh token from an expired one and persist it) and RefreshingTokenStore: when the stored token is near expiry, the first caller refreshes while concurrent callers await the same in-flight task and share its result, so the IdP is not stampeded. The task self-cleans (a done-callback drops its entry), so the map is bounded by in-flight refreshes rather than by distinct users/servers, and is detached from the caller so a cancelled caller does not abort the refresh. An expired token the refresher cannot renew surfaces as None so the arm challenges, never a stale bearer; it composes under CachedOAuthTokenStore. OAuthToken's repr masks the access/refresh tokens so a stray log cannot leak them. Cross-replica single-flight (Redis) and reactive-401 refresh are the later distributed hardening.
…ports in the token modules
… refresh_token docstring
…aching) CachedOAuthTokenStore no longer caches the "not authorized" None result; every miss re-reads the inner store. v1's per-user token cache never caches misses, so a token written by the OAuth flow is visible on the next request without an invalidation hook, and uniformly across replicas since the in-process cache holds no stale None to clear. invalidate() now only covers rotation or revocation of a cached token. Negative caching (with distributed invalidation) can return later if a slow DB-backed v2-native source makes per-miss reads expensive.
The proactive token-refresh / cache-expiry buffer defaulted to 30s, which is an outlier among OAuth clients. Spring Security uses 60s as both its JWT clock-skew tolerance and its refresh buffer, and 60s sits inside RFC 7519's "a few minutes" leeway while preserving nearly all of a typical token's life; 30s was untested, so pin the default with two boundary-probe regression tests.
tin-berri
force-pushed
the
litellm_mcp_v2_oauth_token_foundation
branch
from
June 26, 2026 00:23
81c7402 to
54414ff
Compare
Contributor
The refresh seam took only the OAuthToken, but a refresher needs the server's config (token endpoint, client credentials, scopes) to run the grant and the (user_id, server_id) key to persist the minted token, neither of which is derivable from the token. Widen TokenRefresher.refresh to (user_id, server_id, token) and pass them through from RefreshingTokenStore so each stacked mode PR plugs into the final seam rather than forcing a later signature change across the stack.
…replica token caching) Make CachedOAuthTokenStore's storage and RefreshingTokenStore's single-flight injectable so a cross-replica deployment can back them with Redis without touching the resolver. The defaults preserve today's behavior exactly: InMemoryTokenCacheBackend (the bounded per-process dict) and InProcessRefreshCoordinator (the asyncio single-flight). A distributed deployment injects a shared DualCache-backed backend and a SET NX PX coordinator. invalidate() is now async (the backend may be). The cache stores via the backend with a TTL derived from the token's expiry; the coordinator threads a reread callback for the cross-replica case (losers re-read the persisted token) that the in-process default ignores.
Contributor
Contributor
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Stale token duplicate refresh race
- RefreshingTokenStore now re-reads the persisted token immediately before leading a refresh and skips the IdP call when another caller already stored a fresh token.
You can send follow-ups to the cloud agent here.
|
|
Contributor
|
bugbot run |
Contributor
Contributor
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 66a33c9. Configure here.
mateo-berri
previously requested changes
Jun 27, 2026
mateo-berri
left a comment
mateo-berri
left a comment
Contributor
There was a problem hiding this comment.
If you could verify whether
- the autofix did things incorrectly and we should force push it back, or
- it's fine we should bundle it in
then I'm happy to approve. Otherwise LGTM!
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
4 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.
Relevant issues
Part of the MCP v2 outbound-credential migration. The shared foundation for the OAuth2 / per-user modes of the v2 resolver: the 401-challenge mechanism, the OAuth token model + store seam, the expiry-aware cache, and proactive single-flight refresh. Stacked PRs build each oauth2 mode (authorization_code, client_credentials, token_exchange) on top of this, so the cache/refresh machinery lives once rather than being copied per mode
Scope: this PR vs #31474 (cross-replica)
Both this PR and #31474 say "single-flight refresh" — they are the two halves of the same machinery, and it is worth being explicit about the seam:
TokenCacheBackend,RefreshCoordinatorProtocols) and the behavior decorators (CachedOAuthTokenStore,RefreshingTokenStore), and ships the in-process fillers:InMemoryTokenCacheBackendandInProcessRefreshCoordinator. Its single-flight is per-process — concurrent coroutines within one worker share one refresh. Self-contained and fully correct for a single replica. Depends on nothing below it.RedisRefreshCoordinator/RedisDistributedLock,DualCacheTokenCacheBackend) that replace the in-process defaults via the composition root when Redis is present. Its single-flight elects one worker in the whole fleet, so a rotating IdP doesn't hand every other replica aninvalid_grant. feat(mcp): cross-replica single-flight refresh for the v2 per-user OAuth store [2/2] #31474 importsRefreshCoordinator/TokenCacheBackend/CachedOAuthTokenStore/RefreshingTokenStorefrom this PR — it cannot exist without it, which is why it is stacked on top.In one line: #31275 defines the slots and fills them for a single process; #31474 fills the same slots for a multi-replica fleet. (#31473 sits between them, wiring
authorization_codeonto these seams using the in-process defaults.)Linear ticket
N/A (groundwork for MCP V2)
Pre-Submission checklist
make test-unitType
🆕 New Feature
Changes
Shared infrastructure for the oauth2 resolver modes, so each mode PR plugs in only a token source, a refresh action, and its arm, never re-touching the cache or the cross-replica wiring:
CredError.of_unauthorizedcarries a 401 challenge (theUnauthorizedfrozen dataclass with an optionalWWW-Authenticateheader and a structured body), emitted byraise_publicOAuthToken(access_token / expires_at / refresh_token), theOAuthTokenStoreProtocol seam, andTokenStoreUnavailable(an outage is propagated, never cached as "not authorized")CachedOAuthTokenStore: an expiry-aware cache that serves a token only while it is unexpired (minus a skew) and never caches a "not authorized" miss, so a token written after the OAuth flow is visible on the very next call (matching v1); storage sits behind the injectableTokenCacheBackendseam, whose defaultInMemoryTokenCacheBackendis the bounded per-process dictTokenRefresher(mode-supplied seam overuser_id/server_id/ token, so the refresher has the server config to run the grant and the(user_id, server_id)key to persist under) plusRefreshingTokenStore: when the stored token is near expiry, mint a fresh one serialized per(user, server)by the injectableRefreshCoordinatorso concurrent callers share one refresh; an unrefreshable expired token surfaces as None so the arm challenges, never a stale bearerRefreshCoordinatorseam with the defaultInProcessRefreshCoordinator(asyncio single-flight, self-cleaning so the in-flight map stays bounded by concurrent refreshes); a cross-replica deployment injects a Redis SET NX coordinator without touching the resolver, and the coordinator threads arereadcallback so a distributed loser re-reads the persisted token (the in-process default ignores it)Not in this PR: the per-mode arms, sources, mappings, and wiring (each a stacked PR). The concrete cross-replica backend and coordinator (DualCache / Redis SET NX) land in #31474; this PR only carves out their injection points, with the in-process defaults preserving today's behavior exactly. Reactive-401 refresh is later hardening and lives in the egress transport, which sees the upstream's 401
Screenshots / Proof of Fix
Foundation only; no runtime behavior change. Exercised by the unit tests (the challenge mapping; cache TTL / expiry / miss-never-cached / invalidate / outage / backend delegation; refresh plus single-flight under concurrency; coordinator delegation; repr masking) and by the stacked mode PRs
Note
Medium Risk
Touches authentication credential handling and token refresh concurrency, but ships as isolated foundation with in-process defaults and no mode wiring until stacked PRs land.
Overview
Adds shared MCP v2 outbound OAuth plumbing: per-user
OAuthTokenmodel,OAuthTokenStore/TokenRefresherseams, expiry-awareCachedOAuthTokenStore(positive hits only; never cache “not authorized” or store outages), andRefreshingTokenStorewith injectableRefreshCoordinator/TokenCacheBackendplus in-process defaults (bounded in-memory cache, asyncio single-flight refresh with a re-read-before-refresh guard against duplicate IdP refreshes).401 challenges are richer:
CredError.of_unauthorizednow carries anUnauthorizedpayload (optional structuredbodyandWWW-Authenticate), andraise_publicmaps that ontoHTTPExceptionheaders/detail instead of a plain string.Broad unit coverage for cache TTL/skew, miss behavior, invalidate, concurrency, coordinator/backend injection, and challenge HTTP mapping. Per-mode resolver wiring and Redis cross-replica fillers are explicitly out of scope here.
Reviewed by Cursor Bugbot for commit 66a33c9. Bugbot is set up for automated code reviews on this repo. Configure here.