From f6eff9114469aef53ebdb7a6cde01f08eb551b83 Mon Sep 17 00:00:00 2001 From: Martin Yankovs Date: Wed, 3 Jun 2026 13:40:09 +0300 Subject: [PATCH] fix(migrations): order migration files by numeric id, not lexically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runner derived each migration id from the numeric filename prefix and gated application on a numeric high-water mark, but chose run order with sort.Strings — a lexical sort. The two agree only while every id is the same width. The first 3-digit migration ("100__") sorts before "10__".."99__" lexically, so on a fresh database the runner applies 100/101 first, advances the numeric mark past 99, then silently skips migrations 10..99. Incrementally migrated databases (mark already >= 99) are unaffected, so this only bites fresh environments: new dev setups, CI, ephemeral test databases. Sort by the parsed numeric id — the same value the apply gate uses — with a lexical tie-break, so run order and gate stay consistent regardless of digit count and zero-padding is no longer load-bearing. Extract migrationID as the shared parse helper used by both paths. Fixes #197 Co-Authored-By: Claude Opus 4.8 --- storage/migration.go | 48 ++++++++++++++++++++++++++-- storage/migration_internal_test.go | 51 ++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/storage/migration.go b/storage/migration.go index 524c4b1..b970cae 100644 --- a/storage/migration.go +++ b/storage/migration.go @@ -5,7 +5,6 @@ import ( "log/slog" "path/filepath" "slices" - "sort" "strconv" "strings" @@ -73,6 +72,49 @@ func (m *DatabaseMigration) rollbackMigration(migration MigrationFile) error { return err } +// migrationID extracts the numeric id from a migration filename of the form +// "__.yaml". It is the single source of truth for both +// ordering migrations (sortMigrationKeys) and gating them against the applied +// high-water mark (runMigrations), so the two can never disagree. +// +// The "__" separator is required: descriptions are snake_case, so a single "_" +// is a word boundary, not an id boundary. A missing separator is a malformed +// filename and is surfaced as an error rather than silently misparsed. +func migrationID(key string) (int, error) { + prefix, _, ok := strings.Cut(key, "__") + if !ok { + return 0, fmt.Errorf("migration %q is missing the %q id separator", key, "__") + } + return strconv.Atoi(prefix) +} + +// sortMigrationKeys orders migration filenames by their parsed numeric id +// rather than lexically. Filenames are not fixed-width, so a 3-digit id like +// "100__" sorts before "10__".."99__" under a plain string sort, while the +// apply gate compares ids numerically. That mismatch makes the runner apply +// 100/101 first on a fresh database, advance the numeric high-water mark past +// 99, then silently skip every migration 10..99 (their ids fall below the +// mark). Sorting by the same id the gate uses keeps run order and gate +// consistent regardless of digit count. Ties — and keys whose prefix does not +// parse — fall back to a lexical compare so ordering stays deterministic; an +// unparseable id is surfaced by runMigrations when it reaches that key. +func sortMigrationKeys(keys []string) { + slices.SortStableFunc(keys, func(a, b string) int { + idA, errA := migrationID(a) + idB, errB := migrationID(b) + if errA != nil || errB != nil { + return strings.Compare(a, b) + } + if idA != idB { + if idA < idB { + return -1 + } + return 1 + } + return strings.Compare(a, b) + }) +} + func (m *DatabaseMigration) runMigrations(migrations map[string]MigrationFile) { slog.Info("Getting last migration applied") rollback := false @@ -86,10 +128,10 @@ func (m *DatabaseMigration) runMigrations(migrations map[string]MigrationFile) { for k := range migrations { keys = append(keys, k) } - sort.Strings(keys) + sortMigrationKeys(keys) for _, k := range keys { - migrationId, err := strconv.Atoi(strings.Split(k, "__")[0]) + migrationId, err := migrationID(k) if err != nil { logger.Fatal("failed to determine migration id", slog.Any("error", err)) } diff --git a/storage/migration_internal_test.go b/storage/migration_internal_test.go index 5c07a12..eea5116 100644 --- a/storage/migration_internal_test.go +++ b/storage/migration_internal_test.go @@ -2,6 +2,7 @@ package storage import ( "errors" + "slices" "testing" ) @@ -75,6 +76,56 @@ func TestRollbackMigrationStopsAtFirstFailure(t *testing.T) { } } +func TestSortMigrationKeysOrdersNumericallyNotLexically(t *testing.T) { + // The unsorted set deliberately mixes one-, two-, and three-digit ids, + // and includes two keys that share id 100 to exercise the tie-break. + // A lexical sort would place "100__" before "10__".."99__"; the numeric + // sort must not. + keys := []string{ + "100__add_widgets", "2__add_users", "10__add_orders", + "9__add_carts", "100__add_audit", "1__init", "99__add_index", + } + sortMigrationKeys(keys) + + want := []string{ + "1__init", "2__add_users", "9__add_carts", "10__add_orders", + "99__add_index", "100__add_audit", "100__add_widgets", + } + if len(keys) != len(want) { + t.Fatalf("len = %d; want %d", len(keys), len(want)) + } + for i, got := range keys { + if got != want[i] { + t.Fatalf("keys[%d] = %q; want %q (full order: %v)", i, got, want[i], keys) + } + } +} + +func TestSortMigrationKeysFallsBackToLexicalForUnparseablePrefix(t *testing.T) { + // Keys whose prefix is not a number must still sort deterministically + // rather than panic; runMigrations reports the bad id when it gets there. + keys := []string{"10__ok", "bad__name", "2__ok", "another"} + sortMigrationKeys(keys) + + // Numeric-prefixed keys keep numeric order among themselves; the rest are + // ordered lexically relative to whatever they compare against. + if got := slices.Index(keys, "2__ok"); got > slices.Index(keys, "10__ok") { + t.Fatalf("2__ok should precede 10__ok, got order %v", keys) + } +} + +func TestMigrationIDRequiresDoubleUnderscoreSeparator(t *testing.T) { + // A single underscore is a snake_case word boundary inside the + // description, not an id boundary, so a name lacking "__" is malformed + // and must error loudly rather than misparse its prefix as an id. + if _, err := migrationID("1_init.yaml"); err == nil { + t.Fatal("expected error for filename without __ separator") + } + if got, err := migrationID("12__add_users.yaml"); err != nil || got != 12 { + t.Fatalf("migrationID = %d, %v; want 12, nil", got, err) + } +} + func TestGetMigrationFilesReturnsEmptyForUninitializedConfigFs(t *testing.T) { // ConfigFs is a zero-value embed.FS unless the embedding // application has wired its own in, so ReadDir fails