diff --git a/docs/ops/deploy.md b/docs/ops/deploy.md index ce8c89dbc..032c55730 100644 --- a/docs/ops/deploy.md +++ b/docs/ops/deploy.md @@ -119,6 +119,12 @@ boot drops that INVALID index before it migrates, logs `dropped the invalid index …`, and the re-run builds it again. Without that step the re-run would find the index by name and skip it. +A build that is still running looks the same, INVALID with its migration not +yet finished. The boot therefore leaves every index alone while another +instance is migrating (it checks Prisma's migration lock first) and never +drops an index Postgres reports as being built, so a second container that +starts during the first one's migration cannot cancel it. + An INVALID index whose migration is already recorded applied is not touched at boot. The boot names it on stderr with `WARNING — index … is INVALID`; rebuild it with `REINDEX INDEX CONCURRENTLY "";` when the database is quiet. diff --git a/scripts/migrate-deploy.mjs b/scripts/migrate-deploy.mjs index d7467f8d6..800547a63 100644 --- a/scripts/migrate-deploy.mjs +++ b/scripts/migrate-deploy.mjs @@ -197,12 +197,32 @@ function concurrentIndexMap(dir) { return builtBy; } +/** + * Prisma's migration lock: `migrate deploy` holds this advisory lock (in the + * schema engine, `pg_advisory_lock(72707369)`) for as long as it migrates. + */ +export const PRISMA_MIGRATE_LOCK = 72707369; + /** * Drop the INVALID indexes a migration still to run will build, and name the * others. Best effort: a database that cannot be read here is left to * `migrate deploy`, which reports its own errors. + * + * A concurrent build that is still running looks exactly like a failed one, + * INVALID with its migration not yet finished. With the web and worker roles + * in two containers, both run this script, and dropping the index the other + * one is building cancels its migration and leaves it failed. So the check + * and the drop run in one transaction that first takes Prisma's migration + * lock without waiting: when another instance is migrating, nothing here is + * touched, and while this transaction holds it, no migration starts. An + * index Postgres reports as being built is never dropped either. One + * transaction also keeps `lock_timeout` on the statement it bounds behind a + * transaction pooler. */ -async function clearInvalidConcurrentIndexes(connectionString, migrationsDir) { +export async function clearInvalidConcurrentIndexes( + connectionString, + migrationsDir, +) { if (!connectionString) return; const builtBy = concurrentIndexMap(migrationsDir); let client; @@ -216,18 +236,36 @@ async function clearInvalidConcurrentIndexes(connectionString, migrationsDir) { connectionTimeoutMillis: 5_000, }); await client.connect(); + const schemaSql = client.escapeIdentifier(schema); + await client.query("BEGIN"); + const locked = await client.query( + "SELECT pg_try_advisory_xact_lock($1) AS ok", + [PRISMA_MIGRATE_LOCK], + ); + if (!locked.rows[0]?.ok) { + await client.query("ROLLBACK"); + return; + } + // Dropping an index takes its table's lock for a moment; never wait on + // it for long at boot. + await client.query("SET LOCAL lock_timeout = '5s'"); const invalid = await client.query( `SELECT c.relname AS name FROM pg_index i JOIN pg_class c ON c.oid = i.indexrelid JOIN pg_namespace n ON n.oid = c.relnamespace - WHERE NOT i.indisvalid AND n.nspname = $1`, + WHERE NOT i.indisvalid AND n.nspname = $1 + AND NOT EXISTS ( + SELECT 1 FROM pg_stat_progress_create_index p + WHERE p.index_relid = i.indexrelid + )`, [schema], ); - if (invalid.rows.length === 0) return; - const table = `${client.escapeIdentifier(schema)}._prisma_migrations`; - const exists = await client.query("SELECT to_regclass($1) AS t", [ - `${schema}._prisma_migrations`, - ]); + if (invalid.rows.length === 0) { + await client.query("ROLLBACK"); + return; + } + const table = `${schemaSql}._prisma_migrations`; + const exists = await client.query("SELECT to_regclass($1) AS t", [table]); const applied = new Set( exists.rows[0]?.t ? ( @@ -243,13 +281,13 @@ async function clearInvalidConcurrentIndexes(connectionString, migrationsDir) { builtBy, applied, }); - // Dropping an index takes its table's lock for a moment; never wait on - // it for long at boot. - await client.query("SET lock_timeout = '5s'"); - for (const { index, migration } of plan.drop) { + for (const { index } of plan.drop) { await client.query( - `DROP INDEX IF EXISTS ${client.escapeIdentifier(schema)}.${client.escapeIdentifier(index)}`, + `DROP INDEX IF EXISTS ${schemaSql}.${client.escapeIdentifier(index)}`, ); + } + await client.query("COMMIT"); + for (const { index, migration } of plan.drop) { console.log( `HealthLog: dropped the invalid index ${index} a failed concurrent ` + `build left behind; ${migration} builds it again.`, @@ -264,6 +302,7 @@ async function clearInvalidConcurrentIndexes(connectionString, migrationsDir) { ); } } catch (error) { + await client?.query("ROLLBACK").catch(() => {}); console.error( `HealthLog: could not check for invalid indexes before migrating: ${ error instanceof Error ? error.message : String(error) diff --git a/tests/integration/migrate-deploy-invalid-index.test.ts b/tests/integration/migrate-deploy-invalid-index.test.ts index 25956de43..ee672661a 100644 --- a/tests/integration/migrate-deploy-invalid-index.test.ts +++ b/tests/integration/migrate-deploy-invalid-index.test.ts @@ -16,6 +16,11 @@ import { join } from "node:path"; import { Client } from "pg"; import { afterAll, beforeEach, describe, expect, it } from "vitest"; +import { + clearInvalidConcurrentIndexes, + PRISMA_MIGRATE_LOCK, +} from "../../scripts/migrate-deploy.mjs"; + const SCHEMA = "invalid_index_probe"; const NAME = "0002_probe_value_index"; const INDEX = "probe_value_key"; @@ -171,3 +176,80 @@ describe("a concurrent index build that failed", () => { } }); }); + +/** + * A build still running is INVALID too, and with the web and worker roles in + * two containers the second one's boot runs the cleanup while the first one + * migrates. Dropping that index cancels the first one's migration and leaves + * it failed, so the cleanup must leave a running build alone. + */ +describe("a concurrent index build still running", () => { + async function slowBuildFixture(db: Client): Promise { + await db.query(`CREATE SCHEMA ${SCHEMA}`); + await db.query( + `CREATE TABLE ${SCHEMA}.probe (id integer PRIMARY KEY, value integer)`, + ); + await db.query( + `INSERT INTO ${SCHEMA}.probe SELECT g, g FROM generate_series(1, 400) g`, + ); + // Slow on purpose, so the build is caught half way. + await db.query( + `CREATE FUNCTION ${SCHEMA}.probe_slow(v integer) RETURNS integer + LANGUAGE plpgsql IMMUTABLE AS $$ + BEGIN PERFORM pg_sleep(0.005); RETURN v; END $$`, + ); + const dir = mkdtempSync(join(tmpdir(), "hl-running-index-")); + dirs.push(dir); + currentDir = join(dir, "migrations"); + write( + dir, + NAME, + `CREATE INDEX CONCURRENTLY IF NOT EXISTS "${INDEX}" ON "probe" (probe_slow("value"));`, + ); + } + + async function caughtHalfWay( + holdMigrateLock: boolean, + ): Promise<{ builder: Client; build: Promise }> { + const builder = await connect(); + if (holdMigrateLock) { + await builder.query("SELECT pg_advisory_lock($1)", [PRISMA_MIGRATE_LOCK]); + } + const build = builder.query( + `CREATE INDEX CONCURRENTLY "${INDEX}" ON ${SCHEMA}.probe (${SCHEMA}.probe_slow(value))`, + ); + const db = await connect(); + try { + const until = Date.now() + 10_000; + while ((await indexState(db)) !== false && Date.now() < until) { + await new Promise((r) => setTimeout(r, 50)); + } + expect(await indexState(db)).toBe(false); + } finally { + await db.end(); + } + return { builder, build }; + } + + for (const [label, holdMigrateLock] of [ + ["another instance migrating", true], + ["a build outside any migration", false], + ] as const) { + it(`leaves the index of ${label} alone`, async () => { + const db = await connect(); + try { + await slowBuildFixture(db); + const { builder, build } = await caughtHalfWay(holdMigrateLock); + try { + await clearInvalidConcurrentIndexes(probeUrl(), currentDir); + await expect(build).resolves.toBeDefined(); + expect(await indexState(db)).toBe(true); + } finally { + await builder.end(); + } + } finally { + await db.end(); + } + }, 60_000); + } +});