fix(migrations): opt-in dirty auto-heal, raise timeout default, failure alarm

[cristim]

Why

Migrations run lazily via golang-migrate on DB connect, bounded by
defaultMigrationsTimeout (was 20s, internal/server/app.go). Under
concurrent Lambda cold-starts a slow migration (e.g. an index build on a
growing table) blew the 20s bound mid-run, golang-migrate left
schema_migrations.dirty = true, and because dirty is terminal every
later boot fail-opened ("Migration failed - app continuing with existing
schema", app.go:617) and served 500s on any query needing the unapplied
columns. This caused a prod outage (migrations 070-073 never applied;
purchase_delay_hours / revocation_window_closes_at /
executed_by_user_id missing). Recovery today is the manual
CUDLY_FORCE_MIGRATION_VERSION env var.

Root cause = 20s bound too tight + terminal dirty state with no
automated recovery.

What

Three smallest-blast-radius changes:

1. Default-on dirty auto-heal

internal/database/postgres/migrations/migrate.go — new maybeAutoHealDirty
runs after maybeForceMigrationVersion and before Up(). When the DB is
dirty it logs loudly, Force()s the current recorded version to clear the
dirty flag, then Up() re-applies the pending tail, so a cold start
self-recovers instead of staying broken until a manual force (the
multi-hour outage shape). Enabled by default; CUDLY_MIGRATION_AUTOHEAL=false
is the escape hatch for environments whose migrations aren't idempotent.

Two load-bearing safety properties:

• Force the current version, NEVER lower. Forcing below already-applied
  seed migrations re-runs guards that raise on a second run (e.g. 000059
  errors "a group named Purchaser already exists with a different id" because
  000064 already relocated it). Force(current)+Up() is the only safe shape.
• The app ALWAYS starts. If auto-heal still can't reach head (a genuinely
  broken migration file), the error propagates to ensureDB, which fail-opens
  (existing behavior, untouched) and records the failure so the alarm fires —
  never a crash-loop. A broken schema surfaces via /health + the alarm.

CUDLY_FORCE_MIGRATION_VERSION still takes precedence (it runs first and
leaves the row clean). Relies on migrations being idempotent — a documented
invariant now enforced by a test.

2. Raise the default migration timeout

internal/server/app.go — defaultMigrationsTimeout 20s -> 120s, still
well under the 300s Lambda hard limit, so a normal index build can't be killed
mid-run. CUDLY_MIGRATION_TIMEOUT override unchanged.

3. CloudWatch migration-failure alarm

terraform/modules/compute/aws/lambda/migration-alarm.tf — log metric filter
on the "Migration failed" log line + alarm. Because the app fail-opens, the
built-in AWS/Lambda Errors metric stays clean during a broken migration;
this alarm is what surfaces it. Notification target is optional via
alarm_sns_topic_arn (default []) — wire it to the monitoring module's SNS
topic to get paged; no new SNS infra invented. Mirrors the existing
application_errors alarm + metric-filter patterns. terraform fmt +
validate clean.

Bonus: latent migration bug fixed (surfaced by the idempotency test)

Migration 000067 widened savings_snapshots.account_id before dropping
the materialized views that depend on it, so the full stack failed to reach
head on a fresh DB ("cannot alter type of a column used by a view"). Moved the
view DROPs ahead of the ALTER COLUMN. This is exactly the class of latent
migration failure this PR is about. It moved the migrations integration suite
from 4/37 → 28/42 passing; the remaining 14 failures are pre-existing and
unrelated (users.role column, savings-plans split assertions) — filed
separately.

Tests

• TestMigrations_FullStackIdempotent (integration): migrate to head twice;
  the second run is a clean no-op (ErrNoChange) and not dirty. This caught
  and drove the 000067 fix.
• TestMigrations_AutoHealDirty (integration): a dirty DB self-heals by
  default (and with =true); CUDLY_MIGRATION_AUTOHEAL=false disables it so
  a dirty DB errors with the dirty flag left intact (escape hatch).
