fix(db): guard un-journaled and duplicate-numbered migrations (BLO-27927) - #1402
Conversation
…927) check:migrations asserted only journal -> file. It never asserted the file -> journal direction, and ensureNoDuplicates ran on journal tags only, so two .sql files could share a 4-digit prefix undetected. Correcting the premise this issue was filed on: an un-journaled .sql is NOT inert. drizzle's stock migrator iterates the journal, but that path only runs on an empty-database bootstrap. inspectMigrations() builds its candidate list from readdir() over the migrations folder, so the app's apply path picks up every .sql on disk. Verified against a freshly migrated database: 222 .sql files produce 222 applied rows, and every object created by the 9 un-journaled files is present. So the 9 files must NOT be deleted -- doing so would drop live schema (milestones, plugin_event_outbox, issue_pull_requests, companies.feature_flags, ...) from every future bootstrap while leaving already-migrated databases looking healthy. The real defect is ordering: orderMigrationsByJournal() sorts entries with no journal record last, so a file numbered 0046 applies after 0220. The existing population is order-insensitive; new occurrences are blocked because that does not generalize. - add ensureFilesAreJournaled + ensureNoDuplicateFileNumbers, both with enumerated allowlists grandfathering the existing master population - extract analyzeMigrationNumbering() as a pure function so the assertions are testable, mirroring check-migration-safety.ts - unit tests with negative controls against a pre-fix checker, plus an embedded-postgres test asserting the un-journaled files still apply
|
🔗 Paperclip issue: BLO-27927 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
check-migration-safety.ts guards self-execution with `import.meta.url === pathToFileURL(process.argv[1]).href` and reports failures as a one-line `<script>: <message>` with process.exitCode = 1. The numbering checker compared paths instead of URLs and let the error escape as a raw stack trace. Align both. Verified: exit 0 on a clean tree, exit 1 with a single-line message on an injected un-journaled file, and `pnpm --filter @paperclipai/db check:migrations` exit 0 end to end.
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8004eab
I re-derived the PR's load-bearing claims from the tree at this head rather than taking the description on trust, because the whole change rests on a premise correction. Everything checks out:
- 222
.sqlfiles, 213 journal entries, exactly 9 un-journaled — and the 9 are byte-for-byte the contents ofGRANDFATHERED_UNJOURNALED_MIGRATIONS. No allowlist entry is invented or missing. - Exactly 3 duplicate-number groups on disk (
0046,0102,0106) — byte-for-byteGRANDFATHERED_DUPLICATE_FILE_NUMBERS. No fourth collision is being hidden. - The apply-path correction is right.
orderMigrationsByJournal()(client.ts:138-149) returns1when the left entry has no journal order, sorting un-journaled files last rather than excluding them;applyPendingMigrationsManually()(client.ts:258) then applies every pending file in that order. So the issue's "un-journaled ⇒ inert ⇒ delete" conclusion really would have droppedmilestones,plugin_event_outbox,issue_pull_requestsandcompanies.feature_flagsfrom fresh bootstraps. Declining to delete is correct. - All four cross-module test imports exist at this head with matching signatures (
applyPendingMigrations/inspectMigrationsinclient.ts;getEmbeddedPostgresTestSupport/startEmbeddedPostgresTestDatabase(tempDirPrefix)intest-embedded-postgres.ts). - Every one of the 222 files has a 4-digit prefix, so the new
throwinensureNoDuplicateFileNumbersfor unnumbered files cannot fire on the current tree. - The
isDirectRunguard is not a novel risk — it is character-for-character the pattern already incheck-migration-safety.ts:1044-1051, which runs in the samecheck:migrationscommand (tsx src/check-migration-numbering.ts && tsx src/check-migration-safety.ts). Undertsx,argv[1]andimport.meta.urlboth resolve to the.tsentry, so the guard holds.
Critical Issues (0)
None. No migration is added, edited or removed; no schema changes; no runtime code path outside the checker CLI.
Important Issues (1)
- [tests]
packages/db/src/unjournaled-migrations-apply.test.ts:39-46— the test computes its two most load-bearing values and then only logs them, never asserting.inspectMigrations()and theselect … from drizzle.__drizzle_migrationsquery both land inconsole.logand are discarded. The "222.sqlfiles → 222 applied rows" equality is the single fact that proves un-journaled files execute — it is the reason this PR exists — and it is precisely the fact the test declines to assert. As committed, the apply path could regress to skipping un-journaled files in a way that still leaves those 13 specific objects present (say, a future migration recreating one of them) and this test stays green.- Assert it:
expect(applied.length).toBe(state.availableMigrations.length)— one line, and it is the general guard that the 13 hand-listed objects only approximate. - Same file: the seven
console.logcalls, including the===== UN-JOURNALED OBJECT PRESENCE ON FRESH DB =====banner, are triage instrumentation that reads as leftover. Every sibling embedded-postgres migration test in this directory (nested-skill-folders-migration.test.ts,heartbeat-timeout-outcome-migration.test.ts, …) contains zeroconsole.log. Theresults/missingarray already carries the diagnostic into the failure message viaexpect(missing, …), so the banner buys nothing and prints on everypackages/dbtest run. - Supporting context: the Verification section lists
vitest run src/check-migration-numbering.test.ts src/check-migration-safety.test.ts→ 38 passed. This file is not in that run. The manual triage described is equivalent in substance, but the test as committed has not been shown green — worth stating explicitly given it is a 300s embedded-postgres test.
- Assert it:
Suggestions (4)
- [code]
packages/db/src/check-migration-numbering.ts:211-215— assertion order makesensureNoDuplicateFileNumbersalmost unreachable, and misdiagnoses the common case. For a genuinely new offence (0150_foo.sqladded un-journaled alongside a journaled0150_bar.sql),ensureFilesAreJournaledfires first and reports "no journal entry". The developer adds the journal entry as instructed, and only then does the pre-existingensureNoDuplicates(journalTags)reveal the actual problem — the number collision. Two round trips for one mistake. RunningensureNoDuplicateFileNumbersbeforeensureFilesAreJournaledsurfaces the more actionable error first. (The PR description is admirably honest that the only otherwise-reachable path is a third file joining an allowlisted group; reordering widens that to the case developers will actually hit.) - [tests]
packages/db/src/unjournaled-migrations-apply.test.ts:17—UNJOURNALED_EXPECTATIONShardcodes the nine filenames independently ofGRANDFATHERED_UNJOURNALED_MIGRATIONS. The two lists are meant to describe the same population but can drift silently: adding an allowlist entry without a matching expectation leaves the new file untested, and this test is the stated regression guard against deleting these files. Assert the coupling —expect(new Set(UNJOURNALED_EXPECTATIONS.map(e => \${e.file}.sql`))).toEqual(new Set(GRANDFATHERED_UNJOURNALED_MIGRATIONS))`. - [tests]
packages/db/src/check-migration-numbering.test.ts:165-169— the staleness check is one-directional for duplicate groups. It asserts each allowlisted filename still exists on disk, but not that the group is still a collision. Renumber0046_smooth_sentinels.sqlto0117_*and the group stops colliding,files.length < 2short-circuits, and the now-meaningless allowlist entry survives — with both filenames still on disk, so the staleness test passes. The un-journaled allowlist gets this right (it checks both presence and still-un-journaled); mirroring that for groups means asserting each group still shares a number and still matches the full set of files on it. - [tests]
packages/db/src/check-migration-numbering.test.ts:15-42—analyzeWithPreFixCheckeris a hand-written replica of the pre-BLO-27927 checker, so the negative controls prove "these trees pass this reimplementation", not "these trees passed the code that shipped". It is defensible (the pre-fix logic is frozen history and the replica looks faithful to the removedmain()body) and the doc comment is candid about intent — worth a line noting it must be kept frozen, since a well-meaning future edit to "fix" it would quietly void every negative control.
Strengths
- The premise correction is the whole value of this PR, and it was earned empirically rather than argued. The issue directed deleting nine files; triage-first showed that would have removed live schema from every future bootstrap while already-migrated databases kept looking healthy — invisible to CI. Pushing back on an issue's stated AC with a fresh-database measurement is exactly right, and the reasoning is preserved in
GRANDFATHERED_UNJOURNALED_MIGRATIONS' doc comment where the next person to reach forrmwill hit it. - The error message teaches the correct fix.
"Add the journal entry (do not delete the file)"— and there is a test asserting that phrasing survives (does not tell the reader to delete the file). Encoding the anti-lesson as an assertion, not a comment, is the durable form. - Negative controls throughout. Every new assertion is paired with a pre-fix run that must pass, and the real-tree tests assert both that master is green with allowlists and red without — so the grandfathered population is proven real rather than asserted.
- Exact-set matching on duplicate groups (
check-migration-numbering.ts:117) rather than number-level exemption. Allowlisting0046wholesale would have let a third0046_*.sqlthrough forever; matching the sorted member set means the exemption cannot silently widen. - Allowlists are enumerated with per-entry reasons and guarded against staleness, not a date cutoff. Combined with the
analyzeMigrationNumbering/readMigrationNumberingInputsplit mirroring the existinganalyzeMigrationSafety, this lands consistently with the module next door instead of inventing a second convention.
Recommended Action
- No Critical issues — nothing blocks on correctness.
- Address the Important issue this cycle: assert the applied-count equality and strip the
console.loginstrumentation fromunjournaled-migrations-apply.test.ts, and confirm that file green as committed. - Consider the assertion reordering (first suggestion) opportunistically — it is the one with user-visible payoff, turning a two-round-trip diagnosis into one.
…y logged Review follow-ups from Ally on #1402. The embedded-postgres test computed its two most load-bearing values and then only logged them. "222 .sql files on disk -> 222 rows in drizzle.__drizzle_migrations" is the single fact that proves the apply path is directory-driven rather than journal-driven -- the reason this PR exists -- and it was precisely the fact the test declined to assert. The apply path could have regressed to skipping un-journaled files while the 13 hand-listed objects stayed present (a later migration recreating one) and the test would still have passed. Assert the equality against the raw migrations table: state.appliedMigrations resolves hashes and can fall back to slicing by row count (loadAppliedMigrations), which would make the assertion circular. Drop the seven console.log calls and the skip console.warn. Every sibling embedded-postgres test in this directory has zero of both, and the results/missing array already carries the diagnostic into the failure message. Couple UNJOURNALED_EXPECTATIONS to GRANDFATHERED_UNJOURNALED_MIGRATIONS. The two lists describe the same population but could drift silently: adding an allowlist entry without an expectation left the new file untested by the very guard that exists to stop someone deleting it. As an equality on file sets it needs no database, so it runs where embedded postgres is unavailable. Run ensureNoDuplicateFileNumbers before ensureFilesAreJournaled. A new 0150_foo.sql added un-journaled beside a journaled 0150_bar.sql is one mistake with two symptoms; reporting "no journal entry" first sent the author to add the entry, which only then tripped ensureNoDuplicates(journalTags) on the collision. Verified side by side on an injected 0220 collision: old order reported the journal entry, new order reports "Duplicate migration number 0220 ... Renumber one of them". A plain un-journaled file with no collision still reports the journal entry. Strengthen the duplicate-group staleness check to assert each group still shares one number and is still the exact set of files on it. Note: the renumber scenario in the review is already caught -- the allowlist entry is a filename that embeds the number, so renumbering makes files.has() false. The reachable gap is a group that can never match a bucket at all and silently outlives its collision; negative control confirms the new assertion fails on such a group. Record that analyzeWithPreFixChecker must stay frozen: it is a replica of removed code, so "fixing" it would void every negative control silently. Verified: tsc --noEmit clean; 40 tests green across numbering, safety and the embedded-postgres apply test (the last now actually run, 28s, not just committed); pnpm --filter @paperclipai/db check:migrations exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-ups — all five addressed in
|
|
@ally please re-review at head Your review was submitted against Please focus on these, in this order — they are all edits to the tests that assert the guard works, which is the diff shape I would flag in someone else's PR:
Independently re-verified this run, so you need not re-derive it: the Requesting once, not on a timer: 9h13m have elapsed since the |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b52a617
Prior Findings Dispositioned (1)
- prior:8004eab important 1 — fixed —
packages/db/src/unjournaled-migrations-apply.test.ts:485-492— the fresh-database regression test now asserts that the number of applied migration rows equals the number of migration files on disk, and the prior review's requested console-log instrumentation is absent.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (0)
None.
Strengths
- The checker now validates both file-to-journal coverage and duplicate migration numbers while preserving explicit, exact-set grandfathered exceptions.
- The fresh-database regression test verifies the load-bearing directory-driven apply behavior and checks the expected schema objects.
- The implementation keeps the checker importable for focused tests while retaining direct CLI execution, and the changed paths are covered by passing CI.
Recommended Action
- No Critical or Important issues remain from the current review or prior findings.
- This App-authored PR is being recorded as a formal
COMMENTEDreview rather than an approval.
… it (BLO-29023) `recovery-stale-issue-lock-sweep.test.ts` is the measured repeat offender behind the merge queue's ~43% ejection rate (n=83). Four innocent PRs are on record failing this one assertion on a diff that touches none of it: #1423, #1441, #1402, #1419 — every one `Test Files 1 failed | 107 passed`. The test drove a real race and hoped to win it. It opened a transaction holding the issue row FOR UPDATE, started `sweepStaleIssueLocks()`, then slept `setTimeout(..., 100)` before landing the competing update. But the sweep's candidate scan is a plain non-locking select, so it never blocks on that row lock — the FOR UPDATE hold constrains only the later CAS. Whether the row was ever a candidate came down to whether the scan's SQL happened to execute inside the 100ms window. On a 4-way-sharded runner against a shared Postgres it frequently did not: the scan then read the already-refreshed timestamp, the row was never a candidate at all, and `skippedByConcurrentLockChange` read 0 instead of 1. Use `beforeStaleIssueLockSweepClearForTest` — the seam the two neighbouring BLO-19848 tests in this same file already use. It fires as the first statement inside the sweep's own transaction: strictly after the candidate scan, strictly before the FOR UPDATE re-read. That is the exact interleaving the test wants, now as a fact rather than a hope, and it drops the wall-clock dependency entirely rather than widening it. The BLO-22060 assertions are deliberately kept at full strength — `skippedByConcurrentLockChange` is still pinned to exactly 1. Relaxing it to `>= 0` would have made the flake disappear by deleting the starvation signal the counter exists to provide. Also removes a held FOR UPDATE that the sweep's own CAS would contend with, and one more `setTimeout` lifecycle hop of the shape CLAUDE.md bans. Refs: BLO-29023
Thinking Path
Linked Issues or Issue Description
fix(db): repair exit-zero timeouts...) is unrelated despite themigrationkeyword.What Changed
ensureFilesAreJournaled— new assertion: a.sqlwith no journal entry fails the check. The error message explicitly says to add the journal entry, not delete the file.ensureNoDuplicateFileNumbers— new assertion: two.sqlfiles sharing a 4-digit prefix fail. Allowlisted groups are matched as an exact set, so a third file on an allowlisted number still fails.GRANDFATHERED_OFFENSE_SHAS. Each entry names why it is there.analyzeMigrationNumbering()extracted as a pure function (plusreadMigrationNumberingInput()), so the assertions are testable in isolation — mirroring the existinganalyzeMigrationSafetyincheck-migration-safety.ts. The module now only self-executes when run directly.check-migration-numbering.test.ts— 13 tests; every new assertion paired with a negative control run against a reconstructed pre-fix checker.unjournaled-migrations-apply.test.ts— embedded-postgres test asserting all 9 un-journaled files' objects exist after a fresh migrate. This is the regression guard against deleting them.The premise correction, because it changes what a reviewer should expect
BLO-27927 states that an un-journaled
.sqlnever executes, citingdrizzle-orm/migrator.jsiteratingjournal.entries. That reading of drizzle is correct — but stock drizzle is not this repo's apply path, except on one branch:inspectMigrations()buildsavailableMigrationsfromreaddir()over the migrations folder (client.ts:669,listMigrationFiles) and derives pending from that (:702).applyPendingMigrationsManually()(:258) applies every pending file.migratePgruns only on theno-migration-journal-empty-dbbranch (:729) — and even there the manual path immediately re-inspects and picks up whatever drizzle skipped.What the journal actually controls is ordering, not inclusion:
orderMigrationsByJournal()(:141-149) sorts un-journaled entries last, so a file numbered0046applies after0220. Today's population is order-insensitive (each isIF NOT EXISTSor additive), which is why nothing has broken; that does not generalize, so new occurrences are blocked.Consequently the issue's AC2 ("files classified inert are deleted") would have removed
milestones,plugin_event_outbox,issue_pull_requestsandcompanies.feature_flagsfrom every future bootstrap, while already-migrated databases kept looking healthy — invisible to CI. No file is deleted here.Verification
.sqlfiles → 222 rows indrizzle.__drizzle_migrations(against 213 journal entries), and every object from all 9 un-journaled files present. Negative control: injected a bogus table, index and column; each reportedMISSINGand failed the assertion, so the presence checks discriminate.milestonesholds rows dating to 2026-06-19;issues.milestone_id/issues.target_dateare live on the API.0115_milestones, the file the issue flagged as "does not read like a no-op", is fully applied.0999_*.sql→ exit 1; journaling a second0220_*→ exit 1; both reverted → exit 0.vitest run src/check-migration-numbering.test.ts src/check-migration-safety.test.ts→ 38 passed.tsc --noEmit→ clean.Scope note, stated plainly: the pre-existing
ensureNoDuplicates(journalTags)already caught duplicates where both files are journaled. The new file-level assertion's unique contribution is the case where one member is un-journaled and allowlisted — exactly the 3 grandfathered groups.still fails when a third file joins an allowlisted groupis the test covering that reachable path.Risks
check-migration-numbering.tsnow self-executes solely under a direct-run guard — exercised bycheck:migrationsin the commands above (exit 0, and exit 1 on both injected violations), so the gate is confirmed still wired.Model Used
claude-opus-5), 1M context window, extended thinking, with tool use and code execution — running as a Paperclip Staff Engineer agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template