Repair databases that applied migration 39 in its pre-review shape - #199
Conversation
Migration 39 landed in #198 adding a single `title_regeneration_error` column, then was rewritten in place during review to add the three `title_regeneration_failure_*` columns instead. The migrator records applied migrations by numeric id and skips anything at or below the latest recorded id without comparing names, so a database that ran the earlier shape treats the rewritten 39 as done and never grows the failure columns. Every thread and shell snapshot query selects them, so those databases fail on missing columns. Add the columns again under a fresh id. Verified against sqlite: a database seeded with the pre-review 39 gains all three columns from 40, and a database that got them from 39 runs 40 as a no-op. The orphaned `title_regeneration_error` column is left in place — it is nullable and unreferenced, and dropping it would rewrite the table for no benefit.
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:trusted
Review found the new tests passed vacuously. The shared-layer idiom used elsewhere in this directory hands every case in a block the same in-memory database, so once the first case migrated to the head id the migrator skipped every later runMigrations call and the bodies under test never ran again. Removing all three PRAGMA guards from migration 40 still left the suite green, even though an unguarded 40 dies with "duplicate column name" on every fresh install. Provide a fresh database per case and assert against raw PRAGMA rows rather than a Set, which cannot express a duplicate column. Confirmed by negative control: with the guards removed the fresh-install and pending-columns cases now fail with MigrationError, and the stale-39 repair case still passes; with the guards restored all three pass. The 036 test added in #198 had the same flaw and is fixed the same way.
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 535fa6bbf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!names.has("title_regeneration_failure_request_id")) { | ||
| yield* sql` | ||
| ALTER TABLE projection_threads | ||
| ADD COLUMN title_regeneration_failure_request_id TEXT | ||
| `; |
There was a problem hiding this comment.
When the affected database contains a regeneration that failed under the pre-review migration 39, the old implementation leaves title_regeneration_request_id and title_regeneration_started_at populated and stores the reason in title_regeneration_error. Merely adding three nullable columns therefore makes mapTitleRegeneration classify that finished request as pending; during startup, clearInterruptedThreadTitleRegenerations then clears it as an interrupted request without preserving the error. Backfill those rows into the new failure fields (using an available timestamp such as updated_at) and clear the pending fields so the repair preserves the recorded failure rather than silently discarding it.
AGENTS.md reference: AGENTS.md:L25-L25
Useful? React with 👍 / 👎.
Round two found three assertions that could not fail: - 037's "leaves the pending columns untouched" — the migration only issues ADD COLUMN, so it cannot disturb them. - 036's "adds each column exactly once" — SQLite rejects a duplicate ADD COLUMN outright, so the count can never exceed one, leaving 036's PRAGMA guard uncovered. - No case started at 38 with neither 39 nor 40 applied and asserted both run in order, which is the ordering the repair depends on. Replace the first with that mid-sequence case, and rewrite the second to pre-add one of the three columns so the guard is what keeps the migration alive. Extract the per-case database and PRAGMA helpers into migrationTestSupport.ts rather than duplicating them. Negative control across all five cases: removing 036's guards fails 036/skips-existing alone; removing 037's fails 037/fresh-install-guard and 037/mid-sequence-38 alone. Every guard now has a case that fails without it, and all five pass with the guards in place.
Round three established that the test runner works after all — the `vp` on PATH resolves to a build that fails at collection, but `npx vp test run` runs everything. That surfaced a genuine regression from #198: adding titleRegenerationFailure to the hydration query left ProjectionSnapshotQuery's expected thread and shell objects one key short, so that suite has been red on main since the merge. Add the field to both expectations. Full suites now pass: 2613 in apps/server + packages, 1724 in apps/web. Also corrected two comments that claimed more than they deliver. SQLite rejects a duplicate ADD COLUMN outright, so the array-over-Set note in migrationTestSupport overstated what a count assertion can catch; and both shapes of migration 39 reached main in the same merge, so no released build ever carried the earlier one — only machines that ran the PR branch mid-review are affected.
1 participant
Follow-up to #198, from the second Codex review pass on that PR.
Problem
Migration 39 landed on the #198 branch as
ProjectionThreadTitleRegenerationError, adding a singletitle_regeneration_errorcolumn. During review the failure state moved to its own field, and migration 39 was rewritten in place to add the threetitle_regeneration_failure_*columns instead.The migrator skips by numeric id and never compares names:
So a database that ran the earlier shape has 39 recorded as applied, skips the rewritten 39, and never grows the failure columns.
ProjectionSnapshotQueryselects all three in every thread and shell snapshot query, so those databases fail on missing columns.Both shapes of 39 reached
mainin the same merge, so no released build ever carried the earlier one — the affected databases are those on machines that ran the #198 branch mid-review. Narrow, but the failure is hard rather than degraded.Fix
Add the three columns again under a fresh id (40), guarded by
PRAGMA table_infoso it no-ops where 39 already added them.The orphaned
title_regeneration_errorcolumn is left in place on repaired databases: it is nullable and unreferenced (every read and write ofprojection_threadsuses an explicit column list — there is noSELECT *), and dropping a column rewrites the table in SQLite for no benefit.Also fixes a regression #198 left on
main: addingtitleRegenerationFailureto the hydration query leftProjectionSnapshotQuery.test.ts's expected thread and shell objects one key short, so that suite has been red since the merge.Verification
Full suites pass — 2613 tests in
apps/server+packages, 1724 inapps/web.vp run typecheckandvp checkare clean.A note for anyone who hits it: the
vpbinary on PATH resolves to a build that fails at collection withTypeError: Cannot read properties of undefined (reading 'config')on every file, including a two-line smoke test.npx vp test runuses a different version and works. An earlier revision of this description wrongly reported the suite as unrunnable on that basis.Every migration guard is covered by a case that fails without it, confirmed by negative control:
036 > skips columns that already exist(only)037 > no-ops on a fresh database…,037 > applies 39 then 40 in order…Effect.void037 > adds the failure columns to a database that applied the pre-review 39The five cases cover every reachable database state: a fresh install (39 and 40 in one pass), a database that got the columns from the rewritten 39, a database that applied the pre-review 39, a database sitting at 38 with neither applied, and a partially-columned table exercising 036's own guard.
Review
Three adversarial rounds, each of which found something real:
it.layerblock share one in-memory database, so the migrator skipped everyrunMigrationscall after the first and the cases passed vacuously — stripping all three guards still left the suite green. Fixed by giving each case its own database.migrationTestSupport.ts.ProjectionSnapshotQuerytest above; two comments claimed more rigor than they delivered and were corrected.