• TestResolveMigrationsTimeout (unit): default is 120s, honors the override,
  falls back to the default on invalid / non-positive input.

All three new tests pass. go build ./... and
go test ./internal/database/... ./internal/server/... green.

Follow-up

The structural fix — moving migrations off the request/connect path into a
one-shot deploy-time runner — is tracked separately (see the linked
follow-up issue). This PR is the in-place hardening.

Summary by CodeRabbit

• New Features

  • Added automatic migration self-heal (enabled by default).
  • Added CloudWatch metric + alarm for migration failures and module outputs to expose them.

• Improvements

  • Increased migration timeout from 20s to 120s.
  • Made migration startup and recovery more robust and made migrations idempotent on re-run.

• Documentation

  • Expanded migration resilience runbook with fail-open behavior and recovery guidance.

• Tests

  • Added integration tests for auto-heal, idempotency, and a transactional-safety test for migrations.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b2bb7065-d77b-4ef6-8776-9f034cff6ba1

📥 Commits

Reviewing files that changed from the base of the PR and between b91f078 and 7ff2c77.

📒 Files selected for processing (2)

• internal/database/postgres/migrations/migration_transactional_test.go
• specs/migration-resilience.md

---

📝 Walkthrough

Walkthrough

Adds ordered migrator recovery (operator override then default-on dirty auto-heal), increases migration timeout to 120s, makes an analytics migration idempotent, adds unit/integration tests for timeout/auto-heal/idempotency, and wires CloudWatch metric+alarm (optional SNS) for migration failures.

Changes

Migration Resilience Implementation

Layer / File(s)	Summary
Idempotent SQL migration fix internal/database/postgres/migrations/000067_analytics_snapshot_correctness.up.sql	Views are dropped before ALTER operations to prevent dependency conflicts, with redundant drops removed to ensure idempotency on partial installs.
Migration auto-heal and recovery orchestration internal/database/postgres/migrations/migrate.go	newMigratorWithRecovery centralizes migrator setup with ordered recovery: CUDLY_FORCE_MIGRATION_VERSION override first, then default-on dirty auto-heal via maybeAutoHealDirty. When dirty, the migrator is forced to the current recorded version to clear the flag, allowing pending migrations to re-apply. Ensures migrator cleanup on initialization failure.
Integration tests for auto-heal and idempotency internal/database/postgres/migrations/migrate_autoheal_test.go, internal/database/postgres/migrations/migrate_idempotency_test.go	TestMigrations_AutoHealDirty validates auto-heal behavior under default, explicit-true, and false env var settings; TestMigrations_FullStackIdempotent confirms second run is a clean no-op with no dirty flag.
Migration timeout configuration and validation internal/server/app.go, internal/server/app_test.go	Default timeout increased from 20s to 120s; unit test validates resolveMigrationsTimeout() behavior with CUDLY_MIGRATION_TIMEOUT overrides (unset, valid, invalid, non-positive scenarios).
Migration recovery runbook and configuration documentation specs/migration-resilience.md	Recovery procedures expanded with timeout case and auto-heal flow; CUDLY_MIGRATION_TIMEOUT (120s default) and CUDLY_MIGRATION_AUTOHEAL (default-on with disable semantics) documented; CloudWatch alarm setup instructions added.
CloudWatch alarm and SNS integration for migration monitoring terraform/modules/compute/aws/lambda/migration-alarm.tf, terraform/modules/compute/aws/lambda/variables.tf, terraform/modules/compute/aws/lambda/outputs.tf	Log-metric filter detects "Migration failed" pattern and emits MigrationFailed metric; CloudWatch alarm triggers on metric exceeding zero over 5-minute window and routes to optional SNS topics. Module accepts alarm_sns_topic_arn input and exports alarm ARN and metric filter name.
Static transactional-safety test internal/database/postgres/migrations/migration_transactional_test.go	Adds a test that scans migration SQL files (supports opt-out marker) and fails if non-transactional statements are present.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• Move DB migrations off the request/connect path into a one-shot deploy-time runner #1125: Proposes moving migrations to a one-shot deploy runner; this PR hardens the same RunMigrations/ensureDB path (timeout and auto-heal), addressing related deployment reliability concerns.

