fix(controller): skip DB migration when DB schema is ahead of binary

onematchfox · claude · onematchfox · commit aa49e3efaa75 · 2026-06-05T12:10:59.000+02:00
When an older binary starts against a database that a newer binary has already migrated, calling mg.Up() fails because golang-migrate tries to read the down file for the current DB version, which does not exist in the older binary's embedded FS. This caused a crash loop (issue #1881). Instead, detect when the database version exceeds the binary's highest known migration and return early. This relies on the expand-then-contract policy documented in database-migrations.md, which guarantees each release's code is compatible with the schema applied by the previous release. Adds TestApplyDir_SucceedsWhenDBVersionAhead to cover this path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
diff --git a/go/core/pkg/migrations/runner.go b/go/core/pkg/migrations/runner.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"io/fs"
+	"strings"
 
 	"github.com/golang-migrate/migrate/v4"
 	migratepgx "github.com/golang-migrate/migrate/v4/database/pgx/v5"
@@ -56,12 +57,37 @@ func applyDir(url string, migrationsFS fs.FS, dir, migrationsTable string) (prev
 	}
 	defer closeMigrate(dir, mg)
 
-	prevVersion, _, err = mg.Version()
+	var dirty bool
+	prevVersion, dirty, err = mg.Version()
 	if err != nil && !errors.Is(err, migrate.ErrNilVersion) {
 		return 0, fmt.Errorf("get pre-migration version for %s: %w", dir, err)
 	}
 	// prevVersion == 0 when ErrNilVersion (no migrations applied yet).
 
