Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 1 of the accountAuthorizations scope→scopeId migration in the OAuth MySQL DB by introducing a v2 shadow table and wiring the auth-server to dual-write (and optionally read) via feature flags, plus operational tooling to backfill existing rows.
Changes:
- Add
accountAuthorizations_v2(withscopeIdFK toscopes) and seed common non-URL scopes via anfxa_oauthschema patch. - Add
dualWriteV2andreadV2config flags (default off) and implement v2 dual-write + v2-first/v1-fallback reads in the MySQL OAuth DB layer. - Add a resumable/idempotent v1→v2 backfill script with unit tests, and extend route/integration tests + metrics tagging (
version=v1|v2) for observability.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-auth-server/test/remote/account_consents.in.spec.ts | Adds integration coverage for dual-write and v2-first read behavior behind flags. |
| packages/fxa-auth-server/scripts/backfill-account-authorizations-v2/backfill-account-authorizations-v2.ts | New backfill script to copy v1 rows into v2 via keyset pagination + idempotent upsert. |
| packages/fxa-auth-server/scripts/backfill-account-authorizations-v2/backfill-account-authorizations-v2.spec.ts | Unit tests for backfill helpers and end-to-end run() behavior with stubbed queries. |
| packages/fxa-auth-server/lib/routes/oauth/authorization.spec.ts | Updates/extends route tests for version-tagged metrics and v2 dual-write observability. |
| packages/fxa-auth-server/lib/routes/oauth/authorization.js | Emits recorded as per-row-count with version tag; emits missing-scope + v2 failure metrics/logging. |
| packages/fxa-auth-server/lib/oauth/scopes-cache.ts | Introduces an in-process scope→id cache used by the v2 dual-write/read paths. |
| packages/fxa-auth-server/lib/oauth/scopes-cache.spec.ts | Unit tests for cache hit/miss behavior and non-negative caching semantics. |
| packages/fxa-auth-server/lib/oauth/db/mysql/index.js | Implements v2 shadow-table upserts and v2-first reads; adds bulk scope-id resolution helper. |
| packages/fxa-auth-server/lib/oauth/db/index.js | Wires new flags into the DB API for dual-write and readV2 behavior. |
| packages/fxa-auth-server/config/index.ts | Adds the new oauthServer.accountAuthorizations.{dualWriteV2,readV2} convict config flags. |
| packages/db-migrations/databases/fxa_oauth/target-patch.json | Bumps fxa_oauth schema patch level to 39. |
| packages/db-migrations/databases/fxa_oauth/patches/patch-038-039.sql | Creates accountAuthorizations_v2 and seeds common non-URL scopes. |
| packages/db-migrations/databases/fxa_oauth/patches/patch-039-038.sql | Adds the (commented) reverse patch reference for completeness. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const resolved = new Map<string, number>(); | ||
| const uncached: string[] = []; | ||
| const seen = new Set<string>(); | ||
|
|
||
| // Dedupe input and split into cache hits vs. what needs a DB lookup. | ||
| for (const scope of scopes) { | ||
| if (seen.has(scope)) { | ||
| continue; | ||
| } | ||
| seen.add(scope); | ||
| const cachedId = this.cache.get(scope); | ||
| if (cachedId !== undefined) { | ||
| resolved.set(scope, cachedId); | ||
| } else { | ||
| uncached.push(scope); | ||
| } | ||
| } | ||
|
|
||
| if (uncached.length > 0) { | ||
| const rows = await this.resolver(uncached); | ||
| for (const { scope, id } of rows) { | ||
| this.cache.set(scope, id); | ||
| resolved.set(scope, id); | ||
| } | ||
| } |
| // | ||
| // I'm not entierly sure about this, going to revisit before merge. | ||
| const tags = { |
There was a problem hiding this comment.
Yes, I do intend to revisit this. It's not great, and kind of a mess. Might be worth ripping this out/reverting and just a separate ticket to see if we can get more granular metrics.
The reasoning behind it is; today, the accountAuthorization.recorded fires only once per request, so when it's counted it looks like we have a much lower number of authorizations than we do given an authorization is one-to-many with the authorized scopes. Ideally we emit an accountAuthorization.recorded for each number of authorized scope OR at least include a count signal on the metric that we can sum off of
vbudhram
left a comment
There was a problem hiding this comment.
@nshirley I have not run the script but the changes seem straight forward and low risk (new table, kill switch). Maybe you can update the ticket or file a new ticket to document the steps in the migration. Ie first dual write, backfill, enable V2 read, remove V1 logic, drop old column.
| * `scopeId` from the scopes table. Walks v1 in primary-key order via keyset | ||
| * pagination and upserts each batch into v2. | ||
| * | ||
| * Resolve-only, matching the live dual-write: a v1 row whose `scope` has no |
| } | ||
|
|
||
| async _findAccountConsentForSignIn(uid, scope, service) { | ||
| // Existence check for the sign-in consent row. When readV2 is set, checks |
There was a problem hiding this comment.
Just confirming the plan is that the backfill is run before reading from V2? Otherwise, this route would be doing both lookups (probably ok too)
…ehind flags Because: * accountAuthorizations keys on a wide VARCHAR `scope`. Phase 1 of FXA-14169 migrates it to an integer scopeId FK via a shadow table, dual-writing until cutover. Everything is flag-gated (default off), so merging changes nothing. This commit: * Adds the accountAuthorizations_v2 table + scopes-seed migration and the dualWriteV2 / readV2 config flags. * Dual-writes consents to v2 (resolve-only, isolated from the v1 write) via an in-mem scope->id cache, and adds v2-first / v1-fallback reads. * Adds an idempotent v1->v2 backfill script (inert; running it is FXA-14095). Refs FXA-14094
Because
accountAuthorizationskeys its rows on a wideVARCHARscopecolumn, which bloats the clustered primary key (and every future index). This is Phase 1 of the FXA-14169 epic to migrate that column to an integerscopeIdforeign key.This pull request
accountAuthorizations_v2table (integerscopeIdFK toscopes, noscopestring) and seeds the common non-URL scopes, via a newfxa_oauthmigration.dualWriteV2andreadV2— both defaulting off.dualWriteV2on, writes each consent to v2 alongside v1 using a small in-memory scope→id cache; the v2 write is best-effort and isolated, so it can never affect the authoritative v1 write. WithreadV2on, consent lookups check v2 first and fall back to v1.v1 → v2backfill script that stays inert until run.Issue that this pull request solves
Closes: FXA-14094
Checklist
Put an
xin the boxes that applyHow to review (Optional)
patch-038-039.sql), the dual-write + v2-first/v1-fallback reads inlib/oauth/db/mysql/index.js, and the flag wiring inlib/oauth/db/index.js.scopes-cache.ts→ db-layer dual-write/read → route metrics → backfill script.Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
accountAuthorization.recordedmetric now emits a per-row count with aversiontag (FXA-14202), so its dashboards/alerts should move in lockstep.