Deploy safety: staging tier, real smoke test, destructive-migration gate, failing health checks

.github/workflows/ci.yml

@@ -444,3 +444,29 @@ jobs:
 
       - name: Knip (unused code gate)
         run: bun run knip:check
+
+  migration-safety:
+    name: Destructive Migration Check
+    runs-on: ubuntu-latest
+
+    steps:
+      - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
+        with:
+          persist-credentials: false
+          fetch-depth: 0
+
+      - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2
+
+      # Newly added packages/db/drizzle/*.sql migrations that contain a destructive
+      # statement (DROP TABLE/COLUMN, TRUNCATE, a type change, an enum-swap rename,
+      # or a NOT NULL column with no DEFAULT) must carry a leading
+      # `-- destructive-migration-ack: <reason>` comment. See
+      # scripts/check-destructive-migrations.ts for the full pattern list and scope
+      # notes (per-file only, not retroactive on existing migrations).
+      - name: Check for unacknowledged destructive migrations
+        run: |
+          BASE_REF="${{ github.event.pull_request.base.sha || github.event.before }}"
+          if [ -z "$BASE_REF" ] || [ "$BASE_REF" = "0000000000000000000000000000000000000000" ]; then
+            BASE_REF="HEAD~1"
+          fi
+          bun scripts/check-destructive-migrations.ts "$BASE_REF"

apps/web/src/app/api/health/__tests__/route.test.ts

@@ -1,5 +1,8 @@
 import { describe, it, expect, beforeEach, vi } from 'vitest';
-import { GET } from '../route';
+// GET is re-imported fresh per test (see beforeEach) because route.ts tracks
+// consecutive DB failures in module-level state — a static import would leak
+// that counter across tests.
+let GET: typeof import('../route').GET;
 const mockExecute = vi.hoisted(() => vi.fn());
 const mockGetPoolStats = vi.hoisted(() => vi.fn());
 const mockGetMonitoringIngestStatus = vi.hoisted(() => vi.fn());
