Repository navigation
fix(db/migrate): don't auto-heal a rolled-back migration as applied; make idempotency test real (adversarial-review) - #1451
Conversation
…ng effects maybeAutoHealDirty previously called Force(N) on any dirty schema_migrations row and let Up() continue at N+1. golang-migrate records version=N, dirty=true BEFORE running N's SQL; every migration here is transactional (TestMigrationsAreTransactional), so an interrupted migration rolls back completely, leaving dirty=true at N with N's effects ABSENT. The old auto-heal marked the never-applied migration as complete and skipped its DDL forever while schema_migrations reported success (the 000074 divergence class: 42703 errors at runtime on columns the version record claimed were applied). There is also a narrow race where N commits but the dirty=false update fails, leaving dirty=true WITH effects present. These two cases are indistinguishable from schema_migrations alone without per-migration effect probes, so the conservative fix is to fail loud and let the operator inspect the actual schema before choosing the recovery target via CUDLY_FORCE_MIGRATION_VERSION (N if effects confirmed, N-1 if not). The app still fail-opens (ensureDB swallows the error) and the CloudWatch alarm fires; no crash-loop. Test changes: - Update the two "self-heals" test cases to expect a loud error with recovery instructions and verify the dirty flag is left intact. - Add regression test "dirty-at-N with N-effects-absent": rolls back the last migration (effects absent), injects dirty=true at head, and asserts RunMigrations errors without Force-clearing the flag. - Correct TestMigrations_FullStackIdempotent docstring: the second RunMigrations call returns ErrNoChange without running any SQL, so it does not verify per-migration idempotency; remove the false claim that it "guards the whole directory" and that auto-heal relied on it.
|
@coderabbitai review |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 57 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 Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 57 minutes. |
Summary
Two migration-safety defects found by adversarial review of committed origin/main, both in
internal/database/postgres/migrations/migrate.goand its tests:Defect 1 (HIGH):
maybeAutoHealDirtysilently marks a rolled-back migration as appliedgolang-migrate records
version=N, dirty=trueBEFORE running N's SQL. Every migration in this repo is transactional (TestMigrationsAreTransactional), so an interrupted migration (Lambda timeout, connection drop) rolls back completely -- N's effects are ABSENT -- but the dirty flag persists.The old
Force(N)+Up()auto-heal treated the rolled-back migration as fully applied and continued at N+1, permanently skipping N's DDL whileschema_migrationsreported success. This is the 000074 divergence class: 42703 "column does not exist" errors at runtime on columns the version record claimed were present.There is also a narrow race where N commits but the
dirty=falseupdate fails (N's effects ARE present). These two cases are indistinguishable without per-migration effect probes, so the conservative fix is:maybeAutoHealDirtynow returns a loud error with recovery instructions when dirty is detected, rather than silently clearing the flag.CUDLY_FORCE_MIGRATION_VERSION=N(if effects confirmed) orN-1(if not) to recover.CUDLY_MIGRATION_AUTOHEAL=falsestill bypasses the check (surfaces the generic Up() dirty error instead).Defect 2 (MED):
TestMigrations_FullStackIdempotentfalsely claimed to guard per-migration idempotencyThe test ran
RunMigrationstwice, but the second call returnsErrNoChangefrom golang-migrate without executing any SQL -- no migration body is actually re-run. The docstring incorrectly claimed "this is the invariant the opt-in CUDLY_MIGRATION_AUTOHEAL path relies on" and that it "guards the whole directory."Fix: corrected the docstring to accurately describe the test (verifies a no-op second call on a fully-migrated DB) and removed the false auto-heal dependency claim. The conservative auto-heal no longer relies on per-migration idempotency.
Changes
migrate.go: ReplaceForce(N)+continueinmaybeAutoHealDirtywith a loud error containing structured recovery instructions.migrate_autoheal_test.go: Update the two "self-heals" test cases to expect errors and verify the dirty flag is left intact. Add regression testdirty-at-N with N-effects-absent(rolls back last migration, injects dirty=true at head, asserts error + dirty flag preserved).migrate_idempotency_test.go: Correct the false docstring; remove the unverified "guards the whole directory" claim.Test plan
go build ./internal/database/...passesgo vet ./internal/database/...passesgo test ./internal/database/...(non-integration): 181 passedgo test -tags integration ./internal/database/postgres/migrations/...-- CI will run thesedirty-at-N with N-effects-absentverifies Force(N) is NOT called on a rolled-back migration