Fail migrations correctly and resolve empty direct database URLs

Author: Sg312

.github/workflows/migrations.yml

@@ -64,6 +64,8 @@ jobs:
           MIGRATION_DATABASE_URL: ${{ inputs.environment == 'production' && secrets.MIGRATION_DATABASE_URL || inputs.environment == 'staging' && secrets.STAGING_MIGRATION_DATABASE_URL || '' }}
           ENVIRONMENT: ${{ inputs.environment }}
         run: |
+          set -euo pipefail
+
           if [ -z "$DATABASE_URL" ]; then
             echo "ERROR: no database URL secret resolved for environment '${ENVIRONMENT}'" >&2
             exit 1
@@ -75,7 +77,7 @@ jobs:
             # covers data-loss). In CI it throws "Interactive prompts require a TTY
             # terminal" but still exits 0, so the job goes green without applying the
             # change. tee keeps the output live in the log; we then fail on drizzle's
-            # own TTY error. A genuine non-zero exit already fails via `set -e`.
+            # own TTY error. pipefail also preserves a non-zero db:push exit through tee.
             bun run db:push --force < /dev/null 2>&1 | tee /tmp/db-push.log
             if grep -q "Interactive prompts require a TTY terminal" /tmp/db-push.log; then
               echo "ERROR: db:push needs an interactive rename decision; land it as a versioned migration instead of relying on push." >&2

packages/db/script-migrations/0015_backfill_embedding_search.ts

@@ -1,3 +1,4 @@
+import { resolveMigrationDatabaseUrl } from '@sim/db/script-migrations/database-url'
 import type { ScriptMigration } from '@sim/db/script-migrations/types'
 import { createLogger } from '@sim/logger'
 import postgres, { type Sql } from 'postgres'
