diff --git a/internal/database/postgres/migrations/000067_analytics_snapshot_correctness.up.sql b/internal/database/postgres/migrations/000067_analytics_snapshot_correctness.up.sql index ff4a894e0..1dfa15c89 100644 --- a/internal/database/postgres/migrations/000067_analytics_snapshot_correctness.up.sql +++ b/internal/database/postgres/migrations/000067_analytics_snapshot_correctness.up.sql @@ -35,6 +35,20 @@ -- target state rather than no-op'ing over a wrong column type (project rule -- feedback_migration_full_restore). +-- ------------------------------------------------------------------ +-- H3 (prerequisite): drop the materialized views FIRST. +-- monthly_savings_summary / daily_savings_trend / provider_savings_summary +-- all reference savings_snapshots.account_id, so the H4 ALTER COLUMN TYPE +-- below fails with "cannot alter type of a column used by a view or rule" +-- while they still exist. They are unconditionally recreated further down, +-- so dropping them up front is safe and makes this migration applyable on a +-- fresh DB (where an earlier migration already created them). DROP ... IF +-- EXISTS keeps the step idempotent across re-runs / auto-heal. +-- ------------------------------------------------------------------ +DROP MATERIALIZED VIEW IF EXISTS provider_savings_summary CASCADE; +DROP MATERIALIZED VIEW IF EXISTS daily_savings_trend CASCADE; +DROP MATERIALIZED VIEW IF EXISTS monthly_savings_summary CASCADE; + -- ------------------------------------------------------------------ -- H4: widen account_id on the partitioned parent. -- Postgres propagates the type change to all existing partitions. @@ -71,12 +85,9 @@ ALTER TABLE savings_snapshots ALTER COLUMN coverage_percentage DROP DEFAULT; -- DROP + CREATE because a materialized view's column list / GROUP BY -- cannot be altered in place. Unique indexes are recreated for the -- CONCURRENTLY refresh path. AVG(coverage_percentage) now skips NULLs --- automatically (H2). +-- automatically (H2). The DROPs ran above (before the ALTER COLUMN) so +-- the CREATEs below land on a clean slate. -- ------------------------------------------------------------------ -DROP MATERIALIZED VIEW IF EXISTS provider_savings_summary CASCADE; -DROP MATERIALIZED VIEW IF EXISTS daily_savings_trend CASCADE; -DROP MATERIALIZED VIEW IF EXISTS monthly_savings_summary CASCADE; - CREATE MATERIALIZED VIEW monthly_savings_summary AS SELECT DATE_TRUNC('month', timestamp) as month, diff --git a/internal/database/postgres/migrations/migrate.go b/internal/database/postgres/migrations/migrate.go index 846d66274..c1276bde6 100644 --- a/internal/database/postgres/migrations/migrate.go +++ b/internal/database/postgres/migrations/migrate.go @@ -31,27 +31,14 @@ const defaultAdminGroupID = "00000000-0000-5000-8000-000000000001" // adminEmail is optional - if provided, admin user will be created after migrations complete // adminPassword is optional - if provided, admin is created with hashed password and active=true func RunMigrations(ctx context.Context, pool *pgxpool.Pool, migrationsPath string, adminEmail string, adminPassword string) error { - // Get database connection string from pool config (without admin email parameter - RDS Proxy doesn't support options) - dsn := buildMigrateDSN(pool.Config(), "") - - // Create migrator - m, err := migrate.New( - fmt.Sprintf("file://%s", migrationsPath), - dsn, - ) + // Create the migrator and run the pre-Up recovery hooks (operator force, + // then default-on dirty auto-heal). Kept in a helper so RunMigrations stays + // under the cyclomatic-complexity budget as recovery paths grow. + m, err := newMigratorWithRecovery(pool, migrationsPath) if err != nil { - return fmt.Errorf("failed to create migrator: %w", err) - } - defer m.Close() - - // One-shot operator recovery: if CUDLY_FORCE_MIGRATION_VERSION is set, - // call Force(N) before Up(). Clears the dirty flag and pins state to - // the given version. Used to recover from a partially-applied - // migration without direct DB access. Remove the env var after the - // next successful deploy. - if err := maybeForceMigrationVersion(m); err != nil { return err } + defer m.Close() // Run migrations if err := m.Up(); err != nil && err != migrate.ErrNoChange { @@ -85,6 +72,52 @@ func RunMigrations(ctx context.Context, pool *pgxpool.Pool, migrationsPath strin return nil } +// newMigratorWithRecovery builds the golang-migrate migrator for migrationsPath +// and runs the pre-Up recovery hooks in order: the one-shot operator force +// (CUDLY_FORCE_MIGRATION_VERSION) first, then the default-on dirty auto-heal. +// On success it returns a migrator the caller must Close(); on any error it +// closes the migrator itself and returns the error so the caller never sees a +// half-initialized migrator. +// +// Ordering note: maybeAutoHealDirty runs AFTER maybeForceMigrationVersion so an +// explicit force always takes precedence (it pins+cleans first, leaving nothing +// dirty for auto-heal to act on). The auto-heal error propagates so a heal +// failure is recorded (and the app fail-opens in ensureDB) rather than being +// masked by the later dirty check. +func newMigratorWithRecovery(pool *pgxpool.Pool, migrationsPath string) (*migrate.Migrate, error) { + // Get database connection string from pool config (without admin email parameter - RDS Proxy doesn't support options) + dsn := buildMigrateDSN(pool.Config(), "") + + m, err := migrate.New( + fmt.Sprintf("file://%s", migrationsPath), + dsn, + ) + if err != nil { + return nil, fmt.Errorf("failed to create migrator: %w", err) + } + + // One-shot operator recovery: if CUDLY_FORCE_MIGRATION_VERSION is set, + // call Force(N) before Up(). Clears the dirty flag and pins state to + // the given version. Used to recover from a partially-applied + // migration without direct DB access. Remove the env var after the + // next successful deploy. + if err := maybeForceMigrationVersion(m); err != nil { + m.Close() + return nil, err + } + + // Default-on dirty auto-heal: when the schema_migrations row is dirty, + // clear the dirty flag at the CURRENT recorded version so the subsequent + // Up() re-applies any pending migrations, letting a cold start self-recover + // instead of staying broken until a manual force. + if err := maybeAutoHealDirty(m); err != nil { + m.Close() + return nil, err + } + + return m, nil +} + // ensureAdminUser creates the admin user if it doesn't exist. // When password is provided, the admin is created with a bcrypt-hashed password and active=true. // When password is empty, the admin is created inactive and must use password reset to log in. @@ -292,6 +325,93 @@ func maybeForceMigrationVersion(m *migrate.Migrate) error { return nil } +// maybeAutoHealDirty self-recovers a dirty schema_migrations row: when the DB +// is dirty it calls m.Force(currentRecordedVersion) to clear the dirty flag at +// the version golang-migrate last recorded, and the caller's subsequent m.Up() +// re-applies only the still-pending migrations. This runs ONCE per boot, so a +// cold start that finds a dirty DB self-heals instead of staying broken until +// an operator manually forces a version (the multi-hour outage this addresses). +// +// DEFAULT-ON (not opt-in). The whole outage was "the app started but stayed +// broken for hours needing a manual force", and TestMigrations_FullStackIdempotent +// proves re-running migrations is safe, so auto-heal runs by default. Set the +// escape hatch CUDLY_MIGRATION_AUTOHEAL=false (any strconv.ParseBool falsey +// value) to disable it in an environment whose migrations are not idempotent. +// When disabled, a dirty DB is left untouched here and surfaces as the usual +// "database is in dirty state" error after Up(); the caller (app.go ensureDB) +// still FAIL-OPENS on that error so the app always starts -- a broken schema +// surfaces via /health + the CloudWatch alarm, never via a crash-loop. +// +// CRITICAL -- Force to the CURRENT recorded version, NEVER a lower one. Up() +// then applies only the pending tail. Forcing BELOW already-applied migrations +// would re-run seed/data migrations, and some RAISE on a second run (e.g. +// 000059 errors with "a group named Purchaser already exists with a different +// id" because 000064 already relocated it). Force(current)+Up() is the only +// safe shape; Force(lower) would turn a recoverable dirty state into a hard +// failure. (Proven live during the incident.) +// +// IDEMPOTENCY INVARIANT: Force(version) clears the dirty marker WITHOUT rolling +// back the partial effects of the interrupted migration, so when Up() re-runs +// the pending tail those migrations must tolerate already-applied state +// (CREATE ... IF NOT EXISTS, DROP ... IF EXISTS, DO-blocks that no-op when the +// target already exists). The full-stack idempotency test guards this for the +// whole directory as migrations are added. +// +// CUDLY_FORCE_MIGRATION_VERSION still takes precedence: it runs earlier and +// leaves the row clean, so this is a no-op when both are set. If auto-heal +// itself fails (e.g. Force errors, or the re-applied Up() still fails), the +// error propagates to ensureDB, which fail-opens AND records the failure so +// the migration-failed alarm fires -- the app still starts. +// +// The ParseBool gate is duplicated from internal/server.getEnvBool rather than +// imported because the migrations package must not depend on internal/server, +// which would invert the dependency direction. +func maybeAutoHealDirty(m *migrate.Migrate) error { + if !autoHealEnabled() { + return nil + } + + version, dirty, err := m.Version() + if err == migrate.ErrNilVersion { + // No migrations recorded yet -> nothing to heal. + return nil + } + if err != nil { + return fmt.Errorf("auto-heal: failed to read migration version: %w", err) + } + if !dirty { + return nil + } + + // Force the CURRENT recorded version (never lower -- see the doc comment), + // then let the caller's Up() re-apply only the pending tail. + log.Printf("Database is DIRTY at version %d: auto-heal forcing the current version %d to clear the dirty flag, then re-applying pending migrations (set CUDLY_MIGRATION_AUTOHEAL=false to disable)", version, version) + if err := m.Force(int(version)); err != nil { + return fmt.Errorf("auto-heal: failed to force version %d to clear dirty flag: %w", version, err) + } + log.Printf("Auto-heal cleared dirty flag at version %d; proceeding to re-apply pending migrations", version) + return nil +} + +// autoHealEnabled reports whether dirty auto-heal should run. DEFAULT-ON: it +// returns true unless CUDLY_MIGRATION_AUTOHEAL is explicitly set to a +// strconv.ParseBool falsey value (0/f/false/...). Unset, empty, or unparseable +// values keep auto-heal enabled. Kept local to the migrations package to avoid +// an inverted dependency on internal/server (see maybeAutoHealDirty). +func autoHealEnabled() bool { + v := os.Getenv("CUDLY_MIGRATION_AUTOHEAL") + if v == "" { + return true + } + b, err := strconv.ParseBool(v) + if err != nil { + // Unparseable value -> keep the safe default (enabled) rather than + // silently disabling self-recovery on a typo. + return true + } + return b +} + // RollbackMigrations rolls back N migrations func RollbackMigrations(ctx context.Context, pool *pgxpool.Pool, migrationsPath string, steps int) error { if steps <= 0 { diff --git a/internal/database/postgres/migrations/migrate_autoheal_test.go b/internal/database/postgres/migrations/migrate_autoheal_test.go new file mode 100644 index 000000000..f3b248a12 --- /dev/null +++ b/internal/database/postgres/migrations/migrate_autoheal_test.go @@ -0,0 +1,116 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// markDirty forces schema_migrations into the dirty state at the given version, +// reproducing what golang-migrate leaves behind when a migration is interrupted +// mid-run (Lambda timeout, ENI drop, etc.). golang-migrate keeps exactly one +// row in this table. +func markDirty(ctx context.Context, t *testing.T, container *testhelpers.PostgresContainer, version uint) { + t.Helper() + _, err := container.DB.Exec(ctx, + `UPDATE schema_migrations SET version = $1, dirty = true`, version) + require.NoError(t, err, "failed to mark schema_migrations dirty") +} + +// TestMigrations_AutoHealDirty covers the DEFAULT-ON CUDLY_MIGRATION_AUTOHEAL +// behavior from migrate.go: +// +// - BY DEFAULT (flag unset), a dirty DB is healed: Force(current) clears the +// dirty flag and the subsequent Up() reaches head clean -- a cold start +// self-recovers without operator intervention. +// - With CUDLY_MIGRATION_AUTOHEAL=false, auto-heal is disabled and a dirty DB +// still returns an error with the dirty flag left intact (escape hatch). +// +// In both cases the Force target is the CURRENT recorded version (never lower), +// which is the only safe shape -- forcing below already-applied migrations +// would re-run guarded seed migrations that raise on a second run. +func TestMigrations_AutoHealDirty(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + t.Run("by default a dirty DB self-heals and head is reached", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Migrate to head, then simulate an interrupted migration by forcing + // the dirty flag at the head version. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + headVersion, _, err := migrations.GetMigrationVersion(ctx, pool, migrationsPath) + require.NoError(t, err) + require.Greater(t, headVersion, uint(0)) + markDirty(ctx, t, container, headVersion) + + // Sanity: confirm the DB is genuinely dirty before healing. + _, dirtyBefore, err := migrations.GetMigrationVersion(ctx, pool, migrationsPath) + require.NoError(t, err) + require.True(t, dirtyBefore, "precondition: DB must be dirty before the heal run") + + // Flag explicitly unset: auto-heal is default-on, so the re-run must + // self-recover. t.Setenv auto-restores and forbids t.Parallel(). + t.Setenv("CUDLY_MIGRATION_AUTOHEAL", "") + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", ""), + "default-on auto-heal should clear the dirty flag and reach head without error") + + versionAfter, dirtyAfter, err := migrations.GetMigrationVersion(ctx, pool, migrationsPath) + require.NoError(t, err) + assert.False(t, dirtyAfter, "auto-heal must clear the dirty flag") + assert.Equal(t, headVersion, versionAfter, "auto-heal must leave the DB at head") + }) + + t.Run("with CUDLY_MIGRATION_AUTOHEAL=true a dirty DB self-heals", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + headVersion, _, err := migrations.GetMigrationVersion(ctx, pool, migrationsPath) + require.NoError(t, err) + markDirty(ctx, t, container, headVersion) + + t.Setenv("CUDLY_MIGRATION_AUTOHEAL", "true") + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", ""), + "explicit true should also clear the dirty flag and reach head") + + versionAfter, dirtyAfter, err := migrations.GetMigrationVersion(ctx, pool, migrationsPath) + require.NoError(t, err) + assert.False(t, dirtyAfter, "auto-heal must clear the dirty flag") + assert.Equal(t, headVersion, versionAfter, "auto-heal must leave the DB at head") + }) + + t.Run("CUDLY_MIGRATION_AUTOHEAL=false disables auto-heal and a dirty DB errors", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + headVersion, _, err := migrations.GetMigrationVersion(ctx, pool, migrationsPath) + require.NoError(t, err) + markDirty(ctx, t, container, headVersion) + + // Escape hatch: the falsey value disables auto-heal, so the dirty error + // must surface (the caller fail-opens on it; the app still starts). + t.Setenv("CUDLY_MIGRATION_AUTOHEAL", "false") + err = migrations.RunMigrations(ctx, pool, migrationsPath, "", "") + require.Error(t, err, "CUDLY_MIGRATION_AUTOHEAL=false must disable auto-heal so a dirty DB errors") + + _, dirtyAfter, verr := migrations.GetMigrationVersion(ctx, pool, migrationsPath) + require.NoError(t, verr) + assert.True(t, dirtyAfter, "with auto-heal disabled, the dirty flag must be left intact") + }) +} diff --git a/internal/database/postgres/migrations/migrate_idempotency_test.go b/internal/database/postgres/migrations/migrate_idempotency_test.go new file mode 100644 index 000000000..5626e15aa --- /dev/null +++ b/internal/database/postgres/migrations/migrate_idempotency_test.go @@ -0,0 +1,53 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMigrations_FullStackIdempotent proves the entire migration stack is +// idempotent: running it to head twice against a fresh DB leaves the second +// run a no-op and the schema_migrations row clean (not dirty). +// +// This is the invariant the opt-in CUDLY_MIGRATION_AUTOHEAL path relies on +// (maybeAutoHealDirty in migrate.go Force()s past a dirty version and lets +// Up() re-apply the pending tail). If a migration were NOT idempotent, a +// re-apply would error or corrupt data, so this test guards the whole +// directory as new migrations are added. +func TestMigrations_FullStackIdempotent(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // First run: migrate to head on a fresh DB. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", ""), + "first migration run to head should succeed on a fresh DB") + + versionAfterFirst, dirtyAfterFirst, err := migrations.GetMigrationVersion(ctx, pool, migrationsPath) + require.NoError(t, err) + require.False(t, dirtyAfterFirst, "DB must not be dirty after the first run") + require.Greater(t, versionAfterFirst, uint(0), "head version should be > 0") + + // Second run: must be a no-op (golang-migrate's m.Up() returns ErrNoChange, + // which RunMigrations swallows) and must not flip the dirty flag. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", ""), + "second migration run must be a clean no-op (ErrNoChange), proving idempotency") + + versionAfterSecond, dirtyAfterSecond, err := migrations.GetMigrationVersion(ctx, pool, migrationsPath) + require.NoError(t, err) + assert.False(t, dirtyAfterSecond, "DB must not be dirty after the second (no-op) run") + assert.Equal(t, versionAfterFirst, versionAfterSecond, + "version must be unchanged after a no-op second run") +} diff --git a/internal/database/postgres/migrations/migration_transactional_test.go b/internal/database/postgres/migrations/migration_transactional_test.go new file mode 100644 index 000000000..36fda7004 --- /dev/null +++ b/internal/database/postgres/migrations/migration_transactional_test.go @@ -0,0 +1,168 @@ +// Package migrations: transactional-safety invariant test. +// +// golang-migrate runs each migration file inside one implicit transaction +// (the postgres driver wraps every *.up.sql / *.down.sql in BEGIN ... COMMIT). +// That property is what makes a failed migration roll back atomically: either +// the whole file applies or none of it does, so we never end up with a +// half-applied schema. This test enforces that invariant statically by +// failing if any migration contains a statement that PostgreSQL refuses to +// run inside a transaction block (e.g. CREATE INDEX CONCURRENTLY, VACUUM, +// ALTER SYSTEM, CREATE/DROP DATABASE, CREATE TABLESPACE, REINDEX). +// +// Why this matters: this guard is orthogonal to the dirty-flag auto-heal +// hardening in this PR. Auto-heal recovers a migration whose transaction was +// interrupted; this test closes the *partial-apply* hole, where a +// non-transactional statement could leave the schema half-changed even though +// the migration "failed", because that statement committed outside the +// transaction the rest of the file ran in. Keeping every migration +// transactional means the dirty flag + auto-heal always describe an +// all-or-nothing state. +// +// Opt-out: a migration that genuinely must run non-transactionally may declare +// itself by putting the exact marker comment +// +// -- migrate:no-transaction +// +// on (one of) the first lines of the file. Such files are SKIPPED by the +// forbidden-statement scan, but the test logs a loud warning naming them. +// These files MUST be applied via the one-shot, deploy-time migration runner +// (follow-up issue #1125), NEVER via the in-request auto-migrate path on the +// hot request handler, because outside a transaction a failure there leaves a +// partially-applied schema with no rollback. +// +// The scanner strips SQL comments and dollar-quoted bodies ($$...$$, $tag$...$tag$) +// before matching, so CONCURRENTLY mentioned in a comment or stored inside a +// CREATE FUNCTION / DO body (which is only executed later, outside the +// migration transaction) does not trip the check. Only top-level statements +// that the migration itself executes are scanned. +package migrations + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// noTransactionMarker, when present in a migration file, declares it as an +// intentional, deploy-time-only non-transactional migration (see issue #1125). +const noTransactionMarker = "-- migrate:no-transaction" + +// forbiddenPatterns are statements PostgreSQL cannot run inside a transaction +// block. Each is matched case-insensitively against the comment-and-body-stripped +// SQL with whitespace tolerance and word boundaries. +var forbiddenPatterns = []struct { + name string + re *regexp.Regexp +}{ + {"CREATE INDEX CONCURRENTLY", regexp.MustCompile(`(?is)\bcreate\s+(?:unique\s+)?index\s+concurrently\b`)}, + {"DROP INDEX CONCURRENTLY", regexp.MustCompile(`(?is)\bdrop\s+index\s+concurrently\b`)}, + {"REINDEX", regexp.MustCompile(`(?is)\breindex\b`)}, + {"VACUUM", regexp.MustCompile(`(?is)\bvacuum\b`)}, + {"ALTER SYSTEM", regexp.MustCompile(`(?is)\balter\s+system\b`)}, + {"CREATE DATABASE", regexp.MustCompile(`(?is)\bcreate\s+database\b`)}, + {"DROP DATABASE", regexp.MustCompile(`(?is)\bdrop\s+database\b`)}, + {"CREATE TABLESPACE", regexp.MustCompile(`(?is)\bcreate\s+tablespace\b`)}, + // REFRESH MATERIALIZED VIEW CONCURRENTLY also cannot run in a txn. A bare, + // top-level one is a hazard; one inside a function/DO body is fine and is + // already stripped before matching, so this only fires at top level. + {"REFRESH MATERIALIZED VIEW CONCURRENTLY", regexp.MustCompile(`(?is)\brefresh\s+materialized\s+view\s+concurrently\b`)}, +} + +// lineCommentRe matches a -- comment to end of line. +var lineCommentRe = regexp.MustCompile(`--[^\n]*`) + +// blockCommentRe matches /* ... */ comments (non-greedy, across newlines). +var blockCommentRe = regexp.MustCompile(`(?s)/\*.*?\*/`) + +// dollarTagRe matches a dollar-quote opening tag, e.g. $$ or $func$. +var dollarTagRe = regexp.MustCompile(`\$[A-Za-z0-9_]*\$`) + +// stripDollarBodies removes the contents of dollar-quoted strings (function +// and DO bodies). These are stored/parsed-later text, not statements the +// migration transaction executes, so CONCURRENTLY etc. inside them is benign. +// Matching is tag-aware: a body opened with $tag$ closes only on the same +// $tag$, per PostgreSQL dollar-quoting rules. +func stripDollarBodies(sql string) string { + var b strings.Builder + for { + loc := dollarTagRe.FindStringIndex(sql) + if loc == nil { + b.WriteString(sql) + break + } + b.WriteString(sql[:loc[0]]) + tag := sql[loc[0]:loc[1]] + rest := sql[loc[1]:] + end := strings.Index(rest, tag) + if end == -1 { + // Unterminated dollar quote: keep the tag and the remainder as-is + // rather than silently dropping it, so a malformed file is still + // scanned conservatively. + b.WriteString(tag) + b.WriteString(rest) + break + } + // Drop the body (rest[:end]) and the closing tag; continue after it. + sql = rest[end+len(tag):] + } + return b.String() +} + +// sanitizeSQL removes comments and dollar-quoted bodies so the forbidden-pattern +// scan only sees top-level statements the migration actually executes. +func sanitizeSQL(sql string) string { + sql = blockCommentRe.ReplaceAllString(sql, " ") + sql = lineCommentRe.ReplaceAllString(sql, " ") + sql = stripDollarBodies(sql) + return sql +} + +func TestMigrationsAreTransactional(t *testing.T) { + files, err := filepath.Glob("*.sql") + if err != nil { + t.Fatalf("globbing migration files: %v", err) + } + if len(files) == 0 { + t.Fatal("no *.sql migration files found; test is in the wrong directory or migrations vanished") + } + + var scanned, skipped int + for _, f := range files { + name := filepath.Base(f) + if !strings.HasSuffix(name, ".up.sql") && !strings.HasSuffix(name, ".down.sql") { + continue + } + + raw, err := os.ReadFile(f) + if err != nil { + t.Fatalf("reading %s: %v", name, err) + } + content := string(raw) + + if strings.Contains(content, noTransactionMarker) { + skipped++ + t.Logf("WARNING: %s is declared non-transactional via %q and is SKIPPED by the "+ + "transactional-safety scan. It MUST be applied via the one-shot deploy-time "+ + "migration runner (issue #1125), never the in-request auto-migrate path.", + name, noTransactionMarker) + continue + } + + scanned++ + sanitized := sanitizeSQL(content) + for _, p := range forbiddenPatterns { + if p.re.MatchString(sanitized) { + t.Errorf("%s contains a non-transactional statement (%s). golang-migrate runs "+ + "each migration in one implicit transaction, so this statement would either "+ + "fail outright or commit outside that transaction and leave a partially-applied "+ + "schema. Move it to a deploy-time runner and mark the file with %q (see #1125), "+ + "or rewrite it transactionally.", + name, p.name, noTransactionMarker) + } + } + } + + t.Logf("transactional-safety scan: %d migration files scanned, %d skipped (opt-out)", scanned, skipped) +} diff --git a/internal/server/app.go b/internal/server/app.go index cd4fd8579..39b7b227c 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -148,11 +148,14 @@ type ExternalDeps struct { } // defaultMigrationsTimeout bounds how long ensureDB waits for migrations -// before giving up and proceeding. Deliberately shorter than the default -// Lambda timeout (30s at this writing) so a runaway migration gets -// cancelled cleanly inside ensureDB rather than by Lambda mid-invocation -// (which is exactly what leaves schema_migrations.dirty = true). -const defaultMigrationsTimeout = 20 * time.Second +// before giving up and proceeding. Set well above the time a normal index +// build / DDL takes (the prior 20s could be blown mid-run by a single index +// build on a growing table, leaving schema_migrations.dirty = true and +// fail-opening every later boot) yet still comfortably under the Lambda +// 300s hard limit, so a slow-but-legitimate migration completes rather than +// being killed inside ensureDB. Override per-environment with +// CUDLY_MIGRATION_TIMEOUT (e.g. "180s") for genuinely long migrations. +const defaultMigrationsTimeout = 120 * time.Second // resolveMigrationsTimeout reads CUDLY_MIGRATION_TIMEOUT from the environment. // It is called once in NewApplicationFromDeps to initialise diff --git a/internal/server/app_test.go b/internal/server/app_test.go index 53234c1dd..481918df2 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -687,6 +687,47 @@ func TestEnsureDB_UsesInstanceMigrationsTimeout(t *testing.T) { <-slow } +// TestResolveMigrationsTimeout pins the default migration timeout and the +// CUDLY_MIGRATION_TIMEOUT override contract. The default was raised from 20s +// to 120s so a normal index build can't be killed mid-run (which left +// schema_migrations dirty and fail-opened every later boot). Not parallel: +// resolveMigrationsTimeout reads a process-global env var. +func TestResolveMigrationsTimeout(t *testing.T) { + // defaultMigrationsTimeout is the compile-time constant; assert its value + // directly so a future accidental change to it trips this test. + if defaultMigrationsTimeout != 120*time.Second { + t.Fatalf("defaultMigrationsTimeout = %s, want 120s", defaultMigrationsTimeout) + } + + t.Run("unset returns the 120s default", func(t *testing.T) { + t.Setenv("CUDLY_MIGRATION_TIMEOUT", "") + if got := resolveMigrationsTimeout(); got != 120*time.Second { + t.Fatalf("resolveMigrationsTimeout() = %s, want 120s", got) + } + }) + + t.Run("valid override is honored", func(t *testing.T) { + t.Setenv("CUDLY_MIGRATION_TIMEOUT", "180s") + if got := resolveMigrationsTimeout(); got != 180*time.Second { + t.Fatalf("resolveMigrationsTimeout() = %s, want 180s", got) + } + }) + + t.Run("invalid override falls back to the default", func(t *testing.T) { + t.Setenv("CUDLY_MIGRATION_TIMEOUT", "not-a-duration") + if got := resolveMigrationsTimeout(); got != 120*time.Second { + t.Fatalf("resolveMigrationsTimeout() = %s, want 120s default on invalid input", got) + } + }) + + t.Run("non-positive override falls back to the default", func(t *testing.T) { + t.Setenv("CUDLY_MIGRATION_TIMEOUT", "0s") + if got := resolveMigrationsTimeout(); got != 120*time.Second { + t.Fatalf("resolveMigrationsTimeout() = %s, want 120s default on non-positive input", got) + } + }) +} + // ---------- runMigrationsBoundedWith tests ---------- // // These tests exercise the goroutine+timeout+recover logic in isolation by diff --git a/specs/migration-resilience.md b/specs/migration-resilience.md index f8f16ec7f..c06c01829 100644 --- a/specs/migration-resilience.md +++ b/specs/migration-resilience.md @@ -48,6 +48,14 @@ category of pages: schema will return 500s at query time. Follow the "Recovery" section below. +On AWS, you do not have to rely on polling `/health`: the Lambda compute +module ships a CloudWatch alarm (`-migration-failed`) backed by a +log metric filter on the `"Migration failed"` log line. Because the app +fail-opens, the built-in `AWS/Lambda` `Errors` metric stays clean during a +broken migration; this alarm is what surfaces it. Wire the +`alarm_sns_topic_arn` variable (a list of SNS topic ARNs) to the monitoring +module's SNS topic(s) to get paged. + ## Recovery from a failed migration 1. **Inspect logs to find the specific error.** @@ -63,39 +71,71 @@ category of pages: - `Dirty database version N. Fix and force version.` — migration N was interrupted mid-run (Lambda timeout, ENI drop, etc.) and `schema_migrations.dirty = true`. - - `migration timed out after 20s` — migration N ran longer than + - `migration timed out after 120s` — migration N ran longer than `CUDLY_MIGRATION_TIMEOUT`. Either the migration is genuinely long (DDL on large table) or the DB is slow. Tune `CUDLY_MIGRATION_TIMEOUT` upwards and redeploy. - SQL errors (constraint violations, missing tables, etc.) — the migration file itself is broken. Fix the file, redeploy. -3. **Clear dirty state** if that's the cause. Use - `CUDLY_FORCE_MIGRATION_VERSION` — see `internal/database/postgres/migrations/migrate.go` - for the full operator flow. Short version: - - If migration N's SQL landed on-disk (check the schema), set - `CUDLY_FORCE_MIGRATION_VERSION=N`. Next cold start marks clean and - resumes from N+1. - - If it didn't land, set `CUDLY_FORCE_MIGRATION_VERSION=N-1` to - retry N. - - Remove the env var after the deploy reports `healthy`. +3. **Clear dirty state** if that's the cause. Two options: + + a. **Auto-heal (default-on; usually nothing to do).** On every cold start, + if `schema_migrations` is dirty, the app `Force()`s the CURRENT recorded + version to clear the flag and re-applies any pending migrations, so the + next boot self-recovers without operator action. This is safe because + every migration in this repo is idempotent (guarded with `IF EXISTS` / + `IF NOT EXISTS` / `DO`-blocks); the `TestMigrations_FullStackIdempotent` + integration test enforces that invariant. It is **enabled by default**; + set `CUDLY_MIGRATION_AUTOHEAL=false` only to disable it in an environment + whose migrations are not idempotent. If auto-heal still can't reach head + (a genuinely broken migration), the app fail-opens (it always starts) and + the migration-failed alarm fires — fall through to option (b) or fix the + migration file. Auto-heal always forces the current version, never lower: + forcing below already-applied seed migrations would re-run guards that + raise on a second run (e.g. `000059` "Purchaser already exists"). + + b. **Manual force.** Use `CUDLY_FORCE_MIGRATION_VERSION` — see + `internal/database/postgres/migrations/migrate.go` for the full + operator flow. Short version: + - If migration N's SQL landed on-disk (check the schema), set + `CUDLY_FORCE_MIGRATION_VERSION=N`. Next cold start marks clean and + resumes from N+1. + - If it didn't land, set `CUDLY_FORCE_MIGRATION_VERSION=N-1` to + retry N. + - Remove the env var after the deploy reports `healthy`. + + `CUDLY_FORCE_MIGRATION_VERSION` takes precedence over auto-heal: it runs + first and pins+cleans the version, leaving nothing for auto-heal to act on. 4. **Verify recovery.** `/health` should return `migrations.status = "healthy"` after the next cold start completes successfully. ## Configuration -- `CUDLY_MIGRATION_TIMEOUT` (default `20s`) — parsed by +- `CUDLY_MIGRATION_TIMEOUT` (default `120s`) — parsed by `time.ParseDuration`. Invalid values fall back to the default with a - log warning. Shorter than the default Lambda 30s timeout so a runaway - migration gets cancelled cleanly inside `ensureDB` (preventing the - exact dirty-flag scenario) rather than by Lambda mid-invocation. + log warning. Set well above a normal index build / DDL (the prior 20s + could be blown mid-run by a single index build on a growing table, + leaving `schema_migrations.dirty = true`) yet comfortably under the + Lambda 300s hard limit, so a slow-but-legitimate migration completes + rather than being killed inside `ensureDB`. + +- `CUDLY_MIGRATION_AUTOHEAL` (**default-on**) — dirty auto-heal. When + `schema_migrations` is dirty on cold start, the app `Force()`s the CURRENT + recorded version to clear the dirty flag, then re-applies pending + migrations via `m.Up()`, so the next boot self-recovers. Relies on + migrations being idempotent (see `TestMigrations_FullStackIdempotent`). + Set to a `strconv.ParseBool` falsey value (`false`/`0`/...) to disable it + where idempotency is not guaranteed — a dirty DB then surfaces the usual + error instead of being auto-forced (the app still starts; it fail-opens). + Unset / empty / unparseable values keep it enabled. - `CUDLY_FORCE_MIGRATION_VERSION` (unset by default) — one-shot operator recovery. When set to a non-negative integer, calls `migrate.Force(N)` before `m.Up()`. Rejected with a loud error on non-numeric input — typos should surface immediately, not corrupt - state. + state. Takes precedence over `CUDLY_MIGRATION_AUTOHEAL`. - `DB_AUTO_MIGRATE` (default `false`, set to `true` in deployments that want lazy-init migrations) — when off, the `/health` migrations check @@ -105,10 +145,11 @@ category of pages: - Moving migrations to a dedicated CI deploy step (the longer-term fix — migrations run once per deploy with clear failure visibility, - not lazily per cold-start). Separate plan. -- Per-migration retry / auto-heal logic — if a migration fails, an - operator is still the right recovery mechanism. This feature just - stops the failure from nuking the user-facing app. + not lazily per cold-start). Separate plan / follow-up issue. +- Per-migration retry logic — auto-heal (above) clears a dirty flag and + re-applies idempotent migrations, but it does not retry a migration + whose SQL itself is broken; that still requires fixing the migration + file and redeploying. ## Related diff --git a/terraform/modules/compute/aws/lambda/migration-alarm.tf b/terraform/modules/compute/aws/lambda/migration-alarm.tf new file mode 100644 index 000000000..00d81ce3d --- /dev/null +++ b/terraform/modules/compute/aws/lambda/migration-alarm.tf @@ -0,0 +1,55 @@ +# ============================================== +# Migration-failure alarm +# ============================================== +# +# A dirty/failed schema migration is non-fatal at runtime (the app fail-opens +# and keeps serving requests that don't need the unapplied columns -- see +# internal/server/app.go ensureDB and specs/migration-resilience.md). That +# means a broken migration is INVISIBLE to the AWS/Lambda Errors metric: the +# function returns 200s while handlers needing the new schema 500 at query +# time. This metric filter + alarm makes that failure mode observable. +# +# The filter matches the literal the app logs on a failed migration attempt +# (internal/server/app.go: "Migration failed - app continuing with existing +# schema"). The quoted substring pattern matches the message regardless of the +# leading emoji / surrounding text, so a copy tweak to the log line that keeps +# the "Migration failed" phrase keeps the alarm working. +# +# Notification target is optional (var.alarm_sns_topic_arn). When no topic is +# supplied the alarm still exists and transitions to ALARM state -- visible in +# the console and queryable -- it just has no notification action. This mirrors +# the module's other optional, count/conditional-gated wiring and avoids +# inventing new SNS notification infrastructure here. + +resource "aws_cloudwatch_log_metric_filter" "migration_failed" { + name = "${var.stack_name}-migration-failed" + log_group_name = aws_cloudwatch_log_group.lambda.name + + # Quoted term = substring match anywhere in the log event. Matches the + # app.go log line "⚠️ Migration failed — app continuing with existing schema". + pattern = "\"Migration failed\"" + + metric_transformation { + name = "MigrationFailed" + namespace = "CUDly" + value = "1" + default_value = "0" + } +} + +resource "aws_cloudwatch_metric_alarm" "migration_failed" { + alarm_name = "${var.stack_name}-migration-failed" + comparison_operator = "GreaterThanThreshold" + evaluation_periods = 1 + metric_name = aws_cloudwatch_log_metric_filter.migration_failed.metric_transformation[0].name + namespace = aws_cloudwatch_log_metric_filter.migration_failed.metric_transformation[0].namespace + period = 300 + statistic = "Sum" + threshold = 0 + alarm_description = "A database migration failed on Lambda cold start (schema_migrations dirty or migration error/timeout). The app fail-opens, so AWS/Lambda Errors stays clean while schema-dependent handlers 500. See specs/migration-resilience.md for recovery." + alarm_actions = var.alarm_sns_topic_arn + ok_actions = var.alarm_sns_topic_arn + treat_missing_data = "notBreaching" + + tags = var.tags +} diff --git a/terraform/modules/compute/aws/lambda/outputs.tf b/terraform/modules/compute/aws/lambda/outputs.tf index 94e7ff63d..badbafbdf 100644 --- a/terraform/modules/compute/aws/lambda/outputs.tf +++ b/terraform/modules/compute/aws/lambda/outputs.tf @@ -37,3 +37,13 @@ output "signing_key_id" { description = "KMS asymmetric key ID used by the CUDly OIDC issuer" value = aws_kms_key.signing.key_id } + +output "migration_failed_alarm_arn" { + description = "ARN of the CloudWatch alarm that fires when a database migration fails on cold start" + value = aws_cloudwatch_metric_alarm.migration_failed.arn +} + +output "migration_failed_metric_filter_name" { + description = "Name of the log metric filter counting migration-failure log lines" + value = aws_cloudwatch_log_metric_filter.migration_failed.name +} diff --git a/terraform/modules/compute/aws/lambda/variables.tf b/terraform/modules/compute/aws/lambda/variables.tf index e55c8508e..800012c11 100644 --- a/terraform/modules/compute/aws/lambda/variables.tf +++ b/terraform/modules/compute/aws/lambda/variables.tf @@ -243,6 +243,12 @@ variable "email_from_domain" { default = "" } +variable "alarm_sns_topic_arn" { + description = "Optional list of SNS topic ARNs to notify on the migration-failure alarm (alarm_actions / ok_actions). Leave empty ([]) to create the alarm without a notification target -- it still transitions to ALARM state and is visible/queryable in CloudWatch. Pass the monitoring module's SNS topic ARN here to wire notifications; this module intentionally does not create its own SNS topic." + type = list(string) + default = [] +} + variable "tags" { description = "Tags to apply to all resources" type = map(string)