Possibly related PRs

• LeanerCloud/CUDly#947: Both PRs modify internal/database/postgres/migrations/migrate.go to improve migration resilience — this PR adds pre-Up recovery and auto-heal while the linked PR adjusts admin/bootstrap failure handling.

Suggested labels

urgency/this-sprint

Poem

🐰 I hop through migrations, tidy and bright,
I nudge dirty flags back into the light.
Timeouts stretch longer so slow changes land,
CloudWatch listens with an alarm at hand,
A rabbit's small cheer for resilient plans.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the three main changes: dirty auto-heal feature, migration timeout increase, and CloudWatch failure alarm.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/migration-resilience-autoheal

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

🧹 Nitpick comments (1)

specs/migration-resilience.md (1)

51-56: 💤 Low value

Clarify that alarm_sns_topic_arn accepts a list of SNS topic ARNs.

Line 56 says "Wire its alarm_sns_topic_arn" (singular), but the Terraform variable is type list(string) and accepts multiple SNS topic ARNs. Consider revising to "Wire the alarm_sns_topic_arn variable (a list of SNS topic ARNs)" for clarity.

📝 Suggested wording improvement

-`alarm_sns_topic_arn` to the monitoring module's SNS topic to get paged.
+`alarm_sns_topic_arn` variable (list of SNS topic ARNs) to the monitoring
+module's SNS topics to get paged.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@specs/migration-resilience.md` around lines 51 - 56, Update the documentation
to clarify that the Terraform variable alarm_sns_topic_arn is a list of SNS ARNs
(type list(string)) and may contain multiple ARNs; change the sentence that
currently reads "Wire its `alarm_sns_topic_arn`" to explicitly reference the
variable as a list (e.g., "Wire the `alarm_sns_topic_arn` variable (a list of
SNS topic ARNs) to the monitoring module's SNS topic(s)") so readers understand
multiple ARNs are supported.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@specs/migration-resilience.md`:
- Around line 51-56: Update the documentation to clarify that the Terraform
variable alarm_sns_topic_arn is a list of SNS ARNs (type list(string)) and may
contain multiple ARNs; change the sentence that currently reads "Wire its
`alarm_sns_topic_arn`" to explicitly reference the variable as a list (e.g.,
"Wire the `alarm_sns_topic_arn` variable (a list of SNS topic ARNs) to the
monitoring module's SNS topic(s)") so readers understand multiple ARNs are
supported.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9b68bfde-1c56-441d-906b-d17868055e22

📥 Commits

Reviewing files that changed from the base of the PR and between 61788c1 and b91f078.

📒 Files selected for processing (10)

• internal/database/postgres/migrations/000067_analytics_snapshot_correctness.up.sql
• internal/database/postgres/migrations/migrate.go
• internal/database/postgres/migrations/migrate_autoheal_test.go
• internal/database/postgres/migrations/migrate_idempotency_test.go
• internal/server/app.go
• internal/server/app_test.go
• specs/migration-resilience.md
• terraform/modules/compute/aws/lambda/migration-alarm.tf
• terraform/modules/compute/aws/lambda/outputs.tf
• terraform/modules/compute/aws/lambda/variables.tf

[cristim]

Pushed 7ff2c77c6 with two changes:

1. Transactional-migration enforcement test (internal/database/postgres/migrations/migration_transactional_test.go)

Static guard that reads every *.up.sql / *.down.sql and fails if any file contains a statement PostgreSQL cannot run inside a transaction block: CREATE/DROP INDEX CONCURRENTLY, REINDEX, VACUUM, ALTER SYSTEM, CREATE/DROP DATABASE, CREATE TABLESPACE, REFRESH MATERIALIZED VIEW CONCURRENTLY. golang-migrate runs each file in one implicit transaction, so this guarantees a failed migration rolls back atomically (no partial schema). Orthogonal to the dirty-flag auto-heal; it closes the partial-apply hole.

• Comments and dollar-quoted function/DO bodies are stripped before matching, so CONCURRENTLY inside a stored function (e.g. 000003's refresh_savings_materialized_views()) is correctly benign.
• Opt-out marker -- migrate:no-transaction skips a file with a loud t.Log warning; such files MUST be run via the deploy-time runner (Move DB migrations off the request/connect path into a one-shot deploy-time runner #1125), never the in-request auto-migrate path.
• Verified: passes on all 138 current migrations (none violate it); fails as expected when a CREATE INDEX CONCURRENTLY is injected. go build/go vet/gofmt/gocyclo clean; go test ./internal/database/... ./internal/server/... = 546 passed.

2. Addressed the CR nitpick on specs/migration-resilience.md: clarified that alarm_sns_topic_arn is a list of SNS topic ARNs.

@coderabbitai review

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the PR diff for the latest commit
gh pr view 1124 --repo LeanerCloud/CUDly --json headRefName,headRefOid,commits

# Get the diff of the latest commit
gh api repos/LeanerCloud/CUDly/commits/7ff2c77c6 --jq '.files[] | {filename: .filename, patch: .patch}' 2>/dev/null || true

# Also get the full file content of the new test
gh api repos/LeanerCloud/CUDly/contents/internal/database/postgres/migrations/migration_transactional_test.go?ref=7ff2c77c6 --jq '.content' 2>/dev/null | base64 -d 2>/dev/null || true

Length of output: 21942

---

---

@cristim: The new test file and the doc fix look solid. A few observations and minor gaps worth considering:

---

migration_transactional_test.go — review

✅ Design is sound

• Comment/dollar-body stripping order (block → line → $...$) is correct: /* -- */ can't become a spurious line comment, and an unterminated dollar-quote causes the scanner to retain the remainder conservatively (prevents false-negatives). The opt-out path logs loudly and is easy to grep.
• filepath.Glob("*.sql") works correctly since Go runs unit tests with CWD = package directory.
• The zero-files guard prevents silent mis-configuration.

⚠️ Minor gaps in forbiddenPatterns

Missing	Notes
DROP TABLESPACE	CREATE TABLESPACE is listed; DROP TABLESPACE is equally non-transactional
CLUSTER	Non-transactional (without CONCURRENTLY); rare in migrations but should be covered for completeness

Suggested additions to forbiddenPatterns:

{"DROP TABLESPACE", regexp.MustCompile(`(?is)\bdrop\s+tablespace\b`)},
{"CLUSTER", regexp.MustCompile(`(?is)\bcluster\b`)},

Minor nit — dollarTagRe regex breadth

\$[A-Za-z0-9_]*\$ technically matches digit-only tags like $1$, which aren't valid PostgreSQL dollar-quote tags (PostgreSQL requires the tag to start with a letter or _). In practice this is harmless (no real migration would have $1$ at the top level), but tightening it avoids potential confusion:

var dollarTagRe = regexp.MustCompile(`\$[A-Za-z_][A-Za-z0-9_]*\$|\$\$`)

Minor nit — sanitizeSQL ordering comment

The ordering (block comments → line comments → dollar bodies) is intentional and non-obvious. A one-line comment would make it self-documenting:

// Strip in this order: block comments first (so /* -- */ doesn't seed a line
// comment), then line comments, then dollar-quoted bodies.

---

specs/migration-resilience.md — ✅ clean

The alarm_sns_topic_arn list-clarification reads well.

---

None of the above are blockers. The transactional-guard approach is a solid addition that closes the partial-apply hole; the CLUSTER / DROP TABLESPACE omissions are the only ones worth a follow-up if you want the coverage to be exhaustive.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.