From 30b8cb0fb5c8b8aa811730a47704c2cae0591e1e Mon Sep 17 00:00:00 2001 From: Vijay Budhram Date: Tue, 25 Aug 2026 14:59:06 -0400 Subject: [PATCH] fix(tests): drop throwaway MySQL schemas after integration suites - `testAccountDatabaseSetup` creates a `testAccount-` schema for each spec file and never drops it. Closing the pool does not drop a schema. - A local MySQL gains one schema per spec file per run. One developer machine held 184 of them before anyone noticed. CI is not affected. - Adds `testAccountDatabaseTeardown(db?)` to `tests.ts` and exports it. The helper reads its own schema with `SELECT DATABASE()`. It drops that schema only when the name starts with `testAccount-`, then closes the pool in a `finally`. - Calls the teardown from `afterAll` in the 12 `.in.spec.ts` files that call the setup. Jest runs `afterAll` for failing runs too, so those runs also clean up. - Sweeps `testAccount-*` schemas older than 3 hours at the start of the setup, from `information_schema.TABLES.CREATE_TIME`. This covers a crash or a Ctrl-C, where no hook runs. The sweep is age based, so a concurrent run keeps its schema. - Moves the hook in `backup-code.manager.in.spec.ts` to `afterEach`. That file calls the setup in `beforeEach`, so `afterAll` still left four schemas per run. - Adds `tests.spec.ts`, a unit spec for the `testAccount-` guard. It mocks the `kysely` `sql` tag, so the spec needs no MySQL. Closes: https://mozilla-hub.atlassian.net/browse/FXA-14412 --- .../src/lib/passkey.manager.in.spec.ts | 5 +- .../src/lib/passkey.repository.in.spec.ts | 5 +- .../src/lib/passkey.security.in.spec.ts | 5 +- .../src/lib/recovery-phone.manager.in.spec.ts | 6 +- .../src/lib/backup-code.manager.in.spec.ts | 5 +- .../cart/src/lib/cart.manager.in.spec.ts | 3 +- .../cart/src/lib/checkout.service.in.spec.ts | 3 +- .../paypalCustomer.manager.in.spec.ts | 20 ++-- .../paypalCustomer.repository.in.spec.ts | 5 +- .../accountCustomer.manager.in.spec.ts | 15 +-- .../accountCustomer.repository.in.spec.ts | 5 +- .../src/lib/account.manager.in.spec.ts | 10 +- libs/shared/db/mysql/account/src/index.ts | 5 +- .../db/mysql/account/src/lib/tests.spec.ts | 99 +++++++++++++++++++ libs/shared/db/mysql/account/src/lib/tests.ts | 75 +++++++++++++- 15 files changed, 221 insertions(+), 45 deletions(-) create mode 100644 libs/shared/db/mysql/account/src/lib/tests.spec.ts diff --git a/libs/accounts/passkey/src/lib/passkey.manager.in.spec.ts b/libs/accounts/passkey/src/lib/passkey.manager.in.spec.ts index dd2acb33ff2..c1a93981ed9 100644 --- a/libs/accounts/passkey/src/lib/passkey.manager.in.spec.ts +++ b/libs/accounts/passkey/src/lib/passkey.manager.in.spec.ts @@ -8,6 +8,7 @@ import { AccountDbProvider, PasskeyFactory, testAccountDatabaseSetup, + testAccountDatabaseTeardown, } from '@fxa/shared/db/mysql/account'; import { AccountManager } from '@fxa/shared/account/account'; import { LOGGER_PROVIDER } from '@fxa/shared/log'; @@ -85,9 +86,7 @@ describe('PasskeyManager (Integration)', () => { }); afterAll(async () => { - if (db) { - await db.destroy(); - } + await testAccountDatabaseTeardown(db); }); async function createTestAccount(): Promise { diff --git a/libs/accounts/passkey/src/lib/passkey.repository.in.spec.ts b/libs/accounts/passkey/src/lib/passkey.repository.in.spec.ts index 3408327919b..d82e20de8a2 100644 --- a/libs/accounts/passkey/src/lib/passkey.repository.in.spec.ts +++ b/libs/accounts/passkey/src/lib/passkey.repository.in.spec.ts @@ -6,6 +6,7 @@ import { faker } from '@faker-js/faker'; import { AccountDatabase, testAccountDatabaseSetup, + testAccountDatabaseTeardown, PasskeyFactory, } from '@fxa/shared/db/mysql/account'; import { AccountManager } from '@fxa/shared/account/account'; @@ -51,9 +52,7 @@ describe('PasskeyRepository (Integration)', () => { } afterAll(async () => { - if (db) { - await db.destroy(); - } + await testAccountDatabaseTeardown(db); }); describe('insert and find operations', () => { diff --git a/libs/accounts/passkey/src/lib/passkey.security.in.spec.ts b/libs/accounts/passkey/src/lib/passkey.security.in.spec.ts index 79729404a6f..1665b440af5 100644 --- a/libs/accounts/passkey/src/lib/passkey.security.in.spec.ts +++ b/libs/accounts/passkey/src/lib/passkey.security.in.spec.ts @@ -16,6 +16,7 @@ import { AccountDbProvider, PasskeyFactory, testAccountDatabaseSetup, + testAccountDatabaseTeardown, } from '@fxa/shared/db/mysql/account'; import { AccountManager } from '@fxa/shared/account/account'; import { LOGGER_PROVIDER } from '@fxa/shared/log'; @@ -82,9 +83,7 @@ describe('Passkey Security Tests', () => { }); afterAll(async () => { - if (db) { - await db.destroy(); - } + await testAccountDatabaseTeardown(db); }); async function createTestAccount(): Promise { diff --git a/libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts b/libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts index d8fc5ab1da0..c0eb9b84fb2 100644 --- a/libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts +++ b/libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts @@ -7,6 +7,7 @@ import { AccountDatabase, AccountDbProvider, testAccountDatabaseSetup, + testAccountDatabaseTeardown, RecoveryPhoneFactory, } from '@fxa/shared/db/mysql/account'; import { Test } from '@nestjs/testing'; @@ -82,9 +83,10 @@ describe('RecoveryPhoneManager', () => { }); afterAll(async () => { - await clearRedisSmsKeys(); - await db.destroy(); dateMock.mockReset(); + // Drop the schema first, so a failed Redis cleanup cannot leak it. + await testAccountDatabaseTeardown(db); + await clearRedisSmsKeys(); }); it('should get a recovery phone', async () => { diff --git a/libs/accounts/two-factor/src/lib/backup-code.manager.in.spec.ts b/libs/accounts/two-factor/src/lib/backup-code.manager.in.spec.ts index 79f248fec67..7281e96aa67 100644 --- a/libs/accounts/two-factor/src/lib/backup-code.manager.in.spec.ts +++ b/libs/accounts/two-factor/src/lib/backup-code.manager.in.spec.ts @@ -3,6 +3,7 @@ import { AccountDatabase, AccountDbProvider, testAccountDatabaseSetup, + testAccountDatabaseTeardown, } from '@fxa/shared/db/mysql/account'; import { RecoveryCodeFactory } from './backup-code.factories'; import { faker } from '@faker-js/faker'; @@ -46,8 +47,8 @@ describe('BackupCodeManager', () => { backupCodeManager = moduleRef.get(BackupCodeManager); }); - afterAll(async () => { - await db.destroy(); + afterEach(async () => { + await testAccountDatabaseTeardown(db); }); it('should return that the user has backup codes and count them', async () => { diff --git a/libs/payments/cart/src/lib/cart.manager.in.spec.ts b/libs/payments/cart/src/lib/cart.manager.in.spec.ts index 302388a5679..c3f4f7614d4 100644 --- a/libs/payments/cart/src/lib/cart.manager.in.spec.ts +++ b/libs/payments/cart/src/lib/cart.manager.in.spec.ts @@ -7,6 +7,7 @@ import { CartFactory, CartState, testAccountDatabaseSetup, + testAccountDatabaseTeardown, AccountDatabase, CartUpdate, CartErrorReasonId, @@ -87,7 +88,7 @@ describe('CartManager', () => { }); afterAll(async () => { - await db.destroy(); + await testAccountDatabaseTeardown(db); }); beforeEach(async () => { diff --git a/libs/payments/cart/src/lib/checkout.service.in.spec.ts b/libs/payments/cart/src/lib/checkout.service.in.spec.ts index fd3624ee1a1..96b33a34129 100644 --- a/libs/payments/cart/src/lib/checkout.service.in.spec.ts +++ b/libs/payments/cart/src/lib/checkout.service.in.spec.ts @@ -6,6 +6,7 @@ import { CartErrorReasonId, CartState, testAccountDatabaseSetup, + testAccountDatabaseTeardown, AccountDatabase, } from '@fxa/shared/db/mysql/account'; @@ -65,7 +66,7 @@ describe('CheckoutService', () => { }); afterAll(async () => { - await db.destroy(); + await testAccountDatabaseTeardown(db); }); describe('full flow: cart state transitions in DB', () => { diff --git a/libs/payments/paypal/src/lib/paypalCustomer/paypalCustomer.manager.in.spec.ts b/libs/payments/paypal/src/lib/paypalCustomer/paypalCustomer.manager.in.spec.ts index 419011ab213..245d1398b25 100644 --- a/libs/payments/paypal/src/lib/paypalCustomer/paypalCustomer.manager.in.spec.ts +++ b/libs/payments/paypal/src/lib/paypalCustomer/paypalCustomer.manager.in.spec.ts @@ -3,7 +3,11 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ import { Kysely } from 'kysely'; -import { DB, testAccountDatabaseSetup } from '@fxa/shared/db/mysql/account'; +import { + DB, + testAccountDatabaseSetup, + testAccountDatabaseTeardown, +} from '@fxa/shared/db/mysql/account'; import { CreatePaypalCustomerFactory } from './paypalCustomer.factories'; import { PaypalCustomerManager } from './paypalCustomer.manager'; @@ -25,18 +29,15 @@ describe('PaypalCustomerManager', () => { }); afterAll(async () => { - if (kyselyDb) { - await kyselyDb.destroy(); - } + await testAccountDatabaseTeardown(kyselyDb); }); describe('createPaypalCustomer', () => { it('creates a paypalCustomer successfully', async () => { const paypalCustomer = CreatePaypalCustomerFactory(); - const result = await paypalCustomerManager.createPaypalCustomer( - paypalCustomer - ); + const result = + await paypalCustomerManager.createPaypalCustomer(paypalCustomer); expect(result).toEqual({ ...paypalCustomer, @@ -184,9 +185,8 @@ describe('PaypalCustomerManager', () => { const resultPaypalCustomer = await paypalCustomerManager.createPaypalCustomer(paypalCustomer); - const result = await paypalCustomerManager.deletePaypalCustomer( - resultPaypalCustomer - ); + const result = + await paypalCustomerManager.deletePaypalCustomer(resultPaypalCustomer); expect(result).toEqual(true); }); diff --git a/libs/payments/paypal/src/lib/paypalCustomer/paypalCustomer.repository.in.spec.ts b/libs/payments/paypal/src/lib/paypalCustomer/paypalCustomer.repository.in.spec.ts index 545f9f93d9f..06df6e8241e 100644 --- a/libs/payments/paypal/src/lib/paypalCustomer/paypalCustomer.repository.in.spec.ts +++ b/libs/payments/paypal/src/lib/paypalCustomer/paypalCustomer.repository.in.spec.ts @@ -16,6 +16,7 @@ import { DB, PaypalCustomerFactory, testAccountDatabaseSetup, + testAccountDatabaseTeardown, } from '@fxa/shared/db/mysql/account'; import { PaypalCustomerNoRowsUpdatedError } from './paypalCustomer.error'; @@ -27,9 +28,7 @@ describe('PaypalCustomer Repository', () => { }); afterAll(async () => { - if (kyselyDb) { - await kyselyDb.destroy(); - } + await testAccountDatabaseTeardown(kyselyDb); }); describe('createPaypalCustomer', () => { diff --git a/libs/payments/stripe/src/lib/accountCustomer/accountCustomer.manager.in.spec.ts b/libs/payments/stripe/src/lib/accountCustomer/accountCustomer.manager.in.spec.ts index b966583918a..5e119ca74e5 100644 --- a/libs/payments/stripe/src/lib/accountCustomer/accountCustomer.manager.in.spec.ts +++ b/libs/payments/stripe/src/lib/accountCustomer/accountCustomer.manager.in.spec.ts @@ -5,7 +5,11 @@ import { faker } from '@faker-js/faker'; import { Kysely } from 'kysely'; -import { DB, testAccountDatabaseSetup } from '@fxa/shared/db/mysql/account'; +import { + DB, + testAccountDatabaseSetup, + testAccountDatabaseTeardown, +} from '@fxa/shared/db/mysql/account'; import { AccountCustomerDeleteAccountError, @@ -26,18 +30,15 @@ describe('AccountCustomer Manager', () => { }); afterAll(async () => { - if (kyselyDb) { - await kyselyDb.destroy(); - } + await testAccountDatabaseTeardown(kyselyDb); }); describe('createAccountCustomer', () => { it('creates an accountCustomer successfully', async () => { const mockAccountCustomer = CreateAccountCustomerFactory(); - const result = await accountCustomerManager.createAccountCustomer( - mockAccountCustomer - ); + const result = + await accountCustomerManager.createAccountCustomer(mockAccountCustomer); expect(result).toEqual({ ...mockAccountCustomer, diff --git a/libs/payments/stripe/src/lib/accountCustomer/accountCustomer.repository.in.spec.ts b/libs/payments/stripe/src/lib/accountCustomer/accountCustomer.repository.in.spec.ts index 7beb23df2e3..0294eafc29d 100644 --- a/libs/payments/stripe/src/lib/accountCustomer/accountCustomer.repository.in.spec.ts +++ b/libs/payments/stripe/src/lib/accountCustomer/accountCustomer.repository.in.spec.ts @@ -9,6 +9,7 @@ import { AccountCustomerFactory, DB, testAccountDatabaseSetup, + testAccountDatabaseTeardown, } from '@fxa/shared/db/mysql/account'; import { AccountCustomerUpdatedNoEffectError } from './accountCustomer.error'; @@ -27,9 +28,7 @@ describe('AccountCustomer Repository', () => { }); afterAll(async () => { - if (kyselyDb) { - await kyselyDb.destroy(); - } + await testAccountDatabaseTeardown(kyselyDb); }); describe('createAccountCustomer', () => { diff --git a/libs/shared/account/account/src/lib/account.manager.in.spec.ts b/libs/shared/account/account/src/lib/account.manager.in.spec.ts index b85fd8fbe06..a621db0c6f6 100644 --- a/libs/shared/account/account/src/lib/account.manager.in.spec.ts +++ b/libs/shared/account/account/src/lib/account.manager.in.spec.ts @@ -4,7 +4,11 @@ import { Kysely } from 'kysely'; import { faker } from '@faker-js/faker'; -import { DB, testAccountDatabaseSetup } from '@fxa/shared/db/mysql/account'; +import { + DB, + testAccountDatabaseSetup, + testAccountDatabaseTeardown, +} from '@fxa/shared/db/mysql/account'; import { AccountAlreadyExistsError } from './account.error'; import { AccountManager } from './account.manager'; @@ -19,9 +23,7 @@ describe('accountManager', () => { }); afterAll(async () => { - if (kyselyDb) { - await kyselyDb.destroy(); - } + await testAccountDatabaseTeardown(kyselyDb); }); describe('createAccountStub', () => { diff --git a/libs/shared/db/mysql/account/src/index.ts b/libs/shared/db/mysql/account/src/index.ts index 9968ccc84f0..a4d3a97ed57 100644 --- a/libs/shared/db/mysql/account/src/index.ts +++ b/libs/shared/db/mysql/account/src/index.ts @@ -14,7 +14,10 @@ export { RecoveryPhoneFactory, } from './lib/factories'; export { setupAccountDatabase, AccountDbProvider } from './lib/setup'; -export { testAccountDatabaseSetup } from './lib/tests'; +export { + testAccountDatabaseSetup, + testAccountDatabaseTeardown, +} from './lib/tests'; export type { ACCOUNT_TABLES } from './lib/tests'; export type { AccountDatabase } from './lib/setup'; export { diff --git a/libs/shared/db/mysql/account/src/lib/tests.spec.ts b/libs/shared/db/mysql/account/src/lib/tests.spec.ts new file mode 100644 index 00000000000..4177ebb55d3 --- /dev/null +++ b/libs/shared/db/mysql/account/src/lib/tests.spec.ts @@ -0,0 +1,99 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +import type { AccountDatabase } from './setup'; +import { testAccountDatabaseTeardown } from './tests'; + +const mockExecute = jest.fn(); + +// Replaces the sql tag so the helper runs without MySQL. sql.table becomes the +// identity, so a schema name arrives as a plain string. +jest.mock('kysely', () => { + const actual = jest.requireActual('kysely'); + const sql = Object.assign( + (strings: TemplateStringsArray, ...values: unknown[]) => ({ + execute: () => mockExecute(strings.join('?'), values), + }), + actual.sql, + { table: (name: string) => name } + ); + return { ...actual, sql }; +}); + +describe('testAccountDatabaseTeardown', () => { + const REAL_SCHEMA = 'fxa'; + const TEST_SCHEMA = 'testAccount-2e0b1c4a'; + + let destroy: jest.MockedFunction; + let db: AccountDatabase; + + function mockCurrentSchema(name: string | null) { + mockExecute.mockResolvedValueOnce({ rows: [{ name }] }); + } + + function dropCalls() { + return mockExecute.mock.calls.filter(([query]) => + query.startsWith('DROP DATABASE') + ); + } + + beforeEach(() => { + mockExecute.mockReset(); + destroy = jest.fn(); + db = { destroy } as unknown as AccountDatabase; + }); + + it('does nothing when the setup never returned a database', async () => { + await testAccountDatabaseTeardown(undefined); + + expect(mockExecute).not.toHaveBeenCalled(); + }); + + // The prefix must be anchored, and a pool opened with no database reports a + // null schema. + it.each([ + { label: 'lacks the test prefix', name: REAL_SCHEMA }, + { label: 'only contains the test prefix', name: `fxa_${TEST_SCHEMA}` }, + { label: 'is null', name: null }, + ])('does not drop a schema whose name $label', async ({ name }) => { + mockCurrentSchema(name); + + await testAccountDatabaseTeardown(db); + + expect(dropCalls()).toEqual([]); + }); + + it('closes the pool when it leaves a schema in place', async () => { + mockCurrentSchema(REAL_SCHEMA); + + await testAccountDatabaseTeardown(db); + + expect(destroy).toHaveBeenCalledTimes(1); + }); + + it('drops a schema whose name carries the test prefix', async () => { + mockCurrentSchema(TEST_SCHEMA); + + await testAccountDatabaseTeardown(db); + + expect(dropCalls()).toEqual([['DROP DATABASE IF EXISTS ?', [TEST_SCHEMA]]]); + }); + + it('closes the pool after it drops a schema', async () => { + mockCurrentSchema(TEST_SCHEMA); + + await testAccountDatabaseTeardown(db); + + expect(destroy).toHaveBeenCalledTimes(1); + }); + + it('closes the pool when the drop fails', async () => { + mockCurrentSchema(TEST_SCHEMA); + mockExecute.mockRejectedValueOnce(new Error('ER_DBACCESS_DENIED_ERROR')); + + await expect(testAccountDatabaseTeardown(db)).rejects.toThrow( + 'ER_DBACCESS_DENIED_ERROR' + ); + expect(destroy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/libs/shared/db/mysql/account/src/lib/tests.ts b/libs/shared/db/mysql/account/src/lib/tests.ts index 517094133ce..055240707e8 100644 --- a/libs/shared/db/mysql/account/src/lib/tests.ts +++ b/libs/shared/db/mysql/account/src/lib/tests.ts @@ -6,9 +6,17 @@ import fs from 'fs'; import path from 'path'; import { Kysely, sql } from 'kysely'; -import { DB, setupAccountDatabase } from '@fxa/shared/db/mysql/account'; +import { + AccountDatabase, + DB, + setupAccountDatabase, +} from '@fxa/shared/db/mysql/account'; const SQL_FILE_LOCATION = '../test'; +const TEST_SCHEMA_PREFIX = 'testAccount-'; + +// Wide enough that a schema from a concurrent run is never old enough to sweep. +const ORPHAN_SCHEMA_MAX_AGE_HOURS = 3; export type ACCOUNT_TABLES = | 'accounts' @@ -41,7 +49,9 @@ export async function testAccountDatabaseSetup( user: 'root', }); - const testDbName = `testAccount-${crypto.randomUUID()}`; + await dropOrphanedTestSchemas(db); + + const testDbName = `${TEST_SCHEMA_PREFIX}${crypto.randomUUID()}`; await sql`DROP DATABASE IF EXISTS ${sql.table(testDbName)}`.execute(db); await sql`CREATE DATABASE ${sql.table(testDbName)}`.execute(db); @@ -63,6 +73,67 @@ export async function testAccountDatabaseSetup( return db; } +/** + * Drops the throwaway schema and closes the connection. + * + * Asks the connection for its own schema instead of making each test track + * the name, which keeps the testAccountDatabaseSetup signature unchanged. + */ +export async function testAccountDatabaseTeardown(db?: AccountDatabase) { + if (!db) { + // Setup threw, so there is nothing to clean up. + return; + } + + try { + const name = await currentSchema(db); + if (name?.startsWith(TEST_SCHEMA_PREFIX)) { + await sql`DROP DATABASE IF EXISTS ${sql.table(name)}`.execute(db); + } + } finally { + // Always close the pool. A leaked pool exhausts connections later. + await db.destroy(); + } +} + +async function currentSchema(db: AccountDatabase) { + const result = await sql<{ + name: string | null; + }>`SELECT DATABASE() AS name`.execute(db); + return result.rows[0]?.name; +} + +/** + * Drops throwaway schemas left behind by runs that crashed or were killed + * before their teardown. Age based rather than sweep all, so a concurrent run + * keeps its schema. + * + * A schema with no tables has no row in information_schema.TABLES, so a run + * killed between CREATE DATABASE and the table SQL stays behind. SCHEMATA has + * no create time to work from, so that gap is accepted. + */ +async function dropOrphanedTestSchemas(db: AccountDatabase) { + try { + const result = await sql<{ name: string }>` + SELECT TABLE_SCHEMA AS name + FROM information_schema.TABLES + WHERE TABLE_SCHEMA LIKE ${TEST_SCHEMA_PREFIX + '%'} + GROUP BY TABLE_SCHEMA + HAVING MAX(CREATE_TIME) < NOW() - INTERVAL ${sql.lit( + ORPHAN_SCHEMA_MAX_AGE_HOURS + )} HOUR + `.execute(db); + + for (const { name } of result.rows) { + await sql`DROP DATABASE IF EXISTS ${sql.table(name)}`.execute(db); + } + } catch (err) { + // Two suites can sweep at once and race for the same schema. Cleanup of + // old junk must never fail the run that is starting. + console.warn('Could not sweep orphaned test schemas', err); + } +} + /** * Sequential, not Promise.all: a table with a foreign key must be created after * the table it references, or MySQL fails with "Failed to open the referenced