From ad3f216b3c4a1d693d107ddc606f5a56dd84ceec Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 20 May 2026 02:07:20 +0200 Subject: [PATCH 1/4] fix(db): add regression test for migration 000027 idempotency Migration 000027 (savings_snapshots_pk) already contains the DROP CONSTRAINT IF EXISTS guard that makes it safe on fresh databases where 000018 has already created savings_snapshots_pkey. This commit adds the integration test that would have caught the original failure: - TestMigration_SavingsSnapshotsPK/full_migration_from_scratch: runs all migrations from scratch, verifying 000027 does not fail with "multiple primary keys for table" even though 000018 already added the constraint. - TestMigration_SavingsSnapshotsPK/rollback_to_000026_then_re-apply: rolls back past 000027 (without rolling back 000018) and re-applies all migrations, verifying idempotency in the replay scenario. Closes #246 --- .../migrations/savings_snapshots_pk_test.go | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 internal/database/postgres/migrations/savings_snapshots_pk_test.go diff --git a/internal/database/postgres/migrations/savings_snapshots_pk_test.go b/internal/database/postgres/migrations/savings_snapshots_pk_test.go new file mode 100644 index 000000000..d5fbe0875 --- /dev/null +++ b/internal/database/postgres/migrations/savings_snapshots_pk_test.go @@ -0,0 +1,105 @@ +//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" +) + +// TestMigration_SavingsSnapshotsPK verifies that migration 000027 is +// idempotent on a fresh database. +// +// On a fresh DB, migration 000018 already adds savings_snapshots_pkey. Before +// the fix, 000027's bare ADD CONSTRAINT would fail with "multiple primary +// keys for table savings_snapshots". After the fix, the DROP CONSTRAINT IF +// EXISTS guard makes 000027 safe to apply regardless of whether 000018 has +// already created the constraint. +func TestMigration_SavingsSnapshotsPK(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + t.Run("full migration from scratch succeeds (000018 + 000027 coexist)", func(t *testing.T) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Running all migrations must succeed on a fresh DB โ€” 000027's + // DROP CONSTRAINT IF EXISTS prevents "multiple primary keys" + // that the pre-fix ADD CONSTRAINT caused. + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", ""), + "full migration from scratch should succeed with idempotent 000027") + + // The primary key must exist and cover (id, timestamp). + var constraintExists bool + err = pool.QueryRow(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = 'savings_snapshots_pkey' + AND conrelid = 'savings_snapshots'::regclass + AND contype = 'p' + )`).Scan(&constraintExists) + require.NoError(t, err) + assert.True(t, constraintExists, "savings_snapshots_pkey should exist after migrations") + }) + + t.Run("rollback to 000026 then re-apply 000027 succeeds", 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, "", "")) + + // Determine how many migrations are above 000027 by counting + // migrations numbered higher, then rolling back that many plus one. + // Simpler: roll back until 000027 is gone, i.e. roll back enough + // steps that version 000027 is undone. + // + // We use a raw query to find the current version, roll back + // (current - 26) steps to land at 000026, then run Up again. + var currentVersion int + err = pool.QueryRow(ctx, `SELECT version FROM schema_migrations ORDER BY version DESC LIMIT 1`).Scan(¤tVersion) + require.NoError(t, err) + + stepsToRollback := currentVersion - 26 + require.Greater(t, stepsToRollback, 0, "there should be migrations above 000026") + + require.NoError(t, migrations.RollbackMigrations(ctx, pool, migrationsPath, stepsToRollback), + "rollback to 000026 should succeed") + + // Verify the constraint is gone (000027 was rolled back). + var constraintExists bool + err = pool.QueryRow(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = 'savings_snapshots_pkey' + AND conrelid = 'savings_snapshots'::regclass + AND contype = 'p' + )`).Scan(&constraintExists) + require.NoError(t, err) + assert.False(t, constraintExists, "savings_snapshots_pkey should be gone after rollback past 000027") + + // Re-apply all migrations โ€” 000027 must succeed even though + // 000018 is still in effect (it was not rolled back). + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", ""), + "re-applying 000027 over a DB where 000018 already ran must succeed") + + err = pool.QueryRow(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = 'savings_snapshots_pkey' + AND conrelid = 'savings_snapshots'::regclass + AND contype = 'p' + )`).Scan(&constraintExists) + require.NoError(t, err) + assert.True(t, constraintExists, "savings_snapshots_pkey should be restored after re-apply") + }) +} From 958d51ad18cc890344a065c8a65c8a908c5acec3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 20 May 2026 10:06:11 +0200 Subject: [PATCH 2/4] fix(db): fix 000040 down SQL column scope and test rollback loop The ORDER BY CASE k in 000040_split_savingsplans.down.sql referenced column k from a UNION branch, which is out of scope at the outer ORDER BY level in PostgreSQL. Restructure the SP-collapse branch to use a subquery that exposes a numeric priority column, then ORDER BY that column name inside the subquery. The savings_snapshots_pk_test.go rollback sub-test computed stepsToRollback = currentVersion - 26 (= 21 on a 47-migration DB) and passed it in a single RollbackMigrations call, which hit the 10-step safety cap. Call in a loop of at-most-10-step batches. --- .../000040_split_savingsplans.down.sql | 32 +++++++++++-------- .../migrations/savings_snapshots_pk_test.go | 12 +++++-- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/internal/database/postgres/migrations/000040_split_savingsplans.down.sql b/internal/database/postgres/migrations/000040_split_savingsplans.down.sql index 41149a2e8..b4742ff22 100644 --- a/internal/database/postgres/migrations/000040_split_savingsplans.down.sql +++ b/internal/database/postgres/migrations/000040_split_savingsplans.down.sql @@ -65,20 +65,24 @@ SET services = ( -- created post-split with only sagemaker/database/ec2instance -- still gets a usable umbrella key on rollback. SELECT 'aws:savings-plans' AS new_key, v AS new_val - FROM jsonb_each(services) AS e(k, v) - WHERE k IN ( - 'aws:savings-plans-compute', - 'aws:savings-plans-ec2instance', - 'aws:savings-plans-sagemaker', - 'aws:savings-plans-database' - ) - ORDER BY CASE k - WHEN 'aws:savings-plans-compute' THEN 1 - WHEN 'aws:savings-plans-ec2instance' THEN 2 - WHEN 'aws:savings-plans-sagemaker' THEN 3 - ELSE 4 - END - LIMIT 1 + FROM ( + SELECT v, + CASE k + WHEN 'aws:savings-plans-compute' THEN 1 + WHEN 'aws:savings-plans-ec2instance' THEN 2 + WHEN 'aws:savings-plans-sagemaker' THEN 3 + ELSE 4 + END AS priority + FROM jsonb_each(services) AS e(k, v) + WHERE k IN ( + 'aws:savings-plans-compute', + 'aws:savings-plans-ec2instance', + 'aws:savings-plans-sagemaker', + 'aws:savings-plans-database' + ) + ORDER BY priority + LIMIT 1 + ) sp_best ) merged ) WHERE services ?| ARRAY[ diff --git a/internal/database/postgres/migrations/savings_snapshots_pk_test.go b/internal/database/postgres/migrations/savings_snapshots_pk_test.go index d5fbe0875..3e969898d 100644 --- a/internal/database/postgres/migrations/savings_snapshots_pk_test.go +++ b/internal/database/postgres/migrations/savings_snapshots_pk_test.go @@ -72,8 +72,16 @@ func TestMigration_SavingsSnapshotsPK(t *testing.T) { stepsToRollback := currentVersion - 26 require.Greater(t, stepsToRollback, 0, "there should be migrations above 000026") - require.NoError(t, migrations.RollbackMigrations(ctx, pool, migrationsPath, stepsToRollback), - "rollback to 000026 should succeed") + // RollbackMigrations caps a single call at 10 steps; call in a loop. + const maxRollbackPerCall = 10 + for remaining := stepsToRollback; remaining > 0; remaining -= maxRollbackPerCall { + batch := remaining + if batch > maxRollbackPerCall { + batch = maxRollbackPerCall + } + require.NoError(t, migrations.RollbackMigrations(ctx, pool, migrationsPath, batch), + "rollback to 000026 should succeed") + } // Verify the constraint is gone (000027 was rolled back). var constraintExists bool From e44b7025c6e24fd9b9755891f65e18bbc1c0211c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 21 May 2026 14:56:41 +0200 Subject: [PATCH 3/4] refactor(migrations): extract getMigrationsPath to shared test helper Move getMigrationsPath() from split_savingsplans_test.go into a new helpers_test.go shared by all integration tests in the package. This removes the implicit cross-file dependency that CodeRabbit flagged in savings_snapshots_pk_test.go: the function was defined in a sibling file and invisible to a reader of the calling file alone. Both files are in the same package and build tag (integration), so they always compiled together. The refactor makes each test file self-evidently complete without relying on symbol spillover from a sibling. --- .../postgres/migrations/helpers_test.go | 17 +++++++++++++++++ .../migrations/split_savingsplans_test.go | 8 -------- 2 files changed, 17 insertions(+), 8 deletions(-) create mode 100644 internal/database/postgres/migrations/helpers_test.go diff --git a/internal/database/postgres/migrations/helpers_test.go b/internal/database/postgres/migrations/helpers_test.go new file mode 100644 index 000000000..9a76f93b0 --- /dev/null +++ b/internal/database/postgres/migrations/helpers_test.go @@ -0,0 +1,17 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "path/filepath" + "runtime" +) + +// getMigrationsPath resolves the migrations directory relative to this test +// file so that migration test files can locate the SQL files regardless of +// the working directory when tests are invoked. +func getMigrationsPath() string { + _, filename, _, _ := runtime.Caller(0) + return filepath.Dir(filename) +} diff --git a/internal/database/postgres/migrations/split_savingsplans_test.go b/internal/database/postgres/migrations/split_savingsplans_test.go index 2c6843624..81fb905bb 100644 --- a/internal/database/postgres/migrations/split_savingsplans_test.go +++ b/internal/database/postgres/migrations/split_savingsplans_test.go @@ -5,8 +5,6 @@ package migrations_test import ( "context" - "path/filepath" - "runtime" "testing" "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" @@ -16,12 +14,6 @@ import ( "github.com/stretchr/testify/require" ) -// getMigrationsPath resolves the migrations directory next to this test file. -func getMigrationsPath() string { - _, filename, _, _ := runtime.Caller(0) - return filepath.Dir(filename) -} - // TestMigration_SplitSavingsPlans drives the 000040 migration through // the three scenarios from the plan ยง7: umbrella-only, umbrella + PR // #71's sagemaker row, and fresh install. Round-trips up/down to verify From d9a4db69ba1f3c3292e32176c56be630a73d896d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 15:09:49 +0200 Subject: [PATCH 4/4] fix(db): derive rollback step count from schema_migrations row count Replace `currentVersion - 26` arithmetic with a direct COUNT query on schema_migrations for rows with version > 26. This is correct even when migration numbering has gaps, since Steps(-n) steps back through n applied migrations (not n version-number units). Also clarify the comment about migration 000018 being "still in effect" to make clear that its schema_migrations entry persists but its original constraint was superseded by 000027. Addresses CodeRabbit finding on PR #540 (nitpick, lines 68-73 and 90-93 of savings_snapshots_pk_test.go). --- .../migrations/savings_snapshots_pk_test.go | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/internal/database/postgres/migrations/savings_snapshots_pk_test.go b/internal/database/postgres/migrations/savings_snapshots_pk_test.go index 3e969898d..128c86332 100644 --- a/internal/database/postgres/migrations/savings_snapshots_pk_test.go +++ b/internal/database/postgres/migrations/savings_snapshots_pk_test.go @@ -58,18 +58,14 @@ func TestMigration_SavingsSnapshotsPK(t *testing.T) { require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) - // Determine how many migrations are above 000027 by counting - // migrations numbered higher, then rolling back that many plus one. - // Simpler: roll back until 000027 is gone, i.e. roll back enough - // steps that version 000027 is undone. - // - // We use a raw query to find the current version, roll back - // (current - 26) steps to land at 000026, then run Up again. - var currentVersion int - err = pool.QueryRow(ctx, `SELECT version FROM schema_migrations ORDER BY version DESC LIMIT 1`).Scan(¤tVersion) + // Count how many applied migrations are above version 26. + // Using COUNT from schema_migrations is correct even when migration + // numbering has gaps: Steps(-n) steps back through n applied + // migrations, so we need the actual row count, not an arithmetic + // difference between version numbers. + var stepsToRollback int + err = pool.QueryRow(ctx, `SELECT COUNT(*) FROM schema_migrations WHERE version > 26`).Scan(&stepsToRollback) require.NoError(t, err) - - stepsToRollback := currentVersion - 26 require.Greater(t, stepsToRollback, 0, "there should be migrations above 000026") // RollbackMigrations caps a single call at 10 steps; call in a loop. @@ -96,7 +92,9 @@ func TestMigration_SavingsSnapshotsPK(t *testing.T) { assert.False(t, constraintExists, "savings_snapshots_pkey should be gone after rollback past 000027") // Re-apply all migrations โ€” 000027 must succeed even though - // 000018 is still in effect (it was not rolled back). + // 000018's schema_migrations entry is still present (that migration + // was not rolled back, though its original constraint was superseded + // when 000027 ran and dropped/re-created savings_snapshots_pkey). require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", ""), "re-applying 000027 over a DB where 000018 already ran must succeed")