fix(migrations): address #1411 review — non-blocking FK validation + complete integration coverage

Apply five review findings on the #1126 FK ON DELETE drift fix:

1. Migration 0094 now uses ADD CONSTRAINT ... NOT VALID instead of a bare
   ADD CONSTRAINT. The bare form would have taken AccessExclusiveLock on
   flowsheet (~857k rows) for the full validation-scan duration, blocking
   on-air DJ writes during deploy. NOT VALID makes the ADD metadata-only
   and instant. Drizzle's migrator wraps the whole migration in one
   transaction (drizzle-orm/pg-core/dialect.js:60), so an in-migration
   VALIDATE would defeat the lock benefit — VALIDATE is documented as
   an out-of-band operator step in the header instead. For these five
   constraints the validation is effectively a no-op anyway: the
   existing NO ACTION FK already kept the reference relation consistent,
   and only the ON DELETE action is changing (forward-looking).

2. Integration spec now exercises all three ON DELETE actions in the fix
   (flowsheet SET NULL, rotation CASCADE, reviews CASCADE) instead of
   only the flowsheet SET NULL path. The previous spec ran two tests:
   a static pg_constraint.confdeltype assertion plus a single behavioural
   test. The behavioural test silently never ran because its library
   INSERT omitted four NOT NULL columns (artist_id, genre_id, format_id,
   code_number) and its flowsheet INSERT omitted play_order — so the
   spec was a false green for the actual cascade/SET-NULL behaviour.

3. Spec now uses the shared getTestDb() pool + withRollback helper from
   tests/utils/db.js instead of opening its own postgres() connection
   and rolling back via a hand-written intentional-error idiom. Matches
   the convention used by neighbouring specs (album-metadata-upsert,
   enrichment-worker-claim, library-identity-backfill).

4. Inserts now use the seed_db.sql baseline rows (artist 1, genre 11,
   format 1) to satisfy library's NOT NULL FK columns rather than
   threading FK resolution through the test. Sequence-assigned ids land
   at 7200+ (shape fixture sets library_id_seq to 7199), safely above
   the explicit-id fixture range.

Local CI: lint, format:check, typecheck, test:unit all pass. Migration
dry-run not available locally (Docker daemon not running); will be
covered by CI's migrate-dryrun job since the change touches db-init paths.

Author: jakebromberg

shared/database/src/migrations/0094_fix-fk-on-delete-flowsheet-rotation-reviews.sql

