From 0640594f3bcb1cdf6a1e010bf129e909f9b7298b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:33:32 +0200 Subject: [PATCH 1/3] fix(db): widen purchase_history.account_id to VARCHAR(255) purchase_history.account_id has been VARCHAR(20) since 000001, which fits an AWS account ID (12 digits) and nothing else. An Azure subscription ID is a 36-character GUID and a GCP project ID runs to 30 characters, so SavePurchaseHistory's INSERT is rejected with SQLSTATE 22001 after the commitment has already been purchased and billed. The error is not swallowed: savePurchaseHistory returns it and the caller stamps a history_write_failed audit-gap marker on the execution (#621). But the purchase_history row itself is lost, so the billed commitment is invisible in the History view, absent from GetActivePurchaseHistory (and therefore undercounted in analytics), and unseen by the grace-period and suppression logic. The identical widening was already applied to the sibling column: 000067 widened savings_snapshots.account_id for exactly this reason and 000074 repaired it on partially-migrated databases. purchase_history was missed. - up: probe-guarded ALTER so it is correct on a fresh database, an already-deployed one, and on re-run under auto-heal. Resolves the table via 'purchase_history'::regclass so the probe follows search_path exactly as the ALTER does, and re-reads the catalog afterwards so the migration cannot be recorded as applied without having widened the column. - down: narrows only from VARCHAR(255), and refuses with a named error rather than truncating when any account_id exceeds 20 characters. Documents the CUDLY_FORCE_MIGRATION_VERSION=95 recovery for the dirty state a refusal leaves behind. Rows already lost to 22001 were never inserted and cannot be recovered by this migration; affected executions are identifiable via the history_write_failed marker on purchase_executions.error. Regression test replicates the real failing scenario with SavePurchaseHistory's own INSERT column set and a 36-char Azure GUID / 30-char GCP project ID: both are rejected with 22001 at version 92 and round-trip untruncated after 000095. Also covers the lossless rollback path and up-migration idempotency. Closes #1603 --- ...purchase_history_account_id_width.down.sql | 81 ++++++ ...5_purchase_history_account_id_width.up.sql | 106 ++++++++ ..._purchase_history_account_id_width_test.go | 242 ++++++++++++++++++ 3 files changed, 429 insertions(+) create mode 100644 internal/database/postgres/migrations/000095_purchase_history_account_id_width.down.sql create mode 100644 internal/database/postgres/migrations/000095_purchase_history_account_id_width.up.sql create mode 100644 internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go diff --git a/internal/database/postgres/migrations/000095_purchase_history_account_id_width.down.sql b/internal/database/postgres/migrations/000095_purchase_history_account_id_width.down.sql new file mode 100644 index 000000000..81e877148 --- /dev/null +++ b/internal/database/postgres/migrations/000095_purchase_history_account_id_width.down.sql @@ -0,0 +1,81 @@ +-- Reverse 000095: narrow purchase_history.account_id back to VARCHAR(20). +-- +-- THIS ROLLBACK IS LOSSY BY NATURE and deliberately FAILS LOUDLY rather than +-- truncating. Any row written after the up migration applied may carry an +-- Azure subscription GUID (36 chars) or a long GCP project ID; narrowing the +-- column would either error mid-rewrite or, worse, be "fixed" by a +-- well-meaning operator with a truncating USING clause that silently corrupts +-- the audit trail this table exists to provide. +-- +-- So: refuse the rollback while any over-long value is present, naming the +-- count, and let the operator decide. There is no safe automatic remedy -- +-- unlike 000063's down migration, which could coerce NULL monthly_cost to 0.0 +-- losslessly, there is no lossless 20-character encoding of a 36-character +-- subscription ID. +-- +-- 000067's down migration made the same call for savings_snapshots.account_id +-- and simply left the column at VARCHAR(255). This one goes further and does +-- narrow, but only when it is provably safe. +-- +-- ------------------------------------------------------------------ +-- Reverse exactly what the up migration did, and nothing more +-- ------------------------------------------------------------------ +-- The up lands the column on exactly VARCHAR(255) (atttypmod 259) and leaves +-- anything already >= 255, or unbounded `varchar` (atttypmod -1), untouched. +-- So this narrows ONLY from 259. Guarding on "anything wider than 20" would +-- narrow a VARCHAR(300) or an unbounded varchar down to VARCHAR(20) -- a +-- state 000095 never created, and a rollback must restore the pre-up state +-- rather than impose a new one. +-- +-- Unlike the up, this direction is NOT binary-coercible: narrowing a varchar +-- forces a full table rewrite and rebuilds every dependent index. On a large +-- purchase_history that is a long ACCESS EXCLUSIVE lock, not the near-instant +-- catalog-only change the up migration is. +-- +-- ------------------------------------------------------------------ +-- Recovering from a refused rollback +-- ------------------------------------------------------------------ +-- golang-migrate stamps (version=95, dirty=true) BEFORE running this file, so +-- a refusal leaves schema_migrations dirty at 95 and maybeAutoHealDirty +-- (migrate.go) deliberately will NOT clear it -- the next deploy hard-fails +-- until an operator intervenes. Because this file raises before touching the +-- schema, the database still matches version 95 exactly. Recover by setting +-- CUDLY_FORCE_MIGRATION_VERSION=95 (the CURRENT version, never lower: the +-- 000095 up migration's effects ARE present) and redeploying, then removing +-- the env var. Only then archive or reconcile the over-long rows if the +-- rollback is still wanted. + +DO $$ +DECLARE + typmod INTEGER; + over_long BIGINT; +BEGIN + SELECT atttypmod INTO typmod + FROM pg_attribute + WHERE attrelid = 'purchase_history'::regclass + AND attname = 'account_id' + AND NOT attisdropped; + + -- 259 == VARCHAR(255) (atttypmod is n + 4). Anything else was not + -- produced by 000095's up migration, so leave it alone. + IF typmod IS DISTINCT FROM 259 THEN + RETURN; + END IF; + + SELECT COUNT(*) INTO over_long + FROM purchase_history + WHERE CHAR_LENGTH(account_id) > 20; + + IF over_long > 0 THEN + RAISE EXCEPTION + 'refusing to narrow purchase_history.account_id to VARCHAR(20): ' + '% row(s) hold an account ID longer than 20 characters ' + '(Azure subscription GUIDs are 36 chars, GCP project IDs up to 30). ' + 'Narrowing would truncate the audit trail. Archive or reconcile ' + 'those rows before rolling back migration 000095.', + over_long; + END IF; + + ALTER TABLE purchase_history + ALTER COLUMN account_id TYPE VARCHAR(20); +END $$; diff --git a/internal/database/postgres/migrations/000095_purchase_history_account_id_width.up.sql b/internal/database/postgres/migrations/000095_purchase_history_account_id_width.up.sql new file mode 100644 index 000000000..f88977ff2 --- /dev/null +++ b/internal/database/postgres/migrations/000095_purchase_history_account_id_width.up.sql @@ -0,0 +1,106 @@ +-- Migration 000095: widen purchase_history.account_id to VARCHAR(255). +-- +-- account_id has been VARCHAR(20) since 000001. That fits an AWS account ID +-- (12 digits) and nothing else: +-- +-- * an Azure subscription ID is a 36-character GUID +-- * a GCP project ID is 6 to 30 characters +-- +-- The value written into this column is cloud_accounts.external_id, which is +-- VARCHAR(255) at its source (000011), so nothing upstream constrains it. +-- SavePurchaseHistory therefore issues an INSERT that Postgres rejects with +-- SQLSTATE 22001 (`value too long for type character varying(20)`) for every +-- Azure purchase and every GCP purchase whose project ID exceeds 20 chars. +-- +-- The purchase itself has already succeeded and been billed by the time this +-- INSERT runs, so the failure loses the audit row for real money spent. The +-- error is not swallowed -- savePurchaseHistory returns it and the caller +-- stamps a `history_write_failed` audit-gap marker on the execution +-- (issue #621) -- but the purchase_history row is gone: invisible in the +-- History view, absent from GetActivePurchaseHistory (so analytics undercount +-- committed spend), and unseen by the grace-period and suppression logic. +-- +-- The identical widening was already applied to the sibling column: +-- migration 000067 widened savings_snapshots.account_id to VARCHAR(255) for +-- exactly this reason, and 000074 repaired it on partially-migrated +-- databases. purchase_history was missed. This closes that gap and lands on +-- VARCHAR(255), matching both cloud_accounts.external_id and +-- savings_snapshots.account_id. +-- +-- Operator note: rows already lost to 22001 cannot be recovered by this +-- migration -- they were never inserted. Affected executions are identifiable +-- via `SELECT execution_id, error FROM purchase_executions WHERE error LIKE +-- '%history_write_failed%'`, whose marker carries the provider-side commitment +-- ID; reconciliation has to come from there or from the provider console. The +-- wildcard on both sides is deliberate: recordHistoryAuditGap appends the +-- marker via appendErrNote, so it is not necessarily at the start of the +-- field when the execution already carried an error. +-- +-- ------------------------------------------------------------------ +-- Why the probe, and why pg_attribute rather than information_schema +-- ------------------------------------------------------------------ +-- Guarded so it is correct on a fresh database, on an already-deployed one, +-- and on re-run under the auto-heal path (project rule +-- feedback_migration_full_restore: IF NOT EXISTS does not repair a wrong +-- column type, so the ALTER has to be conditioned on the observed type +-- rather than skipped). +-- +-- 000067 probed information_schema.columns without a schema predicate. This +-- one resolves the table through 'purchase_history'::regclass instead, which +-- follows the connection's search_path in EXACTLY the way the bare +-- `ALTER TABLE purchase_history` below does. buildMigrateDSN deliberately +-- appends no connection options (RDS Proxy does not support them), so +-- search_path is whatever the role default is; a probe that resolved the +-- table differently from the ALTER could silently match nothing, no-op, and +-- let golang-migrate record 000095 as applied while the p0 data loss +-- persisted. On a money path a migration must not be able to report success +-- without having done the work -- hence also the unconditional post-check +-- and the hard failure when the column is absent entirely. +-- +-- atttypmod encodes VARCHAR(n) as n + 4. A value of -1 means unbounded +-- `varchar`, which is WIDER than VARCHAR(255) and must be left alone -- +-- 000067's `character_maximum_length IS NULL` branch would have narrowed it. +-- +-- Nothing else depends on this column's type: no view, materialized view, +-- foreign key or partition references purchase_history, so no drop/recreate +-- dance is needed (contrast 000067, which had to drop three views first). +-- Widening a varchar is binary-coercible, so Postgres skips both the table +-- rewrite and any index rebuild -- idx_purchase_history_account_timestamp +-- (000002) is left in place and the ALTER is near-instant even on a large +-- table. That reasoning does NOT hold in reverse; see the down migration. + +DO $$ +DECLARE + typmod INTEGER; +BEGIN + SELECT atttypmod INTO typmod + FROM pg_attribute + WHERE attrelid = 'purchase_history'::regclass + AND attname = 'account_id' + AND NOT attisdropped; + + IF typmod IS NULL THEN + RAISE EXCEPTION + 'migration 000095: column purchase_history.account_id does not exist'; + END IF; + + IF typmod <> -1 AND typmod - 4 < 255 THEN + ALTER TABLE purchase_history + ALTER COLUMN account_id TYPE VARCHAR(255); + + -- Post-check: re-read the catalog and fail loudly if the widening + -- did not take effect, so this migration can never be recorded as + -- applied while account_id is still too narrow for Azure/GCP. + SELECT atttypmod INTO typmod + FROM pg_attribute + WHERE attrelid = 'purchase_history'::regclass + AND attname = 'account_id' + AND NOT attisdropped; + + IF typmod <> -1 AND typmod - 4 < 255 THEN + RAISE EXCEPTION + 'migration 000095 failed to widen purchase_history.account_id: still VARCHAR(%)', + typmod - 4; + END IF; + END IF; +END $$; diff --git a/internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go b/internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go new file mode 100644 index 000000000..6aac77b4a --- /dev/null +++ b/internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go @@ -0,0 +1,242 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + "time" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/jackc/pgx/v5/pgconn" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Realistic non-AWS account identifiers that do not fit VARCHAR(20). +// +// - azureSubscriptionID is a 36-character GUID, the shape every Azure +// subscription ID has (cloud_accounts.azure_subscription_id is VARCHAR(36)). +// - gcpProjectID is 30 characters, the documented GCP maximum (6-30 chars, +// lowercase letters / digits / hyphens, starting with a letter). +// +// Both reach SavePurchaseHistory as cloud_accounts.external_id, which is +// VARCHAR(255) at its source, so nothing upstream trims them. +const ( + azureSubscriptionID = "3f2504e0-4f89-11d3-9a0c-0305e82c3301" + gcpProjectID = "cudly-production-analytics-001" +) + +// insertPurchaseHistoryRow issues the same INSERT column set that +// PostgresStore.SavePurchaseHistory uses for its NOT-NULL-without-default +// columns, with accountID as purchase_history.account_id. plan_id and +// cloud_account_id are omitted (nullable FKs) so the test needs no fixtures. +func insertPurchaseHistoryRow(ctx context.Context, t *testing.T, pool *pgxpool.Pool, accountID, purchaseID string) error { + t.Helper() + const insertSQL = ` + INSERT INTO purchase_history ( + account_id, purchase_id, timestamp, provider, service, region, + resource_type, term, payment + ) VALUES ($1, $2, $3, 'azure', 'compute', 'westeurope', + 'Standard_D4s_v3', 12, 'All Upfront')` + _, err := pool.Exec(ctx, insertSQL, accountID, purchaseID, time.Now()) + return err +} + +// accountIDMaxLength reads the declared width of purchase_history.account_id +// straight from the catalog, so the assertion is about the real schema rather +// than about what the migration file claims to do. It deliberately probes via +// information_schema rather than reusing the migration's own pg_attribute / +// regclass expression: an independent lookup cannot rubber-stamp a bug in the +// migration's probe. +func accountIDMaxLength(ctx context.Context, t *testing.T, pool *pgxpool.Pool) int { + t.Helper() + var maxLen int + require.NoError(t, pool.QueryRow(ctx, ` + SELECT character_maximum_length + FROM information_schema.columns + WHERE table_schema = current_schema() + AND table_name = 'purchase_history' + AND column_name = 'account_id'`).Scan(&maxLen)) + return maxLen +} + +// TestMigration095_PurchaseHistoryAccountIDWidth is the fail-before/pass-after +// regression test for issue #1603. +// +// It replicates the real failing scenario rather than a narrower unit: the +// exact INSERT column set SavePurchaseHistory issues, carrying the account +// identifier an Azure or GCP purchase actually produces. On the pre-migration +// schema (version 000092) both inserts are rejected by Postgres with SQLSTATE +// 22001, which is precisely how the audit row for an already-billed commitment +// is lost. After 000095 both succeed and round-trip untruncated. +func TestMigration095_PurchaseHistoryAccountIDWidth(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + if err != nil { + t.Skipf("Skipping test: could not setup postgres container: %v", err) + } + defer container.Cleanup(ctx) + + pool := container.DB.Pool() + + // ---- Pre-migration state: pinned at 000092, the last version whose + // schema this test depends on. Pinned to an explicit version rather than + // migrated to HEAD so the next migration PR does not break this test's CI + // (feedback_migration_test_pin_version); any migration landing between 92 + // and 95 is applied by the MigrateToVersion(95) call below. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), 92), + "migrations must apply cleanly through 000092") + + require.Equal(t, 20, accountIDMaxLength(ctx, t, pool), + "pre-fix schema must still have the VARCHAR(20) account_id this migration exists to widen") + + // The bug: both non-AWS identifiers are rejected outright. + for _, tc := range []struct { + name string + accountID string + }{ + {"azure subscription GUID (36 chars)", azureSubscriptionID}, + {"gcp project ID (30 chars)", gcpProjectID}, + } { + t.Run("pre-migration/"+tc.name, func(t *testing.T) { + insErr := insertPurchaseHistoryRow(ctx, t, pool, tc.accountID, "pre-"+tc.accountID) + require.Error(t, insErr, + "pre-fix schema must reject the audit row -- this is the data loss in issue #1603") + + t.Logf("pre-fix rejection for %s: %v", tc.accountID, insErr) + + var pgErr *pgconn.PgError + require.True(t, errors.As(insErr, &pgErr), "expected a Postgres error, got %v", insErr) + assert.Equal(t, "22001", pgErr.Code, + "expected string_data_right_truncation; the purchase is already billed when this INSERT runs") + }) + } + + // ---- Apply 000095. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), 95), + "migration 000095 must apply cleanly on top of 000092") + + assert.Equal(t, 255, accountIDMaxLength(ctx, t, pool), + "account_id must be VARCHAR(255), matching cloud_accounts.external_id and savings_snapshots.account_id") + + // ---- Post-migration: the same inserts succeed and round-trip intact. + for _, tc := range []struct { + name string + accountID string + purchaseID string + }{ + {"azure subscription GUID (36 chars)", azureSubscriptionID, "azure-ri-1603"}, + {"gcp project ID (30 chars)", gcpProjectID, "gcp-cud-1603"}, + } { + t.Run("post-migration/"+tc.name, func(t *testing.T) { + require.NoError(t, + insertPurchaseHistoryRow(ctx, t, pool, tc.accountID, tc.purchaseID), + "audit row for an already-billed commitment must persist after 000095") + + var stored string + require.NoError(t, pool.QueryRow(ctx, + `SELECT account_id FROM purchase_history WHERE purchase_id = $1`, + tc.purchaseID).Scan(&stored)) + assert.Equal(t, tc.accountID, stored, + "account_id must round-trip untruncated") + }) + } + + // ---- Re-running the up migration must be a no-op, not a failure. The + // header claims correctness "on re-run under the auto-heal path"; this + // exercises that claim directly by executing the file's SQL a second time + // against an already-widened column. + upSQL, err := os.ReadFile(filepath.Join(getMigrationsPath(), + "000095_purchase_history_account_id_width.up.sql")) + require.NoError(t, err, "the up migration file must be readable") + _, err = pool.Exec(ctx, string(upSQL)) + require.NoError(t, err, "re-running 000095 on an already-widened column must be a no-op") + assert.Equal(t, 255, accountIDMaxLength(ctx, t, pool), + "a re-run must leave account_id at VARCHAR(255)") +} + +// TestMigration095_DownNarrowsWhenSafe covers the rollback's success path: with +// no over-long value present, narrowing back to VARCHAR(20) is lossless and +// must actually happen. Without this the ALTER branch after the guard is never +// executed by any test and a typo in it would ship green. +func TestMigration095_DownNarrowsWhenSafe(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + if err != nil { + t.Skipf("Skipping test: could not setup postgres container: %v", err) + } + defer container.Cleanup(ctx) + + pool := container.DB.Pool() + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), 95), + "migrations must apply cleanly through 000095") + + // A 12-digit AWS account ID fits VARCHAR(20), so the rollback is lossless. + require.NoError(t, + insertPurchaseHistoryRow(ctx, t, pool, "123456789012", "aws-ri-down-1603"), + "setup: an AWS audit row must be insertable") + + // One step, so exactly 000095's down migration runs. Targeting a version + // would also run the downs of any migration that lands between 92 and 95, + // coupling this test to unrelated future rollbacks. + require.NoError(t, migrations.RollbackMigrations(ctx, pool, getMigrationsPath(), 1), + "rollback must succeed when every account_id fits VARCHAR(20)") + + assert.Equal(t, 20, accountIDMaxLength(ctx, t, pool), + "the rollback must actually narrow the column, not silently no-op") + + var stored string + require.NoError(t, pool.QueryRow(ctx, + `SELECT account_id FROM purchase_history WHERE purchase_id = 'aws-ri-down-1603'`).Scan(&stored)) + assert.Equal(t, "123456789012", stored, "the lossless rollback must preserve the row") +} + +// TestMigration095_DownRefusesToTruncate verifies that the rollback fails +// loudly instead of silently truncating. Narrowing account_id back to +// VARCHAR(20) with an Azure subscription GUID present would destroy the audit +// trail this table exists to provide, so the down migration must refuse. +func TestMigration095_DownRefusesToTruncate(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + if err != nil { + t.Skipf("Skipping test: could not setup postgres container: %v", err) + } + defer container.Cleanup(ctx) + + pool := container.DB.Pool() + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), 95), + "migrations must apply cleanly through 000095") + + require.NoError(t, + insertPurchaseHistoryRow(ctx, t, pool, azureSubscriptionID, "azure-ri-down-1603"), + "setup: an Azure audit row must be insertable after 000095") + + // One step, so exactly 000095's down migration runs (see the sibling test). + err = migrations.RollbackMigrations(ctx, pool, getMigrationsPath(), 1) + require.Error(t, err, + "rolling back 000095 with an over-long account_id present must fail rather than truncate") + assert.Contains(t, err.Error(), "refusing to narrow purchase_history.account_id", + "the failure must name the reason so an operator does not reach for a truncating USING clause") + + // The row is still intact: the guard fires before any rewrite. + var stored string + require.NoError(t, pool.QueryRow(ctx, + `SELECT account_id FROM purchase_history WHERE purchase_id = 'azure-ri-down-1603'`).Scan(&stored)) + assert.Equal(t, azureSubscriptionID, stored, + "the refused rollback must leave the audit row untouched") +} From c6ad11e4efd3f528bfb9236f341857cce5ab8206 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:42:19 +0200 Subject: [PATCH 2/3] test(db): give each 000095 fixture its own provider shape The shared insert helper hardcoded provider='azure' with Azure's service, region and resource_type, so the GCP case inserted an Azure-shaped row and only the account_id differed. In a test whose entire point is that Azure and GCP identifiers overflow VARCHAR(20), a GCP case carrying 'westeurope'/'Standard_D4s_v3' misrepresents the scenario it claims to cover. Group the identifier with its provider/service/region/resource_type in a commitmentRow fixture and pass those through as bind parameters, so each case inserts a row its own provider would actually produce. Adds an AWS fixture for the lossless-rollback test, which previously used a bare 12-digit literal. Test-only; no migration or production code changes. --- ..._purchase_history_account_id_width_test.go | 87 ++++++++++++------- 1 file changed, 58 insertions(+), 29 deletions(-) diff --git a/internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go b/internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go index 6aac77b4a..0b5597b34 100644 --- a/internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go +++ b/internal/database/postgres/migrations/000095_purchase_history_account_id_width_test.go @@ -19,33 +19,62 @@ import ( "github.com/stretchr/testify/require" ) -// Realistic non-AWS account identifiers that do not fit VARCHAR(20). +// commitmentRow is the provider-shaped part of a purchase_history row: the +// account identifier under test plus the provider/service/region/resource_type +// that a real commitment from that provider carries. Keeping them together +// means the GCP case inserts a GCP-shaped row rather than borrowing Azure's, +// so the row each assertion exercises matches the scenario it claims to cover. +type commitmentRow struct { + provider string + service string + region string + resourceType string + accountID string +} + +// The account identifiers that do not fit VARCHAR(20): // -// - azureSubscriptionID is a 36-character GUID, the shape every Azure -// subscription ID has (cloud_accounts.azure_subscription_id is VARCHAR(36)). -// - gcpProjectID is 30 characters, the documented GCP maximum (6-30 chars, -// lowercase letters / digits / hyphens, starting with a letter). +// - azureCommitment carries a 36-character subscription GUID, the shape every +// Azure subscription ID has (cloud_accounts.azure_subscription_id is +// VARCHAR(36)). +// - gcpCommitment carries a 30-character project ID, the documented GCP +// maximum (6-30 chars, lowercase letters / digits / hyphens, starting with +// a letter). // // Both reach SavePurchaseHistory as cloud_accounts.external_id, which is -// VARCHAR(255) at its source, so nothing upstream trims them. -const ( - azureSubscriptionID = "3f2504e0-4f89-11d3-9a0c-0305e82c3301" - gcpProjectID = "cudly-production-analytics-001" +// VARCHAR(255) at its source, so nothing upstream trims them. awsCommitment is +// the 12-digit control that has always fit. +var ( + azureCommitment = commitmentRow{ + provider: "azure", service: "compute", region: "westeurope", + resourceType: "Standard_D4s_v3", + accountID: "3f2504e0-4f89-11d3-9a0c-0305e82c3301", + } + gcpCommitment = commitmentRow{ + provider: "gcp", service: "compute", region: "europe-west1", + resourceType: "n2-standard-4", + accountID: "cudly-production-analytics-001", + } + awsCommitment = commitmentRow{ + provider: "aws", service: "ec2", region: "us-east-1", + resourceType: "m5.large", + accountID: "123456789012", + } ) // insertPurchaseHistoryRow issues the same INSERT column set that // PostgresStore.SavePurchaseHistory uses for its NOT-NULL-without-default -// columns, with accountID as purchase_history.account_id. plan_id and +// columns, with row.accountID as purchase_history.account_id. plan_id and // cloud_account_id are omitted (nullable FKs) so the test needs no fixtures. -func insertPurchaseHistoryRow(ctx context.Context, t *testing.T, pool *pgxpool.Pool, accountID, purchaseID string) error { +func insertPurchaseHistoryRow(ctx context.Context, t *testing.T, pool *pgxpool.Pool, row commitmentRow, purchaseID string) error { t.Helper() const insertSQL = ` INSERT INTO purchase_history ( account_id, purchase_id, timestamp, provider, service, region, resource_type, term, payment - ) VALUES ($1, $2, $3, 'azure', 'compute', 'westeurope', - 'Standard_D4s_v3', 12, 'All Upfront')` - _, err := pool.Exec(ctx, insertSQL, accountID, purchaseID, time.Now()) + ) VALUES ($1, $2, $3, $4, $5, $6, $7, 12, 'All Upfront')` + _, err := pool.Exec(ctx, insertSQL, row.accountID, purchaseID, time.Now(), + row.provider, row.service, row.region, row.resourceType) return err } @@ -101,18 +130,18 @@ func TestMigration095_PurchaseHistoryAccountIDWidth(t *testing.T) { // The bug: both non-AWS identifiers are rejected outright. for _, tc := range []struct { - name string - accountID string + name string + row commitmentRow }{ - {"azure subscription GUID (36 chars)", azureSubscriptionID}, - {"gcp project ID (30 chars)", gcpProjectID}, + {"azure subscription GUID (36 chars)", azureCommitment}, + {"gcp project ID (30 chars)", gcpCommitment}, } { t.Run("pre-migration/"+tc.name, func(t *testing.T) { - insErr := insertPurchaseHistoryRow(ctx, t, pool, tc.accountID, "pre-"+tc.accountID) + insErr := insertPurchaseHistoryRow(ctx, t, pool, tc.row, "pre-"+tc.row.accountID) require.Error(t, insErr, "pre-fix schema must reject the audit row -- this is the data loss in issue #1603") - t.Logf("pre-fix rejection for %s: %v", tc.accountID, insErr) + t.Logf("pre-fix rejection for %s: %v", tc.row.accountID, insErr) var pgErr *pgconn.PgError require.True(t, errors.As(insErr, &pgErr), "expected a Postgres error, got %v", insErr) @@ -131,22 +160,22 @@ func TestMigration095_PurchaseHistoryAccountIDWidth(t *testing.T) { // ---- Post-migration: the same inserts succeed and round-trip intact. for _, tc := range []struct { name string - accountID string + row commitmentRow purchaseID string }{ - {"azure subscription GUID (36 chars)", azureSubscriptionID, "azure-ri-1603"}, - {"gcp project ID (30 chars)", gcpProjectID, "gcp-cud-1603"}, + {"azure subscription GUID (36 chars)", azureCommitment, "azure-ri-1603"}, + {"gcp project ID (30 chars)", gcpCommitment, "gcp-cud-1603"}, } { t.Run("post-migration/"+tc.name, func(t *testing.T) { require.NoError(t, - insertPurchaseHistoryRow(ctx, t, pool, tc.accountID, tc.purchaseID), + insertPurchaseHistoryRow(ctx, t, pool, tc.row, tc.purchaseID), "audit row for an already-billed commitment must persist after 000095") var stored string require.NoError(t, pool.QueryRow(ctx, `SELECT account_id FROM purchase_history WHERE purchase_id = $1`, tc.purchaseID).Scan(&stored)) - assert.Equal(t, tc.accountID, stored, + assert.Equal(t, tc.row.accountID, stored, "account_id must round-trip untruncated") }) } @@ -185,7 +214,7 @@ func TestMigration095_DownNarrowsWhenSafe(t *testing.T) { // A 12-digit AWS account ID fits VARCHAR(20), so the rollback is lossless. require.NoError(t, - insertPurchaseHistoryRow(ctx, t, pool, "123456789012", "aws-ri-down-1603"), + insertPurchaseHistoryRow(ctx, t, pool, awsCommitment, "aws-ri-down-1603"), "setup: an AWS audit row must be insertable") // One step, so exactly 000095's down migration runs. Targeting a version @@ -200,7 +229,7 @@ func TestMigration095_DownNarrowsWhenSafe(t *testing.T) { var stored string require.NoError(t, pool.QueryRow(ctx, `SELECT account_id FROM purchase_history WHERE purchase_id = 'aws-ri-down-1603'`).Scan(&stored)) - assert.Equal(t, "123456789012", stored, "the lossless rollback must preserve the row") + assert.Equal(t, awsCommitment.accountID, stored, "the lossless rollback must preserve the row") } // TestMigration095_DownRefusesToTruncate verifies that the rollback fails @@ -223,7 +252,7 @@ func TestMigration095_DownRefusesToTruncate(t *testing.T) { "migrations must apply cleanly through 000095") require.NoError(t, - insertPurchaseHistoryRow(ctx, t, pool, azureSubscriptionID, "azure-ri-down-1603"), + insertPurchaseHistoryRow(ctx, t, pool, azureCommitment, "azure-ri-down-1603"), "setup: an Azure audit row must be insertable after 000095") // One step, so exactly 000095's down migration runs (see the sibling test). @@ -237,6 +266,6 @@ func TestMigration095_DownRefusesToTruncate(t *testing.T) { var stored string require.NoError(t, pool.QueryRow(ctx, `SELECT account_id FROM purchase_history WHERE purchase_id = 'azure-ri-down-1603'`).Scan(&stored)) - assert.Equal(t, azureSubscriptionID, stored, + assert.Equal(t, azureCommitment.accountID, stored, "the refused rollback must leave the audit row untouched") } From 6eb2777521241d35c9733e131dd76e18c4ba5071 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:47:16 +0200 Subject: [PATCH 3/3] test(db): derive the rollback target from disk, not head minus one TestMigrations_AutoHealDirty rolls back one migration and asserted the resulting version equals headVersion-1. That encodes an invariant this repository does not hold: migration numbers are not contiguous, because renumbering to dodge collisions with in-flight PRs leaves gaps. The set already skips 61-62, 68-69, 82, 84-85. Those gaps were harmless only because none of them sat immediately below head, so headVersion-1 happened to be a real migration. Adding 000095 on top of main's 000093 puts a gap directly below head for the first time, and golang-migrate's Steps(-1) lands on 93 -- the next version that actually exists -- so the assertion failed with expected 94, actual 93. Derive the expected version from the .up.sql filenames instead. This is the root fix rather than a renumber: the next migration to land above a gap would have hit the same assertion, and renumbering 000095 to 000094 would have collided with the in-flight #1523. Test-only; no migration or production code changes. --- .../postgres/migrations/helpers_test.go | 45 +++++++++++++++++++ .../migrations/migrate_autoheal_test.go | 4 +- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/internal/database/postgres/migrations/helpers_test.go b/internal/database/postgres/migrations/helpers_test.go index d466cc9e6..3f73842fe 100644 --- a/internal/database/postgres/migrations/helpers_test.go +++ b/internal/database/postgres/migrations/helpers_test.go @@ -9,6 +9,9 @@ import ( "os" "path/filepath" "runtime" + "sort" + "strconv" + "strings" "testing" "github.com/stretchr/testify/require" @@ -22,6 +25,48 @@ func getMigrationsPath() string { return filepath.Dir(filename) } +// migrationVersionsDesc returns every migration version present on disk, newest +// first, read from the `.up.sql` filenames. +func migrationVersionsDesc(t *testing.T) []uint { + t.Helper() + paths, err := filepath.Glob(filepath.Join(getMigrationsPath(), "*.up.sql")) + require.NoError(t, err, "globbing migration files must succeed") + require.NotEmpty(t, paths, "no migration files found") + + versions := make([]uint, 0, len(paths)) + for _, p := range paths { + base := filepath.Base(p) + idx := strings.IndexByte(base, '_') + require.Positive(t, idx, "migration filename %q must be _.up.sql", base) + n, err := strconv.ParseUint(base[:idx], 10, 64) + require.NoError(t, err, "migration filename %q must start with a numeric version", base) + versions = append(versions, uint(n)) + } + sort.Slice(versions, func(i, j int) bool { return versions[i] > versions[j] }) + return versions +} + +// previousMigrationVersion returns the highest migration version strictly below +// version. +// +// Migration numbers in this repository are NOT contiguous -- renumbering to +// dodge collisions with in-flight PRs leaves gaps (60->63, 67->70, 81->83, +// 83->86, 93->95). So `version - 1` is not necessarily a real migration, and a +// test that rolls back one step lands on the next version that actually EXISTS. +// Deriving it from the files on disk keeps such tests correct no matter where +// the gaps fall -- including the case that motivated this helper, a gap +// immediately below head. +func previousMigrationVersion(t *testing.T, version uint) uint { + t.Helper() + for _, v := range migrationVersionsDesc(t) { + if v < version { + return v + } + } + t.Fatalf("no migration version below %d", version) + return 0 +} + // captureStdout redirects os.Stdout to a pipe and returns a function that // closes the pipe, restores stdout, and returns everything written to it. // diff --git a/internal/database/postgres/migrations/migrate_autoheal_test.go b/internal/database/postgres/migrations/migrate_autoheal_test.go index 11dfdb1d2..973230449 100644 --- a/internal/database/postgres/migrations/migrate_autoheal_test.go +++ b/internal/database/postgres/migrations/migrate_autoheal_test.go @@ -162,8 +162,8 @@ func TestMigrations_AutoHealDirty(t *testing.T) { "rollback of the last migration must succeed") versionAfterRollback, _, err := migrations.GetMigrationVersion(ctx, pool, migrationsPath) require.NoError(t, err) - require.Equal(t, headVersion-1, versionAfterRollback, - "rollback must land exactly one version below head") + require.Equal(t, previousMigrationVersion(t, headVersion), versionAfterRollback, + "rollback must land on the next migration version that exists below head") // Inject dirty=true at headVersion, simulating a migration that started // (schema_migrations updated to dirty) but whose SQL was rolled back