bug(auth): Fix error during account delete

[dschom]

Because

• Sentry FXA-AUTH-23B has fired 1,593 times since 2025-02-21, impacting 169 users. POST /v1/account/destroy returns 500 with TypeError: Cannot read properties of undefined (reading 'verifyHash') when two destroy requests for the same account race: the first deletes the row between the second's accountRecord() lookup and its password check, so Account.checkPassword reads verifyHash off undefined.

This pull request

• Guards Account.checkPassword (packages/fxa-shared/db/models/auth/account.ts) — throws notFound() when the row is missing instead of letting account.verifyHash blow up. Matches the existing pattern in this file (consumeRecoveryCode).
• Translates the notFound to error.unknownAccount() at the DB.checkPassword boundary (packages/fxa-auth-server/lib/db.ts) — same pattern already used by db.account() / db.accountRecord(), so all callers (/account/destroy, /account/reauth, password change/forgot, /session/reauth) see one well-known error shape.
• Adds a unit test in lib/routes/account.spec.ts covering the race: response is 400 ACCOUNT_UNKNOWN, no quickDelete, no Glean success event.

Issue that this pull request solves

Closes: FXA-11660

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:
  • `packages/fxa-shared/db/models/auth/account.ts` — the guard
  • `packages/fxa-auth-server/lib/db.ts` — the error translation
• Suggested review order: shared model first, then db.ts wiring, then the new test.
• Risky or complex parts: confirm the `unknownAccount` shape is acceptable for the other callers of `db.checkPassword` (reauth, password change/forgot, session reauth) — they previously couldn't see this error at all, so any caller-specific handling would be new behavior. None looked impacted in my read.

Other information (Optional)

Out of scope: The underlying race itself is not addressed. Instead we fail gracefully. Two truly concurrent destroys still both pass auth before one deletes; they just no longer 500. A real fix (per-uid lock or SELECT ... FOR UPDATE at destroy entry) might be worth a follow up. But is also more complicated and a bit riskier.

[Copilot]

Pull request overview

Fixes a race-condition failure path during account deletion by converting a missing-account row in password verification into a typed “unknown account” error, preventing 500s and ensuring /v1/account/destroy fails gracefully.

Changes:

• Add a guard in Account.checkPassword to throw a typed not-found error when the account row is missing.
• Translate that not-found at the DB.checkPassword boundary into error.unknownAccount() for consistent caller behavior.
• Add a route-level unit test to ensure /account/destroy surfaces ACCOUNT_UNKNOWN and skips follow-up deletion work when password-check fails this way.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
packages/fxa-shared/db/models/auth/account.ts	Throw notFound() when the account row is missing during password verification to avoid unhandled undefined access.
packages/fxa-auth-server/lib/db.ts	Catch shared-model not-found and translate to ACCOUNT_UNKNOWN for consistent error semantics across callers.
packages/fxa-auth-server/lib/routes/account.spec.ts	Add coverage ensuring /account/destroy propagates ACCOUNT_UNKNOWN and does not enqueue deletion work or emit success telemetry.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.