Repository navigation
fix(auth): compare-and-swap guard on OAuth refresh persist (#4038) - #5294
Merged
Merged
Conversation
The per-connection mutex makes [refresh + persist] atomic for one connection, but a third writer (sibling request / HealthCheck / replica) can land a fresher refresh_token rotation between the staleness read and the persist. Overwriting it reverts the rotation → the next caller loads a consumed token → Auth0/Anthropic revoke the token family (the 1352x claude/aa5dd5cf invalidation storm). getAccessToken now re-reads the row's current refresh_token immediately before persisting (inside the mutex) and skips the write when it rotated past the presented token. The caller still receives the new access token; only the DB overwrite is skipped. Opt-in via runWithCasGuard (no guard => byte-identical); skip/persist counters via getCasGuardStats(). The comparison reuses wasRefreshTokenRotated. chatCore wires the reread via getProviderConnectionById. Regression test: token-refresh-cas-guard-4038.test.ts (3 cases, TDD: fails without the skip). Co-authored-by: KooshaPari <KooshaPari@users.noreply.github.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
diegosouzapw
added a commit
that referenced
this pull request
Jun 29, 2026
tkgo11
pushed a commit
to tkgo11/OmniRoute
that referenced
this pull request
Sep 23, 2026
…nference#4038) (cheaper-inference#5294) Integrated into release/v3.8.40
tkgo11
pushed a commit
to tkgo11/OmniRoute
that referenced
this pull request
Sep 23, 2026
…ve value (cheaper-inference#5166) (cheaper-inference#5304) Integrated into release/v3.8.40 (CHANGELOG re-resolved against post-cheaper-inference#5294 tip; code identical to the green commit)
tkgo11
pushed a commit
to tkgo11/OmniRoute
that referenced
this pull request
Sep 23, 2026
cheaper-inference#5231) (cheaper-inference#5303) Integrated into release/v3.8.40 (CHANGELOG re-resolved against post-cheaper-inference#5294/cheaper-inference#5304 tip; kiro code identical to the green commit)
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.
Summary
Closes part of #4038 (the credential-loss failure mode). Implements the compare-and-swap (CAS) guard on the OAuth refresh persist that #5292 described but did not actually contain — #5292 was closed as unmergeable (a 2,333-file diverged-fork diff with no CAS code in its tree). Full credit to @KooshaPari for the root-cause diagnosis; carried forward here as co-author.
Problem
The per-connection refresh mutex makes
[network refresh + DB write]atomic for one connection, but it does not protect against a third writer (a sibling request, a concurrent HealthCheck, or a replica) landing a fresherrefresh_tokenrotation on the sameconnection_idbetween the staleness read and the persist. Overwriting that fresher row reverts the sibling's rotation; the next caller loads the now-consumed token, Auth0/Anthropic flagrefresh_token_reused, and the whole token family is revoked (the 1352× claude/aa5dd5cfinvalidation storm).Fix
runWithCasGuard/getActiveCasGuardAsyncLocalStorage pair (mirrors the existingrunWithOnPersistFix-A pattern). The guard carries therefresh_tokenthe caller presented (the natural CAS version token, since refresh_tokens rotate every refresh) plus arereadof the row's current refresh_token.getAccessTokenre-reads viacasGuardShouldSkipPersistimmediately beforeeffectiveOnPersist; if a concurrent writer rotated the row past the presented token (wasRefreshTokenRotated), the persist is skipped so the DB stays at the fresher state. The caller still receives the new access token — upstream already authenticated the request.chatCore.tsbuilds therereadfromgetProviderConnectionById(connectionId)and wraps the reactive 401/403 refresh inrunWithCasGuard.getCasGuardStats().Note: the PR body of #5292 also mentioned a "Bifrost outcome mutex split" — that targets
bifrostShadow.ts, which is part of the fork's bifrost architecture and does not exist in OmniRoute, so it is N/A here.Validation (TDD)
tests/unit/token-refresh-cas-guard-4038.test.ts— 3 cases driving the realgetAccessTokenwith a mocked Anthropic token endpoint + an injected reread:getCasGuardStats().skipped === 1, caller still gets the access token;persisted === 1;Fails without the skip (verified).
npm run typecheck:coreclean; the existing 55 token-refresh tests (race-comprehensive + service + this) pass;check:file-sizeOK (baseline bumped with justification).