Skip to content

Commit

Permalink
bug(auth): Fix connection pooling issues in integration tests
Browse files Browse the repository at this point in the history
Because:
- Integration tests would often fail locally.
- Connection pool was getting exhausted
- Connection pool was not getting terminated

This Commit:
- Exposes connection pool
- Makes sure it's terminated when destroy is called on the Kysely db instance.
- Adds a couple missing places where destroy wasn't called on the db instance.
  • Loading branch information
dschom committed Mar 6, 2025
1 parent 5b5fcda commit c649e03
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 5 deletions.
25 changes: 23 additions & 2 deletions libs/shared/db/mysql/account/src/lib/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,29 @@ export type AccountDatabase = Kysely<DB>;
export const AccountDbProvider = Symbol('AccountDbProvider');

export async function setupAccountDatabase(opts: MySQLConfig) {
const dialect = await createDialect(opts);
return new Kysely<DB>({
const { dialect, pool } = await createDialect(opts);
const db = new Kysely<DB>({
dialect,
});

/**
* Important! We've observed that connection pools aren't being destroyed.
* In theory Kysely should do this when the instance is destroyed, but it
* appears not work in practice. The following hijack works around the
* issue and manually closes the connection pool we created.
*
* This was most noticeable when running auth-server remote tests, where
* we'd hit random timeouts acquiring connections because previous connections
* pools were never closed.
*
* When running our services in production, this isn't really a noticeable
* because we aren't creating lots of new account database instances.
*/
const _dbDestroy = db.destroy;
db.destroy = async function () {
await _dbDestroy.call(db);
pool.end(() => {});
};

return db;
}
19 changes: 16 additions & 3 deletions libs/shared/db/mysql/core/src/lib/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* 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 { knex, Knex } from 'knex';
import { MysqlDialect } from 'kysely';
import { MysqlDialect, MysqlPool } from 'kysely';
import { createPool } from 'mysql2';
import { promisify } from 'util';
import { v4 as uuidv4 } from 'uuid';
Expand Down Expand Up @@ -113,7 +113,10 @@ export async function createDialect(
opts: MySQLConfig,
log: Logger = logger,
metrics: StatsD = localStatsD()
) {
): Promise<{
dialect: MysqlDialect;
pool: MysqlPool;
}> {
log.debug('mysqlDialect', {
msg: `mysqlDialect: Creating MysqlDialect`,
connLimit: opts.connectionLimitMax,
Expand All @@ -137,6 +140,9 @@ export async function createDialect(
pool.on('release', (eventId: any) => {
metrics.increment('mysqlDialect.release');
});
pool.on('enqueue', (eventId: any) => {
metrics.increment('mysqlDialect.enqueue');
});

// Connection setup
// Always communicate timestamps in UTC.
Expand Down Expand Up @@ -165,7 +171,14 @@ export async function createDialect(
await pool.promise().query("SET SESSION sql_mode = '" + mode + "'");
}

return new MysqlDialect({
const dialect = new MysqlDialect({
pool,
});

// Important! Exposing the underlying pool, because
// Kysely is not automatically disposing it.
return {
dialect,
pool,
};
}
9 changes: 9 additions & 0 deletions packages/fxa-auth-server/bin/key_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,15 @@ async function run(config) {
message: 'Database connection did not shutdown cleanly. ' + e.message,
});
}

try {
await accountDatabase.destroy();
} catch (e) {
log.warn('shutdown', {
message:
'Account database connection did not shutdown cleanly.' + e.message,
});
}
},
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ describe(`#integration - recovery phone`, function () {

after(async () => {
await TestServer.stop(server);
await db.destroy();
});

it('sets up a recovery phone', async function () {
Expand Down

0 comments on commit c649e03

Please sign in to comment.