fix(core): single-flight the 401 token refresh in Requester - #636
fix(core): single-flight the 401 token refresh in Requester#636d-klotz wants to merge 8 commits into
Conversation
Sync stages drive records through one api-module instance in concurrent chunks, so an access token that lapsed overnight makes every request in the chunk 401 at once. Each one then refreshed independently, up to MAX_AUTH_RETRIES, which was actively harmful: the concurrent refreshes rotated the credential out from under each other (Intuit and others force-expire the previous refresh token), so the framework manufactured its own invalid_grant failures, and every request past the retry budget failed outright while a refresh was in flight and about to succeed. Measured on 5 concurrent 401s before this change: 3 refreshes, 2 self-inflicted invalid_grant, 4 of 5 requests rejected, and 4 INVALID_AUTH notifications flagging the credential dead even though a refresh had just succeeded. A single invocation could therefore reproduce the production signature with no cross-process race at all. Waiters now share the initiator's refresh via _refreshAuthOnce() and only the initiator spends the retry budget. Two guards come with it: - Re-entrancy. Subclasses issue their token request through this._post (OAuth2Requester.refreshAccessToken / getTokenFromClientCredentials), which re-enters _rawRequest. A 401 from the token endpoint itself — invalid_client, i.e. a revoked or rotated secret — would join the in-flight refresh and await the very promise whose resolution depends on that nested call returning: a circular await that hangs until the function times out, never flags the credential, and poisons the slot for every later request on the instance. An AsyncLocalStorage context marks the refresh flow so a 401 raised inside it is fatal, without needing subclass cooperation. - Token identity. A request dispatched before a refresh can have its 401 land after that refresh finished, so the current token was never tried. _authGeneration lets such a request retry with the new token instead of spending another rotation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
✅ Deploy Preview for friggframework-org canceled.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e34c9c0bf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // cannot help, and joining the in-flight refresh would await | ||
| // the promise this very call has to settle. Fail fast so the | ||
| // credential gets flagged and the slot is released. | ||
| if (refreshContext.getStore()?.inRefresh) { |
There was a problem hiding this comment.
When a custom refreshAuth() calls a different Requester instance—for example, a credential-broker client—the nested call inherits this module-level AsyncLocalStorage value. A 401 from that second requester therefore enters this branch and immediately invalidates its credential instead of using its own refresh or grace-retry path, even though awaiting its refresh would not create the circular dependency this guard addresses. Store the initiating requester in the context and apply the fatal path only when it matches this.
Useful? React with 👍 / 👎.
|
|
||
| // Which credential generation this attempt is carrying. If a concurrent | ||
| // request refreshes before our response lands, a 401 here is stale and | ||
| // must not trigger another refresh. |
There was a problem hiding this comment.
For an asynchronous addAuthHeaders() override that captures the current token and then awaits, another request can finish refreshing while this call is suspended. The returned headers then contain the old token, but this assignment records the new generation; when that request receives a 401, the stale-response check does not recognize it and starts another refresh, recreating the redundant token rotation this change is intended to prevent. Capture the generation atomically with the credential used to construct the headers rather than after the await.
Useful? React with 👍 / 👎.
…ng it Implements ADR-031 option 4 (frigg PR #637), the cross-invocation half of the refresh race. Separate Lambda invocations hydrate the same credential, refresh from stale memory, and the loser gets invalid_grant — then flags the shared credential invalid, which silently killed 95 records in a production app over four months. refreshAuth() now asks its delegate for the stored credential (new DLGT_CREDENTIAL_RELOAD, handled read-only by Module.reloadCredential) and adopts it when the stored refresh_token differs from the in-memory one: another invocation already refreshed, so adopting always beats spending a rotation. The comparison is keyed on the refresh token, never the access token — a provider can rotate one and return the other unchanged (review finding on the ADR). Escalation is now restricted to proof of death: a definitive authorization rejection (invalid_grant / invalid_client) AND no newer token appearing across a bounded re-read backoff (default 500/1000/ 1500ms; the observed winner-write-to-loser-failure gap in production was 716ms). Transport failures — timeouts, 429s, 5xx, unmarked 400s — now rethrow as retryable instead of invalidating a healthy credential (second review finding). Two counters expose the outcome: frigg.auth.refresh_race_recovered and frigg.auth.refresh_race_lost. Four existing oauth-2 tests encoded the old any-error-invalidates contract and were updated to the new one; their original intents (no secret leak to delegates, no retry after a failed refresh) are preserved and still asserted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| } | ||
| // In-flight requests that 401ed against the old token retry with | ||
| // this one instead of triggering another refresh. | ||
| this._authGeneration++; |
There was a problem hiding this comment.
Double generation bump bug. When _adoptNewerCredential() successfully adopts a newer credential, it bumps this._authGeneration++ here (line 438), returns true, which causes refreshAuth() to return true, which then triggers another bump in _refreshAuthOnce() at line 525 of requester.js. This results in the generation being incremented twice for a single credential adoption.
Fix: Remove the generation bump from _adoptNewerCredential(). The generation should only be bumped in _refreshAuthOnce() after any successful credential change (whether through adoption or actual refresh):
// Remove line 438:
// this._authGeneration++; // DELETE THIS LINE
this.telemetry?.count?.('frigg.auth.refresh_race_recovered', 1, {
module: this._telemetryModuleLabel(),
});
return true;The single bump in _refreshAuthOnce() (line 525) will correctly increment the generation for both adoption and refresh cases.
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Four defects found by the two-axis review, all verified against source before fixing: 1. The credential reload was dead code. reloadCredential required a nested `data` key, but every findCredentialById adapter returns the decrypted token fields spread at the top level. The reload always returned null, so adoption never ran. The tests hid it by mocking the nested shape. Fixed to accept both shapes; tests now mock the real adapter shape and keep one nested-shape case. 2. Rejection detection failed in production. FetchError sanitizes the response body outside dev, so a real invalid_grant carried no marker and was classified as transport — a dead credential would never invalidate. The status code now decides: 400/401 from the token endpoint is definitive (RFC 6749 §5.2), 429/5xx is always transport even when the body text mentions invalid_grant, and the word-bounded marker fallback remains only for statusless SDK errors. 3. The transport rethrow could leak the refresh request body (which carries client_secret in dev-stage FetchError messages). Transport failures now rethrow a fresh error carrying only the status code. 4. The silent-ack companion was missing from this PR even though the ADR scopes it into PR A. New checkIntegrationRunnable in backend-utils: ERROR now rejects the message so SQS retries and DLQs; DISABLED and IN_DELETION keep the intentional silent ack; FRIGG_LEGACY_ERROR_ACK=true is the kill switch. One worker test encoded the old ERROR-silent-ack behavior — the incident amplifier — and now asserts the new contract, plus a kill-switch case. Also: the recovered-race counter now fires only on a real recovery (adoption after a rejection), not on pre-refresh adoption. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…straction Comments now carry only the non-obvious why; the narration that restated the PR body is gone. The dense promise chain in _refreshAuthOnce() splits into _runMarkedRefresh(), the ALS check gets a name (_isInsideRefreshFlow), the 401 branch names its condition (tokenReplacedWhileInFlight), and the refreshAuth() catch block delegates to _transportFailureError() and _adoptNewerCredentialWithBackoff(). No behavior change; all 111 touched tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
The queue-worker change (ERROR throws instead of silently acking, with the FRIGG_LEGACY_ERROR_ACK kill switch) is independent of the requester refresh work and is the one default-on behavior change affecting every app — the ADR-027 exception a maintainer must ratify on its own terms. It will return as its own small PR; the implementation stays recoverable in this branch's history (0b28b4a and earlier). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…heck Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| this.access_token = stored.access_token ?? this.access_token; | ||
| this.refresh_token = stored.refresh_token; |
There was a problem hiding this comment.
Token mismatch bug: The code creates mismatched credential pairs when adopting a newer credential. Line 447 falls back to the old access_token if the stored one is missing (?? operator), but line 448 unconditionally adopts the new refresh_token. This results in pairing an old access token with a new refresh token, which are from different credential rotations and won't work together.
If the stored credential legitimately has a missing access_token, the code should either:
- Reject the adoption entirely (return false), or
- Accept the missing token as-is and let the next request fail fast
Fix by removing the fallback:
this.access_token = stored.access_token;
this.refresh_token = stored.refresh_token;Or add validation before adoption:
if (!stored?.access_token || !stored?.refresh_token) return false;
this.access_token = stored.access_token;
this.refresh_token = stored.refresh_token;| this.access_token = stored.access_token ?? this.access_token; | |
| this.refresh_token = stored.refresh_token; | |
| if (!stored?.access_token || !stored?.refresh_token) return false; | |
| this.access_token = stored.access_token; | |
| this.refresh_token = stored.refresh_token; | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
The fresh.data fallback guarded a shape no adapter produces — all three findCredentialById implementations spread the decrypted token fields at the top level — so it is gone, along with its speculative test. The result variable now says where the row comes from, and the doc states what a null return means to the requester. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses the Graphite findings: _adoptNewerCredential no longer bumps _authGeneration (the slot in _refreshAuthOnce owns the bump, so one adoption bumps exactly once — the reload test now asserts through the slot and would fail on +2), and the access-token copy is a plain guard instead of a ??-fallback. The keep-old-token semantics stay: rejecting an adoption over a missing access_token would leave the recovery inert for modules that do not persist one, and access and refresh tokens are presented independently — a dead old access token costs one extra 401, which then refreshes with the adopted refresh token. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
1 participant
Problem
Sync stages drive records through one api-module instance in concurrent chunks. When an access token has lapsed (the overnight case), every request in the chunk 401s at nearly the same moment — and each one then refreshed independently, up to
MAX_AUTH_RETRIES.That was actively harmful, not just wasteful. The concurrent refreshes rotate the credential out from under each other — Intuit and others force-expire the previous refresh token — so the framework manufactures its own
invalid_grantfailures.Measured on 5 concurrent 401s against one instance, before this change:
refreshAuth()callsinvalid_grantINVALID_AUTHnotificationsThose 4 notifications flag the credential dead even though a refresh had just succeeded. A single invocation can therefore reproduce the production
invalid_grantsignature with no cross-process race at all.This was found while investigating a customer incident where an Aspire → QuickBooks integration silently orphaned 95 records over four months.
Change
Waiters share the initiator's refresh via
_refreshAuthOnce(), and only the initiator spends the retry budget.Two guards come with it:
Re-entrancy — this one is a deadlock, and it is why the naive fix is not enough. Subclasses issue their token request through
this._post(OAuth2Requester.refreshAccessToken/getTokenFromClientCredentials), which re-enters_rawRequest. A 401 from the token endpoint itself (invalid_client— a revoked or rotated secret) would join the in-flight refresh and await the very promise whose resolution depends on that nested call returning. Circular await: it hangs until the function times out, never flags the credential, and poisons the slot for every later request on the instance. AnAsyncLocalStoragecontext marks the refresh flow so a 401 raised inside it is fatal, with no subclass cooperation required.Token identity. A request dispatched before a refresh can have its 401 land after that refresh finished, meaning the current token was never tried.
_authGenerationlets it retry with the new token instead of spending another rotation — and every avoided rotation is one less invalidation of the pair a concurrent holder just minted.Tests
requester.concurrent-refresh.test.js, 10 tests. The first four fail onnextwith exactly the signature in the table; the deadlock test hangs >1500ms before the fix and settles in 2ms after.Also covers the safety properties the retry budget existed to provide, so this cannot silently weaken them: the slot releases for a genuinely later 401, the budget still bounds a pathological upstream that 401s after a "successful" refresh, and all waiters reject when the shared refresh fails.
Full requester suite: 69 passed, 2 pre-existing skips.
Notes for review
.finally()must stay last in_refreshAuthOnce()— the stored promise has to be the one that clears the slot, so it is released before any waiter's continuation resumes.refreshAuthdoes its own HTTP outside_rawRequest(e.g.intuit-oauth) bypass Frigg's per-attempt timeout, and single-flight now concentrates that exposure — a hung token endpoint pins the initiator and every waiter. Worth a follow-up.🤖 Generated with Claude Code