Skip to content

refactor(auth): Record sign-in consents in a single atomic upsert#20812

Draft
nshirley wants to merge 1 commit into
mainfrom
FXA-14077
Draft

refactor(auth): Record sign-in consents in a single atomic upsert#20812
nshirley wants to merge 1 commit into
mainfrom
FXA-14077

Conversation

@nshirley

@nshirley nshirley commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Because

  • recordAuthorizationRows upserted one accountAuthorizations row per requested scope via Promise.allSettled, so an N-scope sign-in briefly held N pooled DB connections. This showed up as elevated OAuth DB connection counts after the June 10 prod deploy (FXA-14077).

This pull request

  • Replaces the per-scope writes with a single multi-row INSERT ... ON DUPLICATE KEY UPDATE, built at call time in _upsertAccountConsents (packages/fxa-auth-server/lib/oauth/db/mysql/index.js), so every scope for a sign-in is recorded in one query/connection.
  • Renames recordSignInConsent/_upsertAccountConsent to the plural recordSignInConsents/_upsertAccountConsents, taking a scopes array, in packages/fxa-auth-server/lib/oauth/db/index.js and authorization.js.
  • Makes the write atomic — all rows land or none — and lets a rejection bubble to the existing authorizationHandler catch that emits accountAuthorization.write_failed and keeps sign-in working.
  • Types the route spec logger mock as createMock<AuthLogger>() and adds remote coverage in test/remote/account_consents.in.spec.ts for multi-scope row creation and firstAuthorizedTosAt/lastAuthorizedTosAt preserve/bump behavior.

Issue that this pull request solves

Closes: FXA-14077

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).
  • I have manually reviewed all AI generated code.

How to review (Optional)

  • Key files/areas to focus on: lib/oauth/db/mysql/index.js (_upsertAccountConsents query construction), lib/routes/oauth/authorization.js (recordAuthorizationRows failure handling).
  • Suggested review order: mysql query builder → db wrapper → route → tests.
  • Risky or complex parts: the dynamic VALUES tuple construction (only the tuple count is dynamic; all values are ?-bound) and the non-empty scopes invariant, which holds because recordAuthorizationRows returns early when there are no requested scopes.

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

  • Atomicity change: the old Promise.allSettled path could leave partial rows and surfaced only the first rejection; the single INSERT is now all-or-nothing.
  • Failure metric: an initial version added a tagged accountAuthorization.consents_write_failed statsd/log event, but it was removed in favor of the existing write_failed handler to avoid double-counting the same failure. Happy to reintroduce tagged detail in review if we want per-service/scope signal.

Because:
 * recordAuthorizationRows upserted one accountAuthorizations row per
   scope via Promise.allSettled, opening N pooled DB connections for an
   N-scope sign-in and raising connection counts after the June 10 prod
   deploy.

This commit:
 * Replaces the per-scope writes with one multi-row INSERT ... ON
   DUPLICATE KEY UPDATE built at call time, so every scope for a sign-in
   is recorded in a single query/connection.
 * Renames recordSignInConsent/_upsertAccountConsent to the plural
   recordSignInConsents/_upsertAccountConsents taking a scopes array.
 * Makes the write atomic (all rows land or none); a rejection bubbles
   to the existing authorizationHandler catch that emits write_failed
   and keeps sign-in working.
 * Types the route spec logger mock as createMock<AuthLogger>() and adds
   remote coverage for multi-scope row creation and timestamp bumping.

Closes #FXA-14077
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant