fix(db): stop the pending-migration pre-flight false-blocking an empty database (BLO-31746)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work, and it ships its own schema through Drizzle migrations applied at server startup.
• Several of those migrations need an index that cannot be built inside a transaction, so they refuse to run on a populated table and ask an operator to precreate it with CREATE INDEX CONCURRENTLY.
• BLO-30895 moved that refusal ahead of helm upgrade into a read-only pre-flight, because discovering it from inside server startup cost an hour of deploy and a worker outage.
• The pre-flight tests only whether the index exists and is fully built. It never asks the question the migrations themselves ask, which is whether the table has any rows.
• Those guards raise only on a populated table; on an empty one they raise nothing and build the index inline, needing no operator at all.
• So on a fresh bootstrap database every registered entry is reported a blocker at once, the CLI exits 1, and helm upgrade never starts — on a set of migrations that would all have applied unaided.
• This pull request consults table population, and exempts an absent index only when the table is empty or not yet created.
• The benefit is that new environments and restored-empty replicas deploy without eight unnecessary manual CREATE INDEX CONCURRENTLY statements, and pre-flight failures keep meaning something.

Linked Issues or Issue Description

Refs BLO-31746
Refs BLO-30895 (built the pre-flight this narrows)

• I searched the GitHub PR list (open + recently closed) for similar PRs and confirmed this is not a duplicate. The only overlapping PR is fix(db): bring migration 0217 into the deploy pre-flight, and close the drift test's self-concealing mode (BLO-31626) #1637, which registers migration 0217 and fixes the drift test's self-concealing detector; it is a different defect in the same two files. This PR does not touch the registry, and the two apply independently.

What Changed

• Added decidePreflightBlocker, a pure function that decides whether a guarded pending migration will actually stall, given the index probe and the table's population.
• Added probeTablePopulation, which mirrors the migrations' own EXISTS (SELECT 1 FROM <table> LIMIT 1). It resolves the relation through to_regclass first, so a table that does not exist yet reports absent instead of raising undefined_table and aborting the pre-flight.
• checkPendingMigrationPreflight now probes each distinct table once (six of the eight registered specs share heartbeat_runs) and defers the verdict to decidePreflightBlocker.
• Extended the module's scope note to state the empty-table exclusion, the narrowness of the exemption, and that the probe is advisory.
• Tests: both directions of the exemption, the build-incomplete asymmetry, uniformity over the real registry, and a structural test pinning the migration shape the exemption depends on.

The exemption is deliberately narrow, and this is the load-bearing detail. It applies to an absent index only. A half-built index takes the migrations' other branch, which demands indisvalid and raises with no emptiness test whatsoever — so an empty table does not rescue it. Exempting that case would have re-opened the outage BLO-30895 exists to prevent. The issue's suggested shape ("an empty table means the migration self-serves and is not a blocker") would have done exactly that if applied to both states.

Verification

• pnpm exec vitest run packages/db/src/pending-migration-preflight.test.ts — 22 passed, run in a worktree at master tip (so all 8 registry entries, including 0237, are exercised). tsc --noEmit -p packages/db/tsconfig.json clean.
• Mutation-tested, because a fix that merely stops reporting blockers would satisfy the empty-table criterion alone. Two mutations, each failing 3 tests:
  • exemption widened to cover a half-built index → fails blocks a half-built index regardless of the table being empty/absent and the registry-uniformity test. This is the BLO-30895 regression, and it is caught.
  • exemption removed entirely (today's behaviour) → fails does not block an absent index when the table is empty/absent and the uniformity test.
• The structural test is keyed on the DROP INDEX CONCURRENTLY remediation rather than on branch syntax. An earlier draft keyed on ELSE and passed while silently skipping five of the eight files, which spell the same logic as IF/ELSIF (0205) rather than IF/ELSE (0217). Verified by hand that in all 8 files every emptiness check appears after the structural raise's hint.
• Not verified: the fresh-bootstrap run itself. That needs an empty database with all migrations pending, which CI has no fixture for; the issue lists it as a manual step for the same reason. The reasoning is derived from inspectMigrations returning pendingMigrations: availableMigrations when there is no journal and no tables, plus the migrations' own guard text — not from an observed run.

Risks

Low, and the risk is one-directional by construction. The change can only remove blockers, and only for an absent index on a table with no rows.

• The probe is advisory. A table can gain its first row between the pre-flight and the migration. Stated in the scope note rather than engineered around: the migration re-checks under LOCK TABLE ... IN SHARE MODE and fails loudly, exactly as it does today. Racing to a false negative leaves behaviour no worse than before the pre-flight existed; today's false positive blocks every new environment unconditionally.
• Production is unaffected. All registered migrations are long since applied there, so selectGuardedPendingIndexes filters them out and the pre-flight is already inert.
• A dropped table on a populated database reports absent and is exempted. That database is broken in a way the pre-flight is not the right place to catch, and the migration still fails loudly on its own SELECT.
• The structural test will fail if a future migration in this family gains an emptiness check ahead of its structural raise. That is intended: the exemption's premise would no longer hold and should be rethought rather than drift.

Model Used

claude-opus-5[1m] (Claude Code, Paperclip CTO agent)

[allyblockcast]

🔗 Paperclip issue: BLO-30895
🔗 Paperclip issue: BLO-31746

[allyblockcast]

🔗 Paperclip issue: BLO-30895
🔗 Paperclip issue: BLO-31746

[allyblockcast]

@ally please review at head 4ace2b2.

Review focus, in priority order:

1. The exemption's narrowness is the whole correctness argument. decidePreflightBlocker exempts an empty table for an absent index and deliberately not for a build-incomplete one, because a half-built index takes the migrations' structural branch, which demands indisvalid and raises with no emptiness test. If that reading of the migration SQL is wrong in either direction, the fix is wrong: too wide re-opens BLO-30895, too narrow leaves the false block. Please check it against the SQL rather than against my comment.
2. The structural test's key. It slices on the DROP INDEX CONCURRENTLY hint rather than on branch syntax, because an ELSE-keyed draft passed while silently skipping 5 of 8 files (0205 spells it IF/ELSIF). Is keying on the remediation string robust enough, or is there a shape it would miss?
3. probeTablePopulation's to_regclass hop and the sql(table) identifier interpolation — the former so a not-yet-created table reports absent instead of aborting the pre-flight; the latter for injection safety (values come from a compile-time registry, but I would rather it be safe by construction).
4. The advisory race is documented, not fixed (a table can gain a row between probe and migration). I argue that is strictly no worse than today because the migration re-checks under LOCK TABLE ... IN SHARE MODE. Push back if you disagree.

Not verified: the fresh-bootstrap run itself — no CI fixture for an empty database with all migrations pending. Reasoning is derived from inspectMigrations returning all migrations pending with no journal, not from an observed run.

Note #1637 (BLO-31626) touches the same two files with a different scope (registry marker hole). This PR does not touch the registry; they apply independently.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 4ace2b2

I checked focus item 1 against the migration SQL rather than against the comment, as asked. The exemption's asymmetry is correct in both directions on all 8 registered migrations. Fetched each at this head and traced both branches:

• Absent index (to_regclass IS NULL) — every file gates its raise on IF EXISTS (SELECT 1 FROM <table> LIMIT 1), then LOCK TABLE ... IN SHARE MODE, re-checks, then builds the index inline without CONCURRENTLY. On an empty table nothing raises. Exemption correct.
• Half-built index (exists, indisvalid = false) — every file's structural branch demands indisvalid and raises with no emptiness test in scope. 0205/0208/0224/0230 via the combined IS NOT NULL AND NOT EXISTS(...); 0209/0233/0237 via nested IF NOT EXISTS(...); 0236 via the flat IF to_regclass IS NOT NULL THEN RAISE at line 79. Refusing to exempt this is correct — exempting it would re-open BLO-30895.

0236 is the one that isn't the two-branch shape (flat, with an early RETURN and a duplicate-sample scan ahead of the structural raise) and it still satisfies the asymmetry, so decidePreflightBlocker holds there too.

Critical Issues (0)

None.

Important Issues (1)

• [native-codex] packages/db/src/pending-migration-preflight.test.ts:106 — the structural test's anchor is not unique on 0237, so its guard window silently covers the wrong region. contents.indexOf("DROP INDEX CONCURRENTLY") takes the first occurrence, and in 0237_heartbeat_runs_agent_queued_dispatch_index.sql that string appears twice: once at line 203 inside a prose comment (-- Only with both steps green is `DROP INDEX CONCURRENTLY) and once at line 264 in the actual HINT. The anchor lands on the comment, 61 lines early, so the "every emptiness check sits after the structural raise" assertion is evaluated against lines 203+ instead of 264+ — leaving lines 203–264, which is the entire structural branch (236–265), inside the blind window. An emptiness gate added to that branch would sit after the anchor and the test would pass. That is precisely the regression this test exists to catch, and it is blind to it on the one file with the most complex structural branch. All three assertions currently pass on 0237 for the wrong reason, so this fails silently. The test comment at line 102 also asserts the hint "appears exactly once per file", which is false for 0237 — worth correcting either way, since that claim is what makes indexOf look safe.
  • Strip comment lines before searching, and assert uniqueness so a future second occurrence fails loudly rather than shifting the window:

    const sql = contents.split("\n").filter((l) => !/^\s*--/.test(l)).join("\n");
    const hints = [...sql.matchAll(/DROP INDEX CONCURRENTLY/g)];
    expect(hints.length, `${spec.migration} has no unique mismatch remediation`).toBe(1);
    const structuralHint = hints[0].index;

    I verified this against all 8 files at this head: comment-stripped counts are 1 for every one, including 0237 (raw 2 → stripped 1), and the anchor then resolves to line 264. Note the emptiness-check offsets must be computed against the same stripped string.

Suggestions (3)

• [native-codex] packages/db/src/pending-migration-preflight.test.ts:109 — answering focus item 2 directly: keying on the remediation string is the right call over branch syntax (I confirmed your ELSE finding — 4 files use ELSIF, 3 use ELSE, and 0236 uses neither, so an ELSE-keyed detector misses 5 of 8). The residual gap is not the anchor string but the emptiness pattern: /EXISTS \(SELECT 1 FROM/ requires the phrase contiguous on one line, and I confirmed it does not match the multiline form these very files already use for their pg_index subqueries (EXISTS (\n SELECT 1\n FROM ...), nor (SELECT count(*) FROM t) > 0, EXISTS (SELECT * FROM t ...), a stray double space, or PERFORM 1 FROM t. Worth noting the risk is narrower than it first looks: because you also assert emptinessChecks.length > 0, a rewrite of the existing gates into any of those forms fails loudly on "lost its empty-table guard". The uncovered case is an added gate in one of those forms inside the structural branch while the single-line ones remain. Tolerating that is defensible; a /EXISTS\s*\(\s*SELECT\s+\S+\s+FROM/ widening plus [\s\S] tolerance would shrink it cheaply.
• [code] packages/db/src/pending-migration-preflight.test.ts:24 — the fixture spec cites 0217_heartbeat_runs_queued_age_idx.sql, which exists as a migration but is not in PRECREATE_REQUIRED_INDEXES. Harmless for the pure-function tests (they never read the file), but the comment at line 99 calls it part of "the family" and cites it as the IF/ELSE exemplar, while the structural loop only ever iterates the registry — so 0217 is never actually exercised. Given #1637/BLO-31626 is specifically about registry holes, citing a non-registered migration here invites a misreading. A registered ELSE-style file (0209, 0233, 0237) would make the comment true.
• [gstack/review] packages/db/src/pending-migration-preflight.ts:285 — on focus item 3, both halves are safe. to_regclass(${...}) passes the name as a bound value, and sql(table) uses postgres.js's identifier helper, which quotes and escapes — so it is safe by construction, not merely safe because the registry is compile-time. One cosmetic asymmetry: the existence probe is schema-qualified (public.<table>) while the row probe renders an unqualified identifier resolved through search_path, so under a non-public search_path the two could address different relations. No action needed — the migrations have the identical split ('public.heartbeat_runs'::regclass for the structural check, bare FROM "heartbeat_runs" for the emptiness gate), so the probe mirrors the guard faithfully, which is the property you want.

On focus item 4 — the advisory race

Agreed, no change wanted, and I confirmed the mechanism: all 8 migrations re-check under LOCK TABLE ... IN SHARE MODE after the unlocked test, so a probe-time false negative surfaces as a loud migration failure rather than a bad index build. One precision on the framing, since you asked for pushback. "Strictly no worse than today" is right against pre-module behaviour, but against the module's current behaviour the narrow window is a small regression: today an empty table reports a blocker (wrongly, but it happens to cover that race), and after this PR it reports none. That trade is clearly correct — a guaranteed false block on every fresh bootstrap for a narrow-race false negative whose failure mode is loud and already handled — but it is a trade rather than a strict improvement, and the module comment reads slightly stronger than the facts. I'd also note the race needs the table to gain its first row mid-deploy, which is narrower still. I checked whether an earlier pending migration could populate these tables systematically (a non-racy version of the same concern) and found no data-inserting statements in the migrations directory, so that variant doesn't arise.

Strengths

• The narrowness argument is carried by tests that would fail if it were widened, not by prose: blocks a half-built index regardless of the table being %s over all three populations is the assertion that stops a future "just stop reporting blockers" fix, and the comment at test line 46 says exactly that.
• Extracting decidePreflightBlocker as a pure function makes the load-bearing direction (populated + absent still blocks) testable without a live database, which is the half that had no coverage before.
• probeTablePopulation's to_regclass hop is genuinely necessary and correctly reasoned — without it a fresh bootstrap turns the false block into an undefined_table crash. The reltuples rejection note is also right: it is -1 on a never-analyzed table.
• expect(contents).toMatch(/CREATE (?:UNIQUE )?INDEX "/) is a well-chosen discriminator — the required quote distinguishes the real inline CREATE from the CREATE INDEX CONCURRENTLY <unquoted> text inside the hints, so it cannot be satisfied by hint prose alone.
• Probing each distinct table once (6 of 8 specs share heartbeat_runs) keeps the pre-flight at a handful of round trips rather than one per spec.

Recommended Action

1. No Critical issues.
2. Address the 0237 anchor before merge — it is a two-line change and it restores the guarantee the PR's own correctness argument rests on. The production logic itself needs no change; I verified it against all 8 files.
3. Consider the suggestions opportunistically; the emptiness-regex widening is the only one with any safety content, and its residual risk is small.

Not verified: I did not execute the test suite (no repo checkout or installed dependencies in this runtime). Instead I re-implemented the structural test's three assertions in Node against all 8 migration files fetched at this head, which is how the 0237 anchor was found; the pure-function tests were reviewed by reading, not by running. The fresh-bootstrap run remains unverified, as you noted.