@@ -17,27 +17,85 @@
 --
 -- This migration follows the pattern in `0048_fix-fk-on-delete-set-null.sql`
 -- (the predecessor that patched the analogous drift for schedule /
--- shift_covers / shows.primary_dj_id; see #433). The five constraints below
--- were missed by 0048.
+-- shift_covers / shows.primary_dj_id; see #433) but uses `ADD CONSTRAINT
+-- ... NOT VALID` rather than a bare `ADD CONSTRAINT` to avoid blocking
+-- writes on flowsheet (~857k prod rows) during the deploy. The five
+-- constraints below were missed by 0048.
+--
+-- ## Lock behaviour: why NOT VALID (and why VALIDATE runs out-of-band)
+--
+-- A bare `ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY ...` takes an
+-- `AccessExclusiveLock` AND runs a full-table validation scan that holds
+-- the lock for the entire scan — blocking every concurrent INSERT/UPDATE/
+-- DELETE on the table for the deploy's duration. On an on-air station with
+-- active DJs writing flowsheet rows in real time, that is a user-visible
+-- outage window.
+--
+-- `ADD CONSTRAINT ... NOT VALID` skips the validation scan: it takes
+-- `AccessExclusiveLock` for a metadata-only change and releases it
+-- instantly. New writes are enforced against the new FK shape immediately;
+-- only retroactive validation of pre-existing rows is deferred.
+--
+-- The companion `ALTER TABLE ... VALIDATE CONSTRAINT` runs the scan under
+-- the lighter `ShareUpdateExclusiveLock`, which allows concurrent SELECT,
+-- INSERT, UPDATE, DELETE. **But this benefit only materializes if VALIDATE
+-- runs in its OWN transaction** — Drizzle's migrator
+-- (`drizzle-orm/pg-core/dialect.js:60`) wraps the entire migration in one
+-- `session.transaction()`, so a VALIDATE statement inside the migration
+-- file would run under the AccessExclusiveLock that the preceding DROP /
+-- ADD already acquired, defeating the point. We therefore omit VALIDATE
+-- here and document it as the post-deploy operator step below.
+--
+-- For these five constraints the validation is effectively a no-op anyway:
+-- the existing `NO ACTION` FK has already kept the reference relation
+-- consistent (every flowsheet.album_id either points at a live library.id
+-- or is NULL). Changing only the `ON DELETE` action does not introduce
+-- any new data invariant on existing rows — the action governs future
+-- parent-row DELETEs. VALIDATE still has to scan because PostgreSQL
+-- tracks the `convalidated` flag per constraint; until VALIDATE runs the
+-- constraint is recorded as "trusted for new writes but not proven for
+-- old rows."
+--
+-- ## Post-deploy operator step
+--
+-- After this migration deploys, an operator runs the following five
+-- statements (each in its own implicit transaction — do NOT wrap them in
+-- BEGIN/COMMIT) during a low-write window to clear the unvalidated state.
+-- Skipping this step is harmless for correctness; it only leaves the
+-- constraints with `convalidated = false` until the next operator runs
+-- it. A bare `ANALYZE` is not needed (no row mutations).
+--
+--   ALTER TABLE "wxyc_schema"."flowsheet" VALIDATE CONSTRAINT "flowsheet_show_id_shows_id_fk";
+--   ALTER TABLE "wxyc_schema"."flowsheet" VALIDATE CONSTRAINT "flowsheet_album_id_library_id_fk";
+--   ALTER TABLE "wxyc_schema"."flowsheet" VALIDATE CONSTRAINT "flowsheet_rotation_id_rotation_id_fk";
+--   ALTER TABLE "wxyc_schema"."rotation"  VALIDATE CONSTRAINT "rotation_album_id_library_id_fk";
+--   ALTER TABLE "wxyc_schema"."reviews"   VALIDATE CONSTRAINT "reviews_album_id_library_id_fk";
+--
+-- We DROP+ADD rather than `ALTER CONSTRAINT` because PostgreSQL has no
+-- syntax to change `ON DELETE` action in place — you must drop and recreate.
+-- The DROP itself is metadata-only and instant.
 --
 -- See WXYC/Backend-Service#1126 for the full drift table and reproduction.
+-- See PostgreSQL docs:
+--   https://www.postgresql.org/docs/current/sql-altertable.html (NOT VALID)
+--   https://www.postgresql.org/docs/current/explicit-locking.html (lock modes)
 
 -- flowsheet.show_id → shows.id : NO ACTION → SET NULL
 ALTER TABLE "wxyc_schema"."flowsheet" DROP CONSTRAINT "flowsheet_show_id_shows_id_fk";
-ALTER TABLE "wxyc_schema"."flowsheet" ADD CONSTRAINT "flowsheet_show_id_shows_id_fk" FOREIGN KEY ("show_id") REFERENCES "wxyc_schema"."shows"("id") ON DELETE SET NULL ON UPDATE NO ACTION;
+ALTER TABLE "wxyc_schema"."flowsheet" ADD CONSTRAINT "flowsheet_show_id_shows_id_fk" FOREIGN KEY ("show_id") REFERENCES "wxyc_schema"."shows"("id") ON DELETE SET NULL ON UPDATE NO ACTION NOT VALID;
 
 -- flowsheet.album_id → library.id : NO ACTION → SET NULL
 ALTER TABLE "wxyc_schema"."flowsheet" DROP CONSTRAINT "flowsheet_album_id_library_id_fk";
-ALTER TABLE "wxyc_schema"."flowsheet" ADD CONSTRAINT "flowsheet_album_id_library_id_fk" FOREIGN KEY ("album_id") REFERENCES "wxyc_schema"."library"("id") ON DELETE SET NULL ON UPDATE NO ACTION;
+ALTER TABLE "wxyc_schema"."flowsheet" ADD CONSTRAINT "flowsheet_album_id_library_id_fk" FOREIGN KEY ("album_id") REFERENCES "wxyc_schema"."library"("id") ON DELETE SET NULL ON UPDATE NO ACTION NOT VALID;
 
 -- flowsheet.rotation_id → rotation.id : NO ACTION → SET NULL
 ALTER TABLE "wxyc_schema"."flowsheet" DROP CONSTRAINT "flowsheet_rotation_id_rotation_id_fk";
-ALTER TABLE "wxyc_schema"."flowsheet" ADD CONSTRAINT "flowsheet_rotation_id_rotation_id_fk" FOREIGN KEY ("rotation_id") REFERENCES "wxyc_schema"."rotation"("id") ON DELETE SET NULL ON UPDATE NO ACTION;
+ALTER TABLE "wxyc_schema"."flowsheet" ADD CONSTRAINT "flowsheet_rotation_id_rotation_id_fk" FOREIGN KEY ("rotation_id") REFERENCES "wxyc_schema"."rotation"("id") ON DELETE SET NULL ON UPDATE NO ACTION NOT VALID;
 
 -- rotation.album_id → library.id : NO ACTION → CASCADE
 ALTER TABLE "wxyc_schema"."rotation" DROP CONSTRAINT "rotation_album_id_library_id_fk";
-ALTER TABLE "wxyc_schema"."rotation" ADD CONSTRAINT "rotation_album_id_library_id_fk" FOREIGN KEY ("album_id") REFERENCES "wxyc_schema"."library"("id") ON DELETE CASCADE ON UPDATE NO ACTION;
+ALTER TABLE "wxyc_schema"."rotation" ADD CONSTRAINT "rotation_album_id_library_id_fk" FOREIGN KEY ("album_id") REFERENCES "wxyc_schema"."library"("id") ON DELETE CASCADE ON UPDATE NO ACTION NOT VALID;
 
 -- reviews.album_id → library.id : NO ACTION → CASCADE
 ALTER TABLE "wxyc_schema"."reviews" DROP CONSTRAINT "reviews_album_id_library_id_fk";
-ALTER TABLE "wxyc_schema"."reviews" ADD CONSTRAINT "reviews_album_id_library_id_fk" FOREIGN KEY ("album_id") REFERENCES "wxyc_schema"."library"("id") ON DELETE CASCADE ON UPDATE NO ACTION;
+ALTER TABLE "wxyc_schema"."reviews" ADD CONSTRAINT "reviews_album_id_library_id_fk" FOREIGN KEY ("album_id") REFERENCES "wxyc_schema"."library"("id") ON DELETE CASCADE ON UPDATE NO ACTION NOT VALID;

shared/database/src/migrations/meta/applied-hashes.json

@@ -91,5 +91,5 @@
   "0091_venues-and-concerts": "e738de9bdb0ab762c982a6e31f664f88705bc2b7336bbd57a15efe3b250e6e87",
   "0092_normalize-artist-name": "57bf7f03733591917917c5a19b9b67d8527a64ccce224cdc3f1102f4a9cca368",
   "0093_concerts-first-scraped-at": "bbeee146c92ab63fa70e9f2c537d5a06a13f80a5f52c23cb5a1bed1432057421",
-  "0094_fix-fk-on-delete-flowsheet-rotation-reviews": "35e9df0f17f40dbb348b0a4e437bc3d44c72260a2eeeef9572046f3eef713e47"
+  "0094_fix-fk-on-delete-flowsheet-rotation-reviews": "558e6ce5fa9ae4589a988f15ea3e8fd798c430a94c20c5dd480b4ab8290ebaf0"
 }

tests/integration/fk-on-delete-flowsheet-rotation-reviews.spec.js

@@ -12,30 +12,19 @@
  *
  * This spec is the live regression test: it asserts the current
  * `pg_constraint.confdeltype` for each of the five FKs (mirroring the
- * issue body's reproduction query) AND exercises the actual ON DELETE
- * behaviour via a parent-row DELETE under transaction rollback (so the
- * shape fixture loaded by globalSetup is not perturbed).
+ * issue body's reproduction query) AND exercises each of the three
+ * actual ON DELETE behaviours (flowsheet SET NULL, rotation CASCADE,
+ * reviews CASCADE) via a parent-row DELETE under transaction rollback
+ * (so the shape fixture loaded by globalSetup is not perturbed).
  *
  * If a future schema change re-introduces the drift, this spec fails
  * before the migration is ever attempted against prod.
  */
 
-const postgres = require('postgres');
+const { getTestDb, withRollback } = require('../utils/db');
 
 const SCHEMA = process.env.WXYC_SCHEMA_NAME || 'wxyc_schema';
 
-function makeSql() {
-  return postgres({
-    host: process.env.DB_HOST || 'localhost',
-    port: parseInt(process.env.DB_PORT || process.env.CI_DB_PORT || '5433', 10),
-    database: process.env.DB_NAME || 'wxyc_db',
-    user: process.env.DB_USERNAME || 'test-user',
-    password: process.env.DB_PASSWORD || 'test-pw',
-    onnotice: () => {},
-    max: 2,
-  });
-}
-
 // Map from pg_constraint.confdeltype's single-char encoding to the SQL
 // keyword we expect, for readable assertion failure messages.
 const CONFDELTYPE_LABEL = {
@@ -46,15 +35,22 @@ const CONFDELTYPE_LABEL = {
   d: 'SET DEFAULT',
 };
 
+// Seeded baseline (dev_env/seed_db.sql): artists 1-3, genres 1-15
+// (rock=11), formats include cd=1 / vinyl=2. The shape fixture
+// (tests/fixtures/shape.sql) advances the library/rotation/flowsheet
+// sequences to 7199 so any serial-assigned id below lands at 7200+,
+// safely above the explicit-id fixture range (7000-7099). Each
+// behavioural test wraps its INSERT/DELETE in a withRollback transaction
+// so the fixture stays intact for downstream specs.
+const SEED_ARTIST_ID = 1;
+const SEED_GENRE_ID = 11;
+const SEED_FORMAT_ID = 1;
+
 describe('FK ON DELETE on flowsheet / rotation / reviews (#1126, migration 0094)', () => {
   let sql;
 
   beforeAll(() => {
-    sql = makeSql();
-  });
-
-  afterAll(async () => {
-    if (sql) await sql.end({ timeout: 5 });
+    sql = getTestDb();
   });
 
   test('pg_constraint.confdeltype matches the schema-source declarations', async () => {
@@ -96,35 +92,75 @@ describe('FK ON DELETE on flowsheet / rotation / reviews (#1126, migration 0094)
   });
 
   test('DELETE on a library row sets flowsheet.album_id to NULL (was NO ACTION)', async () => {
-    // Run inside a transaction we abort, so the fixture stays intact for
-    // any spec that runs after this one in the same Jest worker.
-    await sql
-      .begin(async (tx) => {
-        // Insert a library row + a flowsheet entry referencing it. Use a
-        // synthetic id well above the shape-fixture range (7000-7099) and
-        // above the `bigserial`/`serial` PK floor for stability.
-        const [lib] = await tx.unsafe(
-          `INSERT INTO "${SCHEMA}".library (artist_name, album_title)
-         VALUES ('FK Test Artist', 'FK Test Album')
-         RETURNING id`
-        );
-        const [fls] = await tx.unsafe(
-          `INSERT INTO "${SCHEMA}".flowsheet (album_id, entry_type, message)
-         VALUES (${lib.id}, 'track', 'fk-test')
-         RETURNING id`
-        );
-
-        await tx.unsafe(`DELETE FROM "${SCHEMA}".library WHERE id = ${lib.id}`);
-
-        const after = await tx.unsafe(`SELECT album_id FROM "${SCHEMA}".flowsheet WHERE id = ${fls.id}`);
-        expect(after.length).toBe(1);
-        expect(after[0].album_id).toBeNull();
-
-        // Roll back so neither row survives the test.
-        throw new Error('intentional rollback');
-      })
-      .catch((err) => {
-        if (err.message !== 'intentional rollback') throw err;
-      });
+    await withRollback(async (tx) => {
+      // library: artist_id, genre_id, format_id, album_title, code_number
+      // are all NOT NULL (see shared/database/src/schema.ts:344).
+      // Let `serial` auto-assign id from the post-fixture sequence floor
+      // (7200+); the explicit-id fixture range stops at 7099.
+      const [lib] = await tx`
+        INSERT INTO ${tx(SCHEMA)}.library (artist_id, genre_id, format_id, album_title, code_number, artist_name)
+        VALUES (${SEED_ARTIST_ID}, ${SEED_GENRE_ID}, ${SEED_FORMAT_ID},
+                'FK Test Album SET NULL', 9001, 'FK Test Artist')
+        RETURNING id
+      `;
+      // flowsheet: entry_type defaults to 'track'; play_order is NOT NULL
+      // with no default (schema.ts:679).
+      const [fls] = await tx`
+        INSERT INTO ${tx(SCHEMA)}.flowsheet (album_id, entry_type, play_order, message)
+        VALUES (${lib.id}, 'track', 1, 'fk-test-set-null')
+        RETURNING id
+      `;
+
+      await tx`DELETE FROM ${tx(SCHEMA)}.library WHERE id = ${lib.id}`;
+
+      const after = await tx`SELECT album_id FROM ${tx(SCHEMA)}.flowsheet WHERE id = ${fls.id}`;
+      expect(after).toHaveLength(1);
+      expect(after[0].album_id).toBeNull();
+    });
+  });
+
+  test('DELETE on a library row cascades to rotation rows (was NO ACTION)', async () => {
+    await withRollback(async (tx) => {
+      const [lib] = await tx`
+        INSERT INTO ${tx(SCHEMA)}.library (artist_id, genre_id, format_id, album_title, code_number, artist_name)
+        VALUES (${SEED_ARTIST_ID}, ${SEED_GENRE_ID}, ${SEED_FORMAT_ID},
+                'FK Test Album CASCADE rotation', 9002, 'FK Test Artist')
+        RETURNING id
+      `;
+      // rotation: album_id is nullable but referenced; rotation_bin is
+      // NOT NULL (schema.ts:558).
+      const [rot] = await tx`
+        INSERT INTO ${tx(SCHEMA)}.rotation (album_id, rotation_bin)
+        VALUES (${lib.id}, 'L')
+        RETURNING id
+      `;
+
+      await tx`DELETE FROM ${tx(SCHEMA)}.library WHERE id = ${lib.id}`;
+
+      const after = await tx`SELECT id FROM ${tx(SCHEMA)}.rotation WHERE id = ${rot.id}`;
+      expect(after).toHaveLength(0);
+    });
+  });
+
+  test('DELETE on a library row cascades to reviews rows (was NO ACTION)', async () => {
+    await withRollback(async (tx) => {
+      const [lib] = await tx`
+        INSERT INTO ${tx(SCHEMA)}.library (artist_id, genre_id, format_id, album_title, code_number, artist_name)
+        VALUES (${SEED_ARTIST_ID}, ${SEED_GENRE_ID}, ${SEED_FORMAT_ID},
+                'FK Test Album CASCADE reviews', 9003, 'FK Test Artist')
+        RETURNING id
+      `;
+      // reviews: album_id is NOT NULL + UNIQUE (schema.ts:1075).
+      const [rev] = await tx`
+        INSERT INTO ${tx(SCHEMA)}.reviews (album_id, review, author)
+        VALUES (${lib.id}, 'fk-test-cascade-reviews', 'fk-test')
+        RETURNING id
+      `;
+
+      await tx`DELETE FROM ${tx(SCHEMA)}.library WHERE id = ${lib.id}`;
+
+      const after = await tx`SELECT id FROM ${tx(SCHEMA)}.reviews WHERE id = ${rev.id}`;
+      expect(after).toHaveLength(0);
+    });
   });
 });