@@ -94,7 +95,7 @@ export const backfillEmbeddingSearchMigration: ScriptMigration = {
 
 /** db:push also installs database behavior that Drizzle's schema cannot express. */
 if (import.meta.main) {
-  const url = process.env.MIGRATION_DATABASE_URL ?? process.env.DATABASE_URL
+  const url = resolveMigrationDatabaseUrl()
   if (!url) throw new Error('DATABASE_URL is required to initialize embedding search')
   const sql = postgres(url, { max: 1, max_lifetime: null, onnotice: () => undefined })
   try {

packages/db/script-migrations/0016_backfill_search_vectors.ts

@@ -1,3 +1,4 @@
+import { resolveMigrationDatabaseUrl } from '@sim/db/script-migrations/database-url'
 import type { ScriptMigration } from '@sim/db/script-migrations/types'
 import { createLogger } from '@sim/logger'
 import postgres, { type Sql } from 'postgres'
@@ -241,7 +242,7 @@ export const backfillSearchVectorsMigration: ScriptMigration = {
 }
 
 if (import.meta.main) {
-  const url = process.env.MIGRATION_DATABASE_URL ?? process.env.DATABASE_URL
+  const url = resolveMigrationDatabaseUrl()
   if (!url) throw new Error('DATABASE_URL is required to initialize search vectors')
   const sql = postgres(url, { max: 1, max_lifetime: null, onnotice: () => undefined })
   try {

packages/db/script-migrations/database-url.test.ts

@@ -0,0 +1,26 @@
+import { resolveMigrationDatabaseUrl } from '@sim/db/script-migrations/database-url'
+import { afterEach, describe, expect, it, vi } from 'vitest'
+
+const applicationUrl = 'postgresql://application.invalid/application'
+const migrationUrl = 'postgresql://migration.invalid/migrations'
+
+describe('standalone migration database URL', () => {
+  afterEach(() => vi.unstubAllEnvs())
+
+  it.each([
+    { direct: undefined, application: applicationUrl, expected: applicationUrl },
+    { direct: '', application: applicationUrl, expected: applicationUrl },
+    { direct: migrationUrl, application: applicationUrl, expected: migrationUrl },
+    { direct: migrationUrl, application: undefined, expected: migrationUrl },
+    { direct: undefined, application: undefined, expected: undefined },
+    { direct: '', application: '', expected: '' },
+  ])(
+    'resolves direct=$direct and application=$application',
+    ({ direct, application, expected }) => {
+      vi.stubEnv('MIGRATION_DATABASE_URL', direct)
+      vi.stubEnv('DATABASE_URL', application)
+
+      expect(resolveMigrationDatabaseUrl()).toBe(expected)
+    }
+  )
+})

packages/db/script-migrations/database-url.ts

@@ -0,0 +1,6 @@
+/** Matches the main migrator: an empty optional direct DSN falls back to DATABASE_URL. */
+export function resolveMigrationDatabaseUrl(
+  env: { MIGRATION_DATABASE_URL?: string; DATABASE_URL?: string } = process.env
+): string | undefined {
+  return env.MIGRATION_DATABASE_URL || env.DATABASE_URL
+}

scripts/migrations-workflow.test.ts

@@ -0,0 +1,102 @@
+import { spawnSync } from 'node:child_process'
+import { mkdtempSync, readFileSync, rmSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { describe, expect, it } from 'vitest'
+
+const workflow = readFileSync(
+  new URL('../.github/workflows/migrations.yml', import.meta.url),
+  'utf8'
+)
+const step = workflow.match(/^ {8}run: \|\r?\n((?: {10}.*(?:\r?\n|$)|\r?\n)+)/m)?.[1]
+if (!step) throw new Error('Migration workflow must contain its schema application shell step')
+const script = step.replace(/^ {10}/gm, '')
+
+/** Execute the checked-in shell with fake database commands and an isolated scratch log. */
+function runMigration(env: Record<string, string> = {}) {
+  const directory = mkdtempSync(join(tmpdir(), 'migration-workflow-'))
+  try {
+    return spawnSync(
+      'bash',
+      [
+        '-e',
+        '-c',
+        `bun() {
+          printf 'COMMAND: %s\n' "$*"
+          case "$2" in
+            db:push) printf '%s\n' "$PUSH_OUTPUT"; return "$PUSH_EXIT" ;;
+            ./scripts/apply-dev-workspace-file-size-cutover.ts) return "$CUTOVER_EXIT" ;;
+            ./scripts/migrate.ts) return "$MIGRATE_EXIT" ;;
+            *) return 99 ;;
+          esac
+        }
+        ${script.replaceAll('/tmp/db-push.log', '"$MIGRATION_TEST_LOG"')}`,
+      ],
+      {
+        encoding: 'utf8',
+        env: {
+          PATH: process.env.PATH,
+          DATABASE_URL: 'postgresql://example.invalid/unused',
+          MIGRATION_DATABASE_URL: '',
+          ENVIRONMENT: 'dev',
+          MIGRATION_TEST_LOG: join(directory, 'push.log'),
+          PUSH_EXIT: '0',
+          PUSH_OUTPUT: 'Changes applied',
+          CUTOVER_EXIT: '0',
+          MIGRATE_EXIT: '0',
+          ...env,
+        },
+      }
+    )
+  } finally {
+    rmSync(directory, { recursive: true, force: true })
+  }
+}
+
+describe('migration workflow exit propagation', () => {
+  it('fails when post-schema initialization fails before tee succeeds', () => {
+    const result = runMigration({
+      PUSH_EXIT: '42',
+      PUSH_OUTPUT: 'Changes applied\nDATABASE_URL is required to initialize search vectors',
+    })
+    expect(result.status).toBe(42)
+    expect(result.stdout).toContain('DATABASE_URL is required')
+    expect(result.stdout).not.toContain(
+      'COMMAND: run ./scripts/apply-dev-workspace-file-size-cutover.ts'
+    )
+  })
+
+  it('runs the dev cutover only after a successful schema push', () => {
+    const result = runMigration()
+    expect(result.status).toBe(0)
+    expect(result.stdout).toContain('COMMAND: run db:push --force')
+    expect(result.stdout).toContain(
+      'COMMAND: run ./scripts/apply-dev-workspace-file-size-cutover.ts'
+    )
+  })
+
+  it('still rejects drizzle interactive failures that exit zero', () => {
+    const result = runMigration({ PUSH_OUTPUT: 'Interactive prompts require a TTY terminal' })
+    expect(result.status).toBe(1)
+    expect(result.stdout).not.toContain(
+      'COMMAND: run ./scripts/apply-dev-workspace-file-size-cutover.ts'
+    )
+  })
+
+  it('propagates dev cutover failures', () => {
+    expect(runMigration({ CUTOVER_EXIT: '43' }).status).toBe(43)
+  })
+
+  it('keeps versioned migration failures fatal outside dev', () => {
+    const result = runMigration({ ENVIRONMENT: 'staging', MIGRATE_EXIT: '44' })
+    expect(result.status).toBe(44)
+    expect(result.stdout).toContain('COMMAND: run ./scripts/migrate.ts')
+    expect(result.stdout).not.toContain('COMMAND: run db:push')
+  })
+
+  it('fails before invoking commands when no database URL is configured', () => {
+    const result = runMigration({ DATABASE_URL: '' })
+    expect(result.status).toBe(1)
+    expect(result.stdout).not.toContain('COMMAND:')
+  })
+})