@@ -31,8 +34,10 @@ vi.mock('@/middleware/monitoring', () => ({
 }));
 
 describe('GET /api/health', () => {
-  beforeEach(() => {
+  beforeEach(async () => {
     vi.clearAllMocks();
+    vi.resetModules();
+    ({ GET } = await import('../route'));
     mockGetMonitoringIngestStatus.mockReturnValue('active');
     mockGetPoolStats.mockReturnValue({ total: 0, idle: 0, waiting: 0 });
   });
@@ -112,7 +117,7 @@ describe('GET /api/health', () => {
   });
 
   describe('database failures', () => {
-    it('given database connection fails, should return degraded status', async () => {
+    it('given a single database failure, should report degraded but still return 200 (debounced)', async () => {
       mockExecute.mockRejectedValue(new Error('Connection refused'));
 
       const request = new Request('https://example.com/api/health', {
@@ -125,21 +130,45 @@ describe('GET /api/health', () => {
       expect(response.status).toBe(200);
       expect(body.status).toBe('degraded');
       expect(body.checks.database).toBe('disconnected');
+      expect(body.error).toContain('Database');
     });
 
-    it('given database timeout, should return degraded with error details', async () => {
-      mockExecute.mockRejectedValue(new Error('Query timeout'));
+    it('given two consecutive database failures, should still return 200', async () => {
+      mockExecute.mockRejectedValue(new Error('Connection refused'));
+      const request = new Request('https://example.com/api/health', { method: 'GET' });
 
-      const request = new Request('https://example.com/api/health', {
-        method: 'GET',
-      });
+      await GET(request);
+      const response = await GET(request);
+
+      expect(response.status).toBe(200);
+    });
+
+    it('given three consecutive database failures, should return 503', async () => {
+      mockExecute.mockRejectedValue(new Error('Connection refused'));
+      const request = new Request('https://example.com/api/health', { method: 'GET' });
 
+      await GET(request);
+      await GET(request);
       const response = await GET(request);
       const body = await response.json();
 
-      expect(response.status).toBe(200);
+      expect(response.status).toBe(503);
       expect(body.status).toBe('degraded');
-      expect(body.error).toContain('Database');
+    });
+
+    it('given a recovery after sustained failures, should return to 200', async () => {
+      mockExecute.mockRejectedValue(new Error('Connection refused'));
+      const request = new Request('https://example.com/api/health', { method: 'GET' });
+      await GET(request);
+      await GET(request);
+      await GET(request);
+
+      mockExecute.mockResolvedValue([{ '1': 1 }]);
+      const response = await GET(request);
+      const body = await response.json();
+
+      expect(response.status).toBe(200);
+      expect(body.status).toBe('healthy');
     });
   });
 

apps/web/src/app/api/health/route.ts

@@ -26,11 +26,20 @@ interface HealthResponse {
   error?: string;
 }
 
+// Fly's health check has no failure-count threshold of its own — a single failed
+// probe right after the grace period elapses is enough to fail a rolling deploy.
+// Require this many CONSECUTIVE failures before reporting unhealthy over HTTP, so
+// one transient blip (e.g. a slow DB reconnect) doesn't flip the check.
+const CONSECUTIVE_FAILURE_THRESHOLD = 3;
+let consecutiveDbFailures = 0;
+
 const checkDatabase = async (): Promise<boolean> => {
   try {
     await db.execute(sql`SELECT 1`);
+    consecutiveDbFailures = 0;
     return true;
   } catch {
+    consecutiveDbFailures += 1;
     return false;
   }
 };
@@ -87,14 +96,20 @@ export async function GET(_request: Request): Promise<Response> {
       });
     }
 
+    // Only fail the HTTP status on a sustained DB outage, not monitoring
+    // misconfiguration — the latter is a config warning, not a traffic-serving
+    // failure, and shouldn't cause Fly to cycle machines over it.
+    const dbSustainedFailure = consecutiveDbFailures >= CONSECUTIVE_FAILURE_THRESHOLD;
+
     return Response.json(response, {
-      status: 200,
+      status: dbSustainedFailure ? 503 : 200,
       headers: {
         'Cache-Control': 'no-store, no-cache, must-revalidate',
       },
     });
   } catch (error) {
     loggers.api.error('Health check failed', error as Error);
+    consecutiveDbFailures += 1;
 
     const response: HealthResponse = {
       status: 'degraded',
@@ -115,7 +130,7 @@ export async function GET(_request: Request): Promise<Response> {
     };
 
     return Response.json(response, {
-      status: 200,
+      status: consecutiveDbFailures >= CONSECUTIVE_FAILURE_THRESHOLD ? 503 : 200,
       headers: {
         'Cache-Control': 'no-store, no-cache, must-revalidate',
       },

infrastructure/__tests__/ci-admin-migrate.test.ts

@@ -42,18 +42,44 @@ describe('docker-images.yml admin-DB migrate step', () => {
     expect(adminStep.if).toContain('ADMIN_DB_MIGRATIONS_ENABLED');
   });
 
-  it('given the admin migrations step, should run db:migrate:admin on a one-shot machine', () => {
-    expect(adminStep.run).toContain('flyctl machine run');
+  // The one-shot-machine mechanics (flyctl machine run, polling for
+  // failed/destroyed states) live in scripts/deploy/run-fly-migration.sh —
+  // shared by both the main and admin migration steps, so it's tested once
+  // there (scripts/deploy is not a vitest project; verified by reading the
+  // script directly below) instead of duplicated inline in each step.
+  const RUN_FLY_MIGRATION_SCRIPT = readFileSync(
+    resolve(__dirname, '../../scripts/deploy/run-fly-migration.sh'),
+    'utf-8'
+  );
+
+  it('given the admin migrations step, should run db:migrate:admin via the shared one-shot-machine script', () => {
+    expect(adminStep.run).toContain('scripts/deploy/run-fly-migration.sh');
     expect(adminStep.run).toContain('db:migrate:admin');
   });
 
+  it('given the shared one-shot-machine script, should run flyctl machine run', () => {
+    expect(RUN_FLY_MIGRATION_SCRIPT).toContain('flyctl machine run');
+  });
+
   it('given the admin migrations step, should use the same migrate image as the main step', () => {
-    expect(adminStep.run).toContain('pagespace-migrate:latest');
+    const imageOf = (run?: string) => run?.match(/ghcr\.io\/2witstudios\/pagespace-migrate\b/)?.[0];
+    expect(imageOf(adminStep.run)).toBeDefined();
+    expect(imageOf(adminStep.run)).toBe(imageOf(steps[mainIdx].run));
+  });
+
+  it('given the admin and main migrations steps, should never pin the mutable :latest tag', () => {
+    expect(adminStep.run).not.toContain('pagespace-migrate:latest');
+    expect(steps[mainIdx].run).not.toContain('pagespace-migrate:latest');
+  });
+
+  it('given the admin migrations step, should delegate to the same script as the main step (so it fails on failed/destroyed machine states identically)', () => {
+    expect(adminStep.run).toContain('scripts/deploy/run-fly-migration.sh');
+    expect(steps[mainIdx].run).toContain('scripts/deploy/run-fly-migration.sh');
   });
 
-  it('given the admin migrations step, should fail on failed/destroyed machine states like the main step', () => {
-    expect(adminStep.run).toContain('failed');
-    expect(adminStep.run).toContain('destroyed');
+  it('given the shared one-shot-machine script, should fail on failed/destroyed machine states', () => {
+    expect(RUN_FLY_MIGRATION_SCRIPT).toContain('failed');
+    expect(RUN_FLY_MIGRATION_SCRIPT).toContain('destroyed');
   });
 
   it('given the main migrations step, should NOT be conditional (main DB always migrates)', () => {

scripts/__tests__/check-destructive-migrations.test.ts

@@ -0,0 +1,142 @@
+import { describe, it, expect } from 'vitest';
+import { ACK_PATTERN, findDestructiveReasons, statementsOf, stripSqlComments } from '../check-destructive-migrations';
+
+describe('statementsOf', () => {
+  it('given a Drizzle-style file, should split on statement-breakpoint markers', () => {
+    const sql = 'DROP TABLE "a";--> statement-breakpoint\nDROP TABLE "b";';
+    expect(statementsOf(sql)).toEqual(['DROP TABLE "a";', 'DROP TABLE "b";']);
+  });
+
+  it('given a single-statement file, should return one statement', () => {
+    expect(statementsOf('TRUNCATE "x";')).toEqual(['TRUNCATE "x";']);
+  });
+});
+
+describe('findDestructiveReasons', () => {
+  it('given DROP TABLE, should flag it', () => {
+    expect(findDestructiveReasons('DROP TABLE "alert_history";')).toEqual(['DROP TABLE']);
+  });
+
+  it('given DROP COLUMN, should flag it', () => {
+    expect(findDestructiveReasons('ALTER TABLE "users" DROP COLUMN IF EXISTS "password";')).toEqual([
+      'DROP COLUMN',
+    ]);
+  });
+
+  it('given TRUNCATE, should flag it', () => {
+    expect(findDestructiveReasons('TRUNCATE security_audit_log;')).toEqual(['TRUNCATE']);
+  });
+
+  it('given DROP TYPE, should flag it', () => {
+    expect(findDestructiveReasons('DROP TYPE "WorkflowRunStatus";')).toEqual(['DROP TYPE']);
+  });
+
+  it('given an enum-swap rename (RENAME TO ..._old), should flag it', () => {
+    expect(
+      findDestructiveReasons('ALTER TYPE "WorkflowRunStatus" RENAME TO "WorkflowRunStatus_old";')
+    ).toEqual(['enum-swap rename (RENAME TO ..._old)']);
+  });
+
+  it('given ALTER COLUMN ... TYPE, should flag it', () => {
+    expect(
+      findDestructiveReasons(
+        'ALTER TABLE "activity_logs" ALTER COLUMN "contentFormat" SET DATA TYPE content_format USING "contentFormat"::content_format;'
+      )
+    ).toEqual(['ALTER COLUMN ... TYPE (data type change)']);
+  });
+
+  it('given ADD COLUMN ... NOT NULL with no DEFAULT, should flag it', () => {
+    expect(
+      findDestructiveReasons('ALTER TABLE "calendar_triggers" ADD COLUMN "workflowId" text NOT NULL;')
+    ).toEqual(['ADD COLUMN ... NOT NULL without a DEFAULT']);
+  });
+
+  it('given ADD COLUMN ... NOT NULL with a DEFAULT, should not flag it', () => {
+    expect(
+      findDestructiveReasons('ALTER TABLE "t" ADD COLUMN "x" boolean DEFAULT false NOT NULL;')
+    ).toEqual([]);
+  });
+
+  it('given ADD COLUMN ... bigserial NOT NULL, should not flag it (self-populating)', () => {
+    expect(
+      findDestructiveReasons('ALTER TABLE "activity_logs" ADD COLUMN "chainSeq" bigserial NOT NULL;')
+    ).toEqual([]);
+  });
+
+  it('given a purely additive statement, should not flag anything', () => {
+    expect(findDestructiveReasons('ALTER TABLE "t" ADD COLUMN "x" text;')).toEqual([]);
+  });
+
+  it('given multiple destructive statements in one file, should flag each distinct reason once', () => {
+    const sql = [
+      'TRUNCATE TABLE "calendar_triggers";--> statement-breakpoint',
+      'ALTER TABLE "calendar_triggers" DROP COLUMN IF EXISTS "status";--> statement-breakpoint',
+      'DROP TYPE "CalendarTriggerStatus";',
+    ].join('\n');
+    const reasons = findDestructiveReasons(sql);
+    expect(reasons).toContain('TRUNCATE');
+    expect(reasons).toContain('DROP COLUMN');
+    expect(reasons).toContain('DROP TYPE');
+    expect(reasons).toHaveLength(3);
+  });
+
+  it('given ADD COLUMN NOT NULL with a comment mentioning DEFAULT (not a real one), should still flag it', () => {
+    expect(
+      findDestructiveReasons(
+        '-- table is empty, so no DEFAULT is needed\nALTER TABLE "t" ADD COLUMN "x" text NOT NULL;'
+      )
+    ).toEqual(['ADD COLUMN ... NOT NULL without a DEFAULT']);
+  });
+
+  it('given ADD COLUMN NOT NULL with a comment mentioning SERIAL (not a real one), should still flag it', () => {
+    expect(
+      findDestructiveReasons(
+        '-- not a serial column, just named that way\nALTER TABLE "t" ADD COLUMN "x" text NOT NULL;'
+      )
+    ).toEqual(['ADD COLUMN ... NOT NULL without a DEFAULT']);
+  });
+
+  it('given a real DROP TABLE only inside a comment, should not flag it', () => {
+    expect(findDestructiveReasons('-- old code used to DROP TABLE "x" here, no longer true\nSELECT 1;')).toEqual(
+      []
+    );
+  });
+});
+
+describe('stripSqlComments', () => {
+  it('given a line comment, should remove it', () => {
+    expect(stripSqlComments('-- a comment\nDROP TABLE "x";')).toBe('\nDROP TABLE "x";');
+  });
+
+  it('given a block comment, should remove it', () => {
+    expect(stripSqlComments('/* a block comment */ DROP TABLE "x";')).toBe(' DROP TABLE "x";');
+  });
+
+  it('given a multi-line block comment, should remove it', () => {
+    expect(stripSqlComments('/* line one\nline two */\nDROP TABLE "x";')).toBe('\nDROP TABLE "x";');
+  });
+
+  it('given no comments, should return the input unchanged', () => {
+    expect(stripSqlComments('DROP TABLE "x";')).toBe('DROP TABLE "x";');
+  });
+});
+
+describe('ACK_PATTERN', () => {
+  it('given a destructive-migration-ack comment, should match', () => {
+    expect(ACK_PATTERN.test('-- destructive-migration-ack: no old code reads this table\nDROP TABLE "x";')).toBe(
+      true
+    );
+  });
+
+  it('given an ack comment with no reason text, should not match', () => {
+    expect(ACK_PATTERN.test('-- destructive-migration-ack:\nDROP TABLE "x";')).toBe(false);
+  });
+
+  it('given no ack comment, should not match', () => {
+    expect(ACK_PATTERN.test('DROP TABLE "x";')).toBe(false);
+  });
+
+  it('given an unrelated comment, should not match', () => {
+    expect(ACK_PATTERN.test('-- this table is old\nDROP TABLE "x";')).toBe(false);
+  });
+});