From 9183c8dd533a62e4b468aef0e4fbe2603fb14c8e Mon Sep 17 00:00:00 2001 From: Pedro Del Moral Lopez Date: Fri, 5 Jun 2026 11:25:24 -0400 Subject: [PATCH 1/3] fix: close migration CLI db clients on failure paths Wrap migrate and status handlers in try/finally so dbClient.close() runs when runner.apply() or runner.status() throws, matching connectAndIntrospect(). --- packages/migrations/src/commands/cli.ts | 38 ++++++----- .../tests/registerMigrationsCommands.test.ts | 66 +++++++++++++++++++ 2 files changed, 87 insertions(+), 17 deletions(-) diff --git a/packages/migrations/src/commands/cli.ts b/packages/migrations/src/commands/cli.ts index 11659116..1dfce5f4 100644 --- a/packages/migrations/src/commands/cli.ts +++ b/packages/migrations/src/commands/cli.ts @@ -354,11 +354,13 @@ export function registerMigrationsCommands(yargsBuilder: Argv): Argv { const dbClient = await connectDbClient(resolved.db, resolved.dialect); - const runner = new MigrationRunner(dbClient, resolved.dialect, resolved.dir); - await runner.apply(argv.to); - - await dbClient.close(); - logger.info('Migrations applied successfully'); + try { + const runner = new MigrationRunner(dbClient, resolved.dialect, resolved.dir); + await runner.apply(argv.to); + logger.info('Migrations applied successfully'); + } finally { + await dbClient.close(); + } } ) .command( @@ -532,19 +534,21 @@ export function registerMigrationsCommands(yargsBuilder: Argv): Argv { const dbClient = await connectDbClient(resolved.db, resolved.dialect); - const runner = new MigrationRunner(dbClient, resolved.dialect, resolved.dir); - const statuses = await runner.status(); - - if (statuses.length === 0) { - logger.info('No migrations found'); - } else { - statuses.forEach((statusItem) => { - const marker = statusItem.applied ? '[x]' : '[ ]'; - logger.info(` ${marker} ${statusItem.id}`); - }); + try { + const runner = new MigrationRunner(dbClient, resolved.dialect, resolved.dir); + const statuses = await runner.status(); + + if (statuses.length === 0) { + logger.info('No migrations found'); + } else { + statuses.forEach((statusItem) => { + const marker = statusItem.applied ? '[x]' : '[ ]'; + logger.info(` ${marker} ${statusItem.id}`); + }); + } + } finally { + await dbClient.close(); } - - await dbClient.close(); } ); } diff --git a/packages/migrations/src/commands/tests/registerMigrationsCommands.test.ts b/packages/migrations/src/commands/tests/registerMigrationsCommands.test.ts index 374ad69a..a3f9e98e 100644 --- a/packages/migrations/src/commands/tests/registerMigrationsCommands.test.ts +++ b/packages/migrations/src/commands/tests/registerMigrationsCommands.test.ts @@ -8,6 +8,17 @@ const createdDirs: string[] = []; const LOAD_COUNT_KEY = '__tangoMigrationsLoadCount'; const SLOW_TEST_TIMEOUT_MS = 15_000; +type CliDbClient = { + close: () => Promise; +}; + +const migrationRunnerMock = { + applyImpl: async (): Promise => {}, + statusImpl: async (): Promise<{ id: string; applied: boolean }[]> => [], +}; + +let capturedClient: CliDbClient | undefined; + type LoadCounterGlobal = typeof globalThis & { [LOAD_COUNT_KEY]?: number; }; @@ -25,6 +36,23 @@ vi.mock('@danceroutine/tango-core', async () => { }; }); +vi.mock('../../runner/MigrationRunner', () => ({ + MigrationRunner: class { + constructor(client: CliDbClient) { + vi.spyOn(client, 'close'); + capturedClient = client; + } + + async apply(): Promise { + return migrationRunnerMock.applyImpl(); + } + + async status(): Promise<{ id: string; applied: boolean }[]> { + return migrationRunnerMock.statusImpl(); + } + }, +})); + async function importRegisterMigrationsCommands() { return (await import('../cli')).registerMigrationsCommands; } @@ -96,9 +124,25 @@ async function runMakeMigrations(root: string): Promise { } } +async function runMigrationsCommand(root: string, args: string[]): Promise { + const cwdBefore = process.cwd(); + process.chdir(root); + + try { + const registerMigrationsCommands = await importRegisterMigrationsCommands(); + const parser = registerMigrationsCommands(yargs(args)); + await parser.parseAsync(); + } finally { + process.chdir(cwdBefore); + } +} + afterEach(async () => { warnings.length = 0; delete (globalThis as LoadCounterGlobal)[LOAD_COUNT_KEY]; + capturedClient = undefined; + migrationRunnerMock.applyImpl = async () => {}; + migrationRunnerMock.statusImpl = async () => []; vi.resetModules(); for (const directory of createdDirs.splice(0)) { @@ -266,4 +310,26 @@ describe(importRegisterMigrationsCommands, () => { }, SLOW_TEST_TIMEOUT_MS ); + + it('closes the database client when migrate fails', async () => { + const root = await makeTempDir('tango-migrations-cli-migrate-close-'); + await writeConfigFile(root); + migrationRunnerMock.applyImpl = async () => { + throw new Error('apply failed'); + }; + + await expect(runMigrationsCommand(root, ['migrate', '--dir', './migrations'])).rejects.toThrow('apply failed'); + expect(capturedClient?.close).toHaveBeenCalledOnce(); + }); + + it('closes the database client when status fails', async () => { + const root = await makeTempDir('tango-migrations-cli-status-close-'); + await writeConfigFile(root); + migrationRunnerMock.statusImpl = async () => { + throw new Error('status failed'); + }; + + await expect(runMigrationsCommand(root, ['status', '--dir', './migrations'])).rejects.toThrow('status failed'); + expect(capturedClient?.close).toHaveBeenCalledOnce(); + }); }); From 6658445955c4432644d0b56cd42d3d369ecdb16a Mon Sep 17 00:00:00 2001 From: Pedro Del Moral Lopez Date: Fri, 5 Jun 2026 11:42:35 -0400 Subject: [PATCH 2/3] fix: preserve migration CLI errors when db cleanup fails Log close failures instead of letting them replace the primary migrate or status error. Add changeset and regression tests for the dual-failure path. --- .changeset/cli-db-cleanup-on-failure.md | 5 +++ packages/migrations/src/commands/cli.ts | 28 +++++++++++++-- .../tests/registerMigrationsCommands.test.ts | 34 ++++++++++++++++++- 3 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 .changeset/cli-db-cleanup-on-failure.md diff --git a/.changeset/cli-db-cleanup-on-failure.md b/.changeset/cli-db-cleanup-on-failure.md new file mode 100644 index 00000000..e1a9fe68 --- /dev/null +++ b/.changeset/cli-db-cleanup-on-failure.md @@ -0,0 +1,5 @@ +--- +"@danceroutine/tango-migrations": patch +--- + +Close migration CLI database clients on `migrate` and `status` failure paths, preserve the primary command error when cleanup also fails, and log close failures instead of masking the original failure. diff --git a/packages/migrations/src/commands/cli.ts b/packages/migrations/src/commands/cli.ts index 1dfce5f4..3bc7f1d5 100644 --- a/packages/migrations/src/commands/cli.ts +++ b/packages/migrations/src/commands/cli.ts @@ -280,6 +280,16 @@ async function connectDbClient(db: string, dialect: Dialect): Promise { + try { + await dbClient.close(); + } catch (closeError) { + logger.warn( + `Unable to close database client: ${closeError instanceof Error ? closeError.message : String(closeError)}` + ); + } +} + async function ensureSqliteParentDirectory(filename: string): Promise { if (filename === ':memory:' || filename === 'file::memory:') { return; @@ -354,12 +364,19 @@ export function registerMigrationsCommands(yargsBuilder: Argv): Argv { const dbClient = await connectDbClient(resolved.db, resolved.dialect); + let error: unknown; try { const runner = new MigrationRunner(dbClient, resolved.dialect, resolved.dir); await runner.apply(argv.to); logger.info('Migrations applied successfully'); + } catch (e) { + error = e; } finally { - await dbClient.close(); + await closeCliDbClientSafely(dbClient); + } + + if (error) { + throw error; } } ) @@ -534,6 +551,7 @@ export function registerMigrationsCommands(yargsBuilder: Argv): Argv { const dbClient = await connectDbClient(resolved.db, resolved.dialect); + let error: unknown; try { const runner = new MigrationRunner(dbClient, resolved.dialect, resolved.dir); const statuses = await runner.status(); @@ -546,8 +564,14 @@ export function registerMigrationsCommands(yargsBuilder: Argv): Argv { logger.info(` ${marker} ${statusItem.id}`); }); } + } catch (e) { + error = e; } finally { - await dbClient.close(); + await closeCliDbClientSafely(dbClient); + } + + if (error) { + throw error; } } ); diff --git a/packages/migrations/src/commands/tests/registerMigrationsCommands.test.ts b/packages/migrations/src/commands/tests/registerMigrationsCommands.test.ts index a3f9e98e..9278215a 100644 --- a/packages/migrations/src/commands/tests/registerMigrationsCommands.test.ts +++ b/packages/migrations/src/commands/tests/registerMigrationsCommands.test.ts @@ -15,6 +15,7 @@ type CliDbClient = { const migrationRunnerMock = { applyImpl: async (): Promise => {}, statusImpl: async (): Promise<{ id: string; applied: boolean }[]> => [], + closeImpl: async (): Promise => {}, }; let capturedClient: CliDbClient | undefined; @@ -39,7 +40,7 @@ vi.mock('@danceroutine/tango-core', async () => { vi.mock('../../runner/MigrationRunner', () => ({ MigrationRunner: class { constructor(client: CliDbClient) { - vi.spyOn(client, 'close'); + vi.spyOn(client, 'close').mockImplementation(() => migrationRunnerMock.closeImpl()); capturedClient = client; } @@ -143,6 +144,7 @@ afterEach(async () => { capturedClient = undefined; migrationRunnerMock.applyImpl = async () => {}; migrationRunnerMock.statusImpl = async () => []; + migrationRunnerMock.closeImpl = async () => {}; vi.resetModules(); for (const directory of createdDirs.splice(0)) { @@ -332,4 +334,34 @@ describe(importRegisterMigrationsCommands, () => { await expect(runMigrationsCommand(root, ['status', '--dir', './migrations'])).rejects.toThrow('status failed'); expect(capturedClient?.close).toHaveBeenCalledOnce(); }); + + it('preserves the migration error when close also fails during migrate', async () => { + const root = await makeTempDir('tango-migrations-cli-migrate-primary-error-'); + await writeConfigFile(root); + migrationRunnerMock.applyImpl = async () => { + throw new Error('apply failed'); + }; + migrationRunnerMock.closeImpl = async () => { + throw new Error('close failed'); + }; + + await expect(runMigrationsCommand(root, ['migrate', '--dir', './migrations'])).rejects.toThrow('apply failed'); + expect(capturedClient?.close).toHaveBeenCalledOnce(); + expect(warnings).toEqual([expect.stringContaining('Unable to close database client')]); + }); + + it('preserves the status error when close also fails during status', async () => { + const root = await makeTempDir('tango-migrations-cli-status-primary-error-'); + await writeConfigFile(root); + migrationRunnerMock.statusImpl = async () => { + throw new Error('status failed'); + }; + migrationRunnerMock.closeImpl = async () => { + throw new Error('close failed'); + }; + + await expect(runMigrationsCommand(root, ['status', '--dir', './migrations'])).rejects.toThrow('status failed'); + expect(capturedClient?.close).toHaveBeenCalledOnce(); + expect(warnings).toEqual([expect.stringContaining('Unable to close database client')]); + }); }); From e50bd90dde74e20e22ea74a9f4e92aae0ffe8419 Mon Sep 17 00:00:00 2001 From: Pedro Del Moral Lopez Date: Fri, 5 Jun 2026 11:49:07 -0400 Subject: [PATCH 3/3] docs: rewrite migrations changeset for release-facing clarity State the migrate/status failure behavior in outcome language and remove contrastive implementation phrasing from the patch note. --- .changeset/cli-db-cleanup-on-failure.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/cli-db-cleanup-on-failure.md b/.changeset/cli-db-cleanup-on-failure.md index e1a9fe68..52dc7b35 100644 --- a/.changeset/cli-db-cleanup-on-failure.md +++ b/.changeset/cli-db-cleanup-on-failure.md @@ -2,4 +2,4 @@ "@danceroutine/tango-migrations": patch --- -Close migration CLI database clients on `migrate` and `status` failure paths, preserve the primary command error when cleanup also fails, and log close failures instead of masking the original failure. +Fix `migrate` and `status` failure handling so a migration error remains the reported failure when closing the database connection afterward also fails. Failed runs now release the connection, and teardown problems are logged separately.