fix: close migration CLI db clients on failure paths

.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.

packages/migrations/src/commands/cli.ts

@@ -280,6 +280,16 @@ async function connectDbClient(db: string, dialect: Dialect): Promise<CliDbClien
     throw new Error(`Unsupported dialect: ${dialect}`);
 }
 
+async function closeCliDbClientSafely(dbClient: CliDbClient): Promise<void> {
+    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<void> {
     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;
+                }
             }
         );
 }

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<void>;
+};
+
+const migrationRunnerMock = {
+    applyImpl: async (): Promise<void> => {},
+    statusImpl: async (): Promise<{ id: string; applied: boolean }[]> => [],
+    closeImpl: async (): Promise<void> => {},
+};
+
+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<void> {
+            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<void> {
     }
 }
 
+async function runMigrationsCommand(root: string, args: string[]): Promise<void> {
+    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')]);
+    });
 });