ADR-031: Concurrent OAuth Credential Refresh Across Lambda Invocations - #637
Conversation
A production Frigg app silently orphaned 95 records over 4 months. The trigger was a cross-invocation credential rotation race: separate Lambda invocations each hydrate the credential into memory, refresh without re-reading it, and persist with a blind last-writer-wins upsert. The loser presents an already-rotated refresh token, gets invalid_grant, flags the shared credential invalid, and the queue worker then silently acks every subsequent message. PR #636 shipped a per-instance single-flight refresh; it cannot reach a race between invocations. No existing ADR covers refresh concurrency (nearest: ADR-005 names token refresh as an admin utility, ADR-006 defines credential endpoints, ADR-009 flags refresh-on-401 as a test gap). The proposed decision is a layered stack: failure containment in the invalidation and worker paths, loser recovery by re-read-and-adopt, compare-and-swap on the credential write, core-owned token expiry with a narrow pre-flight gate, declared per-module rotation semantics, and optional jitter. Provable serialization (SQS FIFO per credential) is deferred behind a named metric. The maintainer's three candidate designs (central token service, DB-event subscription, per-API re-read flag with a dual-write cache) are evaluated in Alternatives Considered; the re-read half of the third is adopted, the rest rejected on mechanism. Status is Proposed. One open question is named for ratification, plus the default-on exception to ADR-027's opt-in bar. Numbered 031 because 028-030 are claimed by drafts on unmerged branches. 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.
✅ 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: f24409b596
ℹ️ 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".
| adopted token is itself rejected on the retry. Either one means nobody holds | ||
| a valid token — this is no longer a lost race, it is a genuinely dead | ||
| credential (user revoked access, or a whole-grant-revoking provider killed | ||
| the winner's tokens too; see "Provider semantics"). At that point the | ||
| original behavior is correct and must run: `markCredentialsInvalid()`, a |
There was a problem hiding this comment.
When the token endpoint fails with a timeout, network error, 429, or 5xx, the re-read will legitimately find no newer token, but that does not mean the credential is dead. The current OAuth2Requester.refreshAuth() funnels every thrown error through the same failure path (oauth-2.js:317-330), so this escalation rule would mark a healthy credential invalid and put its integrations into ERROR during a provider outage. Restrict invalidation to definitive authorization rejections such as invalid_grant, while leaving transport and provider failures retryable.
Useful? React with 👍 / 👎.
| `invalid_grant` after failing, re-read the credential; if the persisted | ||
| `access_token` differs from the in-memory copy, adopt it, bump | ||
| `_authGeneration`, and retry via `requester.js:379`. The datastore is the |
There was a problem hiding this comment.
For a rotating provider that returns the same access-token value while issuing a new refresh token, this test misses the winner even though layer 1 correctly detects the changed refresh_token; the loser then follows the adoption-failure path and invalidates a usable credential. OAuth does not make access-token string inequality a refresh-generation invariant, so adoption should be keyed to the changed refresh token or the proposed tokenVersion, and should copy the complete persisted token state.
Useful? React with 👍 / 👎.
Same decision, same facts, same code citations, same numbers. Only the language changes: short sentences, active voice, one idea per sentence, and a terms list at the top. A checklist pass verified that every code reference, measurement, and count from the previous revision is present in the new text. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
seanspeaks
left a comment
seanspeaks
left a comment
There was a problem hiding this comment.
ADR is too verbose by 60%+ (rough estimate)
The problem can be stated pretty succinctly... Frigg's API modules and requester approach currently does not have any way to deconflict when parallel processes run into auth refresh errors for any OAuth or session based auth. That leads to multiple parallel processes refreshing credentials in-memory and then whichever was the "last write" wins in the DB.
In most cases, this is a non-issue when the API provider allows for either multiple live access tokens, or multi use for a given refresh token.
In some cases, it's an acute issue. Either generating an access token invalidates all prior ones; or refresh tokens are single use.
^^ we can get into more details by pointing to the parts of the code where this happens or doesn't happen, ultimately in the Requester class and the api class.
From this we have some options.
- We can have each invocation/process/thread "listen" for changes to the specific database table/models upon connection. Any changes will then propagate to the in-process instance of the api class, swapping out or referencing the newly refreshed credential details
- We can proactively refresh using the new admin scripts with optional core scripts that can run on a cron
- We can pre-flight freshly fetch a credential, either naively (every time), or in a specific context (i.e. when it's known this is a parallel request, a flag gets added/passed into the api class instance)
- We can reactively check against the DB first before we attempt an auth refresh or retry (on 401).
Option 1 feels like it adds too much overhead and may be brittle. Alternatively, it's a good pattern broadly to consider and maybe use in Frigg with distributed workloads.
Option 2 seems like we should build it anyway and have it be automatically installed or offered to be installed via CLI whenever someone installs an API module that has expiritng OAuth tokens with refresh. It won't solve everything but it likely would make the problem more rare for a low cost.
Option 3 feels like we'd incur too much overhead for each request, essentially flooding requests to the DB.
Option 4 feels like the right answer, with the caveat noted below.
1, 3 and 4 also would need to consider delays to the DB read/writes... i.e. maybe it takes an extra 2-5 seconds for a db write to percolate? Depending on how heavily used the DB is?
Verdict/recommendation is Option 2 and 4
From there we can dig into the architecture and considerations.
Responds to the review on PR #637 (changes requested): - Cut from 1027 to 337 lines (~67%). - Restructured around the review's four options. The decision is options 4 and 2 together: a reactive database check before refresh and on 401 as the mechanism, and proactive scheduled refresh via the admin scripts as the companion. - Folded in both inline review findings: the adoption test keys on the refresh token (not the access token), and only definitive authorization rejections may invalidate a credential — transport failures (timeout, 429, 5xx) stay retryable. - Addressed the database-propagation caveat with the measured 716ms window, bounded re-read backoff, and the readPreference=primary requirement. - Compare-and-swap moved from load-bearing to deferred hardening. The serialization family stays deferred behind the named metric. - The full prior analysis remains available at commit 42d1e6f. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 scheduled-refresh companion no longer requires core changes. The script hydrates each module and calls refreshAuth() serially; the existing setTokens -> DLGT_TOKEN_UPDATE -> onTokenUpdate -> upsertCredential chain persists the result with no new code. Expiry tracking (the persist/hydrate work and the null-expires_in computation fix) moves from prerequisite to optional later optimization, taken only if the bounded waste of blind refreshes ever matters. Option 4 is unchanged: the database lookup before any refresh, and the re-read-and-adopt on 401/invalid_grant, remain the mechanism and ship first. The sequence shrinks from three PRs to two. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
…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>
|
🚀 PR was released in |
|
🚀 PR was released in |
2 similar comments
|
🚀 PR was released in |
|
🚀 PR was released in |
2 participants
What this is
A Proposed ADR. It decides how Frigg handles concurrent OAuth credential refresh across separate Lambda invocations. It needs maintainer ratification, not merge-on-green.
Why now
A production Frigg app (Aspire → QBO) silently orphaned 95 records over 4 months. Verified root cause, with log and code evidence in the ADR's Context section:
invalid_grant.ERROR→ the queue worker silently acked every later message. No DLQ entry. That amplifier, not the race itself, is what turned 3 failed invoices into 95 lost records.PR #636 (per-instance single-flight refresh) ships the in-process half. It cannot reach a race between invocations — this ADR is the cross-invocation half.
Prior art check
No existing ADR covers refresh concurrency. Nearest: ADR-005 (token refresh as an admin utility example), ADR-006 (credential endpoints), ADR-009 (flags refresh-on-401 as a low-coverage test gap). ADR-003 does not constrain this — it is scoped to the local-dev GUI.
The decision, in one paragraph
A layered stack, no new AWS infrastructure: (1) failure containment — a lost race is retryable, only genuinely dead credentials invalidate, and the worker's
ERRORpath stops silently acking (with a kill switch); (2) loser recovery — re-read the credential on 401/invalid_grantand adopt the winner's token, with an escalation rule when adoption fails; (3) compare-and-swap on the credential write via a plaintexttokenVersion; (4) core-owned token expiry plus a narrow pre-flight gate; (5) declared per-module rotation semantics; (6) optional jitter. Provable serialization (SQS FIFO per credential) is deferred behind a named metric (frigg.auth.refresh_race_lost).The three designs you asked about
All three are evaluated in Alternatives Considered, on mechanism:
What ratification requires
Two named calls, in the open-question section:
Review notes
claude/frigg-adr-ssm-env-management,claude/frigg-adrs-app-init-skills-pipeline).website/roadmap/data/adrs.json) are updated.🤖 Generated with Claude Code