+	// If the database is ahead of this binary's max known version, skip Up
+	// entirely. The expand-then-contract policy in database-migrations.md
+	// guarantees that each release's code is compatible with the schema applied
+	// by the previous release, so rolling back one release at a time is safe.
+	// We cannot enforce a tighter constraint here because migration version
+	// numbers don't align with release versions.
+	// A dirty database is excluded: dirty state means a previous migration
+	// attempt failed and must be resolved, not silently accepted.
+	if maxVer, scanErr := maxEmbeddedVersion(migrationsFS, dir); scanErr != nil {
+		log.Error(scanErr, "could not determine max embedded migration version; proceeding with Up", "track", dir)
+	} else if prevVersion > maxVer {
+		if dirty {
+			// DB is both dirty and ahead of this binary. Attempting Up/rollback would
+			// fail (the migration files for prevVersion don't exist), producing noisy
+			// and misleading logs. Return a clear error so operators act on the real
+			// problem rather than chasing rollback noise.
+			return prevVersion, fmt.Errorf("database is dirty at version %d and ahead of this binary's max known version %d for track %s: manual operator intervention required: %w",
+				prevVersion, maxVer, dir, migrate.ErrDirty{Version: int(prevVersion)})
+		}
+		log.Info("database schema is ahead of this binary; running in compatibility mode",
+			"track", dir, "dbVersion", prevVersion, "binaryMax", maxVer)
+		return prevVersion, nil
+	}
+
 	if upErr := mg.Up(); upErr != nil {
 		if errors.Is(upErr, migrate.ErrNoChange) {
 			return prevVersion, nil
@@ -186,6 +212,41 @@ func newMigrate(url string, migrationsFS fs.FS, dir, migrationsTable string) (*m
 	return mg, nil
 }
 
+// maxEmbeddedVersion scans dir inside migrationsFS and returns the highest migration
+// version number found. Only files with a ".up.sql" suffix are considered. Version
+// numbers are parsed from the leading decimal digits of each filename; the remainder
+// of the name is not validated. Returns an error if the directory cannot be read or
+// contains no recognisable migration files.
+func maxEmbeddedVersion(migrationsFS fs.FS, dir string) (uint, error) {
+	entries, err := fs.ReadDir(migrationsFS, dir)
+	if err != nil {
+		return 0, fmt.Errorf("read migration dir %s: %w", dir, err)
+	}
+	var highest uint
+	var foundUpSQL, foundVersioned bool
+	for _, e := range entries {
+		if e.IsDir() || !strings.HasSuffix(e.Name(), ".up.sql") {
+			continue
+		}
+		foundUpSQL = true
+		var v uint
+		if _, scanErr := fmt.Sscanf(e.Name(), "%d", &v); scanErr != nil {
+			continue
+		}
+		foundVersioned = true
+		if v > highest {
+			highest = v
+		}
+	}
+	if !foundUpSQL {
+		return 0, fmt.Errorf("no .up.sql migration files found in %s", dir)
+	}
+	if !foundVersioned {
+		return 0, fmt.Errorf("no versioned .up.sql migration files found in %s; expected names like 000001_description.up.sql", dir)
+	}
+	return highest, nil
+}
+
 // closeMigrate closes mg, logging source and database close errors separately.
 func closeMigrate(dir string, mg *migrate.Migrate) {
 	srcErr, dbErr := mg.Close()
diff --git a/go/core/pkg/migrations/runner_test.go b/go/core/pkg/migrations/runner_test.go
@@ -3,12 +3,14 @@ package migrations
 import (
 	"context"
 	"database/sql"
+	"errors"
 	"fmt"
 	"maps"
 	"testing"
 	"testing/fstest"
 	"time"
 
+	"github.com/golang-migrate/migrate/v4"
 	_ "github.com/jackc/pgx/v5/stdlib"
 	testcontainers "github.com/testcontainers/testcontainers-go"
 	tcpostgres "github.com/testcontainers/testcontainers-go/modules/postgres"
@@ -285,6 +287,133 @@ func TestApplyDir_RollsBackWithExistingVersion(t *testing.T) {
 	}
 }
 
+func TestMaxEmbeddedVersion(t *testing.T) {
+	tests := []struct {
+		name    string
+		fs      fstest.MapFS
+		dir     string
+		want    uint
+		wantErr bool
+	}{
+		{
+			name: "returns highest version from up.sql files",
+			fs: fstest.MapFS{
+				"core/000001_a.up.sql":   {},
+				"core/000001_a.down.sql": {},
+				"core/000003_b.up.sql":   {},
+				"core/000003_b.down.sql": {},
+			},
+			dir:  "core",
+			want: 3,
+		},
+		{
+			name: "ignores non-sql files",
+			fs: fstest.MapFS{
+				"core/README.md":         {},
+				"core/000002_x.up.sql":   {},
+				"core/000002_x.down.sql": {},
+			},
+			dir:  "core",
+			want: 2,
+		},
+		{
+			name: "ignores down.sql when computing max",
+			fs: fstest.MapFS{
+				"core/000001_a.up.sql":   {},
+				"core/000002_b.down.sql": {},
+			},
+			dir:  "core",
+			want: 1,
+		},
+		{
+			name:    "up.sql files with unparseable names returns error",
+			fs:      fstest.MapFS{"core/init.up.sql": {}},
+			dir:     "core",
+			wantErr: true,
+		},
+		{
+			name:    "empty dir returns error",
+			fs:      fstest.MapFS{"core/.keep": {}},
+			dir:     "core",
+			wantErr: true,
+		},
+		{
+			name:    "nonexistent dir returns error",
+			fs:      fstest.MapFS{},
+			dir:     "missing",
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := maxEmbeddedVersion(tt.fs, tt.dir)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("maxEmbeddedVersion() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			if !tt.wantErr && got != tt.want {
+				t.Errorf("maxEmbeddedVersion() = %d, want %d", got, tt.want)
+			}
+		})
+	}
+}
+
+// TestApplyDir_SucceedsWhenDBVersionAhead verifies that an older binary starting against
+// a database that a newer binary has migrated does not crash-loop. It skips Up entirely
+// and returns success, leaving the schema unchanged. Safe rollback relies on the
+// expand-then-contract discipline in database-migrations.md and rolling back one release
+// at a time.
+func TestApplyDir_SucceedsWhenDBVersionAhead(t *testing.T) {
+	connStr := startTestDB(t)
+
+	// Newer binary applies v1 and v2.
+	if _, err := applyDir(connStr, goodCoreFS, "core", "schema_migrations"); err != nil {
+		t.Fatalf("newer binary apply: %v", err)
+	}
+
+	// Older binary (max v1) starts against the v2 schema — must not error.
+	if _, err := applyDir(connStr, oneCoreFS, "core", "schema_migrations"); err != nil {
+		t.Fatalf("older binary apply against newer schema: %v", err)
+	}
+
+	// Schema version must be unchanged — the older binary has no business rolling back.
+	if got := trackVersion(t, connStr, "schema_migrations"); got != 2 {
+		t.Errorf("version = %d, want 2 (older binary must not modify schema version)", got)
+	}
+}
+
+// TestApplyDir_DirtyStateNotMaskedByCompatibilityMode verifies that a dirty database
+// is not silently accepted by compatibility mode. If the DB is both dirty and ahead of
+// the binary's max known version, the dirty state must still be surfaced as an error.
+func TestApplyDir_DirtyStateNotMaskedByCompatibilityMode(t *testing.T) {
+	connStr := startTestDB(t)
+
+	// Apply v1 cleanly first so the tracking table exists.
+	if _, err := applyDir(connStr, oneCoreFS, "core", "schema_migrations"); err != nil {
+		t.Fatalf("setup: %v", err)
+	}
+
+	// Simulate a newer binary having applied v2 but leaving it dirty.
+	db, err := sql.Open("pgx", connStr)
+	if err != nil {
+		t.Fatalf("open db: %v", err)
+	}
+	defer db.Close()
+	if _, err := db.Exec("UPDATE schema_migrations SET version = 2, dirty = true"); err != nil {
+		t.Fatalf("set dirty state: %v", err)
+	}
+
+	// Older binary (max v1) starts: DB is at v2 dirty. Compatibility mode must NOT
+	// trigger — dirty state must be returned as an error so the operator can act.
+	_, err = applyDir(connStr, oneCoreFS, "core", "schema_migrations")
+	if err == nil {
+		t.Fatal("expected error for dirty database, got nil")
+	}
+	var dirtyErr migrate.ErrDirty
+	if !errors.As(err, &dirtyErr) {
+		t.Errorf("expected migrate.ErrDirty, got %T: %v", err, err)
+	}
+}
+
 // --- rollbackDir tests ---
 
 func TestRollbackDir_RollsBackToTarget(t *testing.T) {
