diff --git a/.changeset/cli-db-cleanup-on-failure.md b/.changeset/cli-db-cleanup-on-failure.md new file mode 100644 index 00000000..52dc7b35 --- /dev/null +++ b/.changeset/cli-db-cleanup-on-failure.md @@ -0,0 +1,5 @@ +--- +"@danceroutine/tango-migrations": patch +--- + +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. diff --git a/packages/migrations/src/commands/cli.ts b/packages/migrations/src/commands/cli.ts index 11659116..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,11 +364,20 @@ 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); + 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 closeCliDbClientSafely(dbClient); + } - await dbClient.close(); - logger.info('Migrations applied successfully'); + if (error) { + throw error; + } } ) .command( @@ -532,19 +551,28 @@ 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}`); - }); + let error: unknown; + 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}`); + }); + } + } catch (e) { + error = e; + } finally { + await closeCliDbClientSafely(dbClient); } - await dbClient.close(); + 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 374ad69a..9278215a 100644 --- a/packages/migrations/src/commands/tests/registerMigrationsCommands.test.ts +++ b/packages/migrations/src/commands/tests/registerMigrationsCommands.test.ts @@ -8,6 +8,18 @@ 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 }[]> => [], + closeImpl: async (): Promise => {}, +}; + +let capturedClient: CliDbClient | undefined; + type LoadCounterGlobal = typeof globalThis & { [LOAD_COUNT_KEY]?: number; }; @@ -25,6 +37,23 @@ vi.mock('@danceroutine/tango-core', async () => { }; }); +vi.mock('../../runner/MigrationRunner', () => ({ + MigrationRunner: class { + constructor(client: CliDbClient) { + vi.spyOn(client, 'close').mockImplementation(() => migrationRunnerMock.closeImpl()); + 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 +125,26 @@ 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 () => []; + migrationRunnerMock.closeImpl = async () => {}; vi.resetModules(); for (const directory of createdDirs.splice(0)) { @@ -266,4 +312,56 @@ 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(); + }); + + 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')]); + }); });