From e0f53a0bde7d11c575c09943518b265462c03811 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 19:16:12 +0000 Subject: [PATCH] fix(db): make purchase_history.ramp_step and purchase_executions.error NOT NULL Both columns have been nullable since migration 000001, but the Go readers scan them into plain values (ramp_step into an int; error into a string, tolerated since #1473). The writers never store NULL, so a NULL only comes from a legacy or hand-written row, and one such purchase_history row fails the scan of the whole history listing. Migration 000146 backfills NULL to 0 / '' and sets both columns NOT NULL with that default. Both tables are locked ACCESS EXCLUSIVE before the backfill so a NULL committed by a concurrent older writer cannot slip in between the UPDATE and SET NOT NULL, and the purchase_executions updated_at trigger is disabled for the backfill statement only, so the lifecycle clock of legacy rows is not reset. Down makes both columns nullable again. Adds real-Postgres migration tests (backfill, rows survive, updated_at kept, defaults, NULL rejected, GetAllPurchaseHistory lists the formerly-NULL row, idempotent up, down, and a concurrent in-flight NULL writer). The #1465 regression test now relaxes the constraint in its own database to keep covering the scanner's NULL tolerance. Closes #1484 Co-Authored-By: Claude Opus 5.5 --- ...s_execution_null_error_integration_test.go | 7 +- ...amp_step_execution_error_not_null.down.sql | 10 + ..._ramp_step_execution_error_not_null.up.sql | 60 ++++ ...ramp_step_execution_error_not_null_test.go | 272 ++++++++++++++++++ 4 files changed, 348 insertions(+), 1 deletion(-) create mode 100644 internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null.down.sql create mode 100644 internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null.up.sql create mode 100644 internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null_test.go diff --git a/internal/config/store_postgres_execution_null_error_integration_test.go b/internal/config/store_postgres_execution_null_error_integration_test.go index d8409c84c..0e5b7e028 100644 --- a/internal/config/store_postgres_execution_null_error_integration_test.go +++ b/internal/config/store_postgres_execution_null_error_integration_test.go @@ -35,7 +35,12 @@ func TestIntegration_ExecutionReaders_NullErrorColumn(t *testing.T) { other := saveClean(1, "pending") nullErr := saveClean(2, "pending") last := saveClean(3, "notified") - _, err := store.db.Exec(ctx, `UPDATE purchase_executions SET error = NULL WHERE execution_id = $1`, nullErr) + // Migration 000146 (issue #1484) made the column NOT NULL. The scanner + // still tolerates NULL for a schema rolled back below 000146, so relax + // the constraint in this test's own database to seed the legacy shape. + _, err := store.db.Exec(ctx, `ALTER TABLE purchase_executions ALTER COLUMN error DROP NOT NULL`) + require.NoError(t, err) + _, err = store.db.Exec(ctx, `UPDATE purchase_executions SET error = NULL WHERE execution_id = $1`, nullErr) require.NoError(t, err) wantIDs := []string{last, nullErr, other} diff --git a/internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null.down.sql b/internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null.down.sql new file mode 100644 index 000000000..759dd1149 --- /dev/null +++ b/internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null.down.sql @@ -0,0 +1,10 @@ +-- Revert migration 000146: both columns become nullable again with no default, +-- as 000001 left them. The backfilled 0 / '' values stay: they are what the +-- Go readers already return for NULL, so there is nothing to restore. +ALTER TABLE purchase_history + ALTER COLUMN ramp_step DROP NOT NULL, + ALTER COLUMN ramp_step DROP DEFAULT; + +ALTER TABLE purchase_executions + ALTER COLUMN error DROP NOT NULL, + ALTER COLUMN error DROP DEFAULT; diff --git a/internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null.up.sql b/internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null.up.sql new file mode 100644 index 000000000..c45585848 --- /dev/null +++ b/internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null.up.sql @@ -0,0 +1,60 @@ +-- Migration 000146: purchase_history.ramp_step and purchase_executions.error +-- become NOT NULL with a default (issue #1484). +-- +-- Both columns have been nullable since 000001, but the Go readers scan them +-- into plain values: ramp_step into an int, error into a string (the error +-- scanner has tolerated NULL since #1473; the purchase_history readers do +-- not). The Go writers never store NULL, so a NULL can only come from a legacy +-- or hand-written row, and one such purchase_history row fails the scan of the +-- whole listing (GetAllPurchaseHistory and the other history readers). +-- +-- The fix is at the schema: backfill NULL to the value the Go zero value +-- already means (ramp_step 0 = not part of a ramp step, error '' = no error), +-- then set NOT NULL with that value as the default. +-- +-- ------------------------------------------------------------------ +-- Concurrency +-- ------------------------------------------------------------------ +-- The migration runs as one implicit transaction (see +-- migration_transactional_test.go). Both tables are locked ACCESS EXCLUSIVE +-- FIRST, in one statement, before the backfill. Without the lock a writer +-- still running an older build, or a hand-written INSERT, could commit a NULL +-- between the UPDATE and SET NOT NULL, and SET NOT NULL would then fail the +-- whole migration. Taking ACCESS EXCLUSIVE up front (rather than letting the +-- UPDATE take ROW EXCLUSIVE and the ALTER upgrade it later) also avoids a +-- lock upgrade that could deadlock with a reader that goes on to write. +-- Writers wait for the commit; the backfill only touches NULL rows and SET +-- NOT NULL is one scan, so the lock is short. +-- +-- ------------------------------------------------------------------ +-- updated_at +-- ------------------------------------------------------------------ +-- update_purchase_executions_updated_at would stamp NOW() on every backfilled +-- execution row. updated_at is a lifecycle clock (plan health windows its +-- failed/canceled counts on it, CleanupOldExecutions ages canceled rows by +-- it; see 000128), and turning a NULL error into '' is not a lifecycle event. +-- So the trigger is disabled for the backfill statement only. DISABLE TRIGGER +-- names only that trigger and needs no superuser (the same pattern as 000127 +-- and 000145); the file is one transaction, so it is re-enabled before commit +-- and a failure rolls both back. purchase_history has no UPDATE trigger. +-- +-- Idempotent: re-running finds no NULL rows, and SET DEFAULT / SET NOT NULL +-- on a column that already has them are no-ops. + +LOCK TABLE purchase_executions, purchase_history IN ACCESS EXCLUSIVE MODE; + +UPDATE purchase_history SET ramp_step = 0 WHERE ramp_step IS NULL; + +ALTER TABLE purchase_history + ALTER COLUMN ramp_step SET DEFAULT 0, + ALTER COLUMN ramp_step SET NOT NULL; + +ALTER TABLE purchase_executions DISABLE TRIGGER update_purchase_executions_updated_at; + +UPDATE purchase_executions SET error = '' WHERE error IS NULL; + +ALTER TABLE purchase_executions ENABLE TRIGGER update_purchase_executions_updated_at; + +ALTER TABLE purchase_executions + ALTER COLUMN error SET DEFAULT '', + ALTER COLUMN error SET NOT NULL; diff --git a/internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null_test.go b/internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null_test.go new file mode 100644 index 000000000..32b54893c --- /dev/null +++ b/internal/database/postgres/migrations/000146_history_ramp_step_execution_error_not_null_test.go @@ -0,0 +1,272 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/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" +) + +const ( + m146Version = 146 + m146UpFile = "000146_history_ramp_step_execution_error_not_null.up.sql" +) + +// m146InsertHistory seeds a purchase_history row with the given ramp_step +// (nil seeds NULL, the legacy shape). Rows are read back by purchase_id. +func m146InsertHistory(ctx context.Context, t *testing.T, pool *pgxpool.Pool, purchaseID string, rampStep *int) { + t.Helper() + _, err := pool.Exec(ctx, ` + INSERT INTO purchase_history ( + account_id, purchase_id, timestamp, provider, service, region, + resource_type, term, payment, ramp_step + ) VALUES ('test-account', $1, NOW(), 'aws', 'ec2', 'us-east-1', + 'm5.large', 1, 'all-upfront', $2)`, purchaseID, rampStep) + require.NoError(t, err) +} + +// m146InsertExecution seeds a purchase_executions row with the given error +// (nil seeds NULL) and an updated_at an hour in the past, and returns its +// execution_id and the stored updated_at. +func m146InsertExecution(ctx context.Context, t *testing.T, pool *pgxpool.Pool, errText *string) (string, time.Time) { + t.Helper() + var id string + var updatedAt time.Time + require.NoError(t, pool.QueryRow(ctx, ` + INSERT INTO purchase_executions (status, scheduled_date, error, updated_at) + VALUES ('failed', NOW(), $1, NOW() - INTERVAL '1 hour') + RETURNING execution_id::text, updated_at`, errText).Scan(&id, &updatedAt)) + return id, updatedAt +} + +func m146RampStep(ctx context.Context, t *testing.T, pool *pgxpool.Pool, purchaseID string) *int { + t.Helper() + var v *int + require.NoError(t, pool.QueryRow(ctx, + `SELECT ramp_step FROM purchase_history WHERE purchase_id = $1`, purchaseID).Scan(&v)) + return v +} + +func m146Execution(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id string) (*string, time.Time) { + t.Helper() + var errText *string + var updatedAt time.Time + require.NoError(t, pool.QueryRow(ctx, + `SELECT error, updated_at FROM purchase_executions WHERE execution_id = $1`, id).Scan(&errText, &updatedAt)) + return errText, updatedAt +} + +// m146Column reports a column's nullability and default expression. +func m146Column(ctx context.Context, t *testing.T, pool *pgxpool.Pool, table, column string) (nullable bool, def *string) { + t.Helper() + var isNullable string + require.NoError(t, pool.QueryRow(ctx, ` + SELECT is_nullable, column_default FROM information_schema.columns + WHERE table_name = $1 AND column_name = $2`, table, column).Scan(&isNullable, &def)) + return isNullable == "YES", def +} + +func m146RequireNotNullViolation(t *testing.T, err error, msg string) { + t.Helper() + var pgErr *pgconn.PgError + require.True(t, errors.As(err, &pgErr), "%s: want a PostgreSQL error, got %v", msg, err) + assert.Equal(t, "23502", pgErr.Code, "%s: want not_null_violation", msg) +} + +func m146Ptr[T any](v T) *T { return &v } + +// TestMigration146_BackfillsAndEnforcesNotNull locks down migration 000146 +// (issue #1484) on a real PostgreSQL: +// - legacy NULL ramp_step / error rows are backfilled to 0 / empty string and every +// existing row survives with its other values intact; +// - the backfill does not stamp purchase_executions.updated_at, and the +// updated_at trigger is live again afterwards; +// - both columns are NOT NULL with a default, so an insert that omits them +// gets 0 / empty string and an explicit NULL is rejected; +// - GetAllPurchaseHistory lists the formerly-NULL row (before the migration, +// the NULL fails the scan of the whole listing); +// - the up file is idempotent and the down file makes both columns nullable +// again with no default. +// +// Fail-before: at the previous version the NULL rows stay NULL, +// GetAllPurchaseHistory fails to scan, and an explicit NULL insert succeeds. +func TestMigration146_BackfillsAndEnforcesNotNull(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + container := testhelpers.RequireEmptyDatabase(ctx, t) + pool := container.DB.Pool() + migrationsPath := getMigrationsPath() + prev := previousMigrationVersion(t, m146Version) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + + m146InsertHistory(ctx, t, pool, "ri-146-null", nil) + m146InsertHistory(ctx, t, pool, "ri-146-step", m146Ptr(3)) + nullExec, nullExecUpdated := m146InsertExecution(ctx, t, pool, nil) + errExec, _ := m146InsertExecution(ctx, t, pool, m146Ptr("boom")) + + store := config.NewPostgresStore(container.DB) + _, err := store.GetAllPurchaseHistory(ctx, 100) + require.Error(t, err, "precondition: a NULL ramp_step fails the history scan before 000146") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, m146Version)) + + assert.Equal(t, m146Ptr(0), m146RampStep(ctx, t, pool, "ri-146-null"), "NULL ramp_step is backfilled to 0") + assert.Equal(t, m146Ptr(3), m146RampStep(ctx, t, pool, "ri-146-step"), "a set ramp_step is untouched") + gotErr, gotUpdated := m146Execution(ctx, t, pool, nullExec) + assert.Equal(t, m146Ptr(""), gotErr, "NULL error is backfilled to ''") + assert.True(t, gotUpdated.Equal(nullExecUpdated), + "the backfill must not stamp updated_at: got %s, want %s", gotUpdated, nullExecUpdated) + gotErr, _ = m146Execution(ctx, t, pool, errExec) + assert.Equal(t, m146Ptr("boom"), gotErr, "a set error is untouched") + + history, err := store.GetAllPurchaseHistory(ctx, 100) + require.NoError(t, err, "the formerly-NULL row no longer breaks the listing") + assert.Len(t, history, 2) + + m146AssertConstraints(ctx, t, pool) + m146AssertUpdatedAtTriggerLive(ctx, t, pool, nullExec, nullExecUpdated) + + up, err := os.ReadFile(filepath.Join(migrationsPath, m146UpFile)) + require.NoError(t, err) + _, err = pool.Exec(ctx, string(up)) + require.NoError(t, err, "the up file must be idempotent") + m146AssertConstraints(ctx, t, pool) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + nullable, def := m146Column(ctx, t, pool, "purchase_history", "ramp_step") + assert.True(t, nullable, "down: ramp_step is nullable again") + assert.Nil(t, def, "down: ramp_step has no default") + nullable, def = m146Column(ctx, t, pool, "purchase_executions", "error") + assert.True(t, nullable, "down: error is nullable again") + assert.Nil(t, def, "down: error has no default") + m146InsertHistory(ctx, t, pool, "ri-146-down", nil) + m146InsertExecution(ctx, t, pool, nil) + assert.Equal(t, m146Ptr(3), m146RampStep(ctx, t, pool, "ri-146-step"), "down keeps existing rows") +} + +// m146AssertConstraints checks both columns are NOT NULL with the 0 / empty string +// default: an insert that omits them gets the default, an explicit NULL fails. +func m146AssertConstraints(ctx context.Context, t *testing.T, pool *pgxpool.Pool) { + t.Helper() + nullable, def := m146Column(ctx, t, pool, "purchase_history", "ramp_step") + assert.False(t, nullable, "ramp_step is NOT NULL") + require.NotNil(t, def) + assert.Equal(t, "0", *def) + nullable, def = m146Column(ctx, t, pool, "purchase_executions", "error") + assert.False(t, nullable, "error is NOT NULL") + require.NotNil(t, def) + assert.Equal(t, "''::text", *def) + + purchaseID := "ri-146-default-" + time.Now().Format("150405.000000000") + _, err := pool.Exec(ctx, ` + INSERT INTO purchase_history (account_id, purchase_id, timestamp, provider, service, region, resource_type, term, payment) + VALUES ('test-account', $1, NOW(), 'aws', 'ec2', 'us-east-1', 'm5.large', 1, 'all-upfront')`, purchaseID) + require.NoError(t, err) + assert.Equal(t, m146Ptr(0), m146RampStep(ctx, t, pool, purchaseID), "an omitted ramp_step defaults to 0") + _, err = pool.Exec(ctx, `DELETE FROM purchase_history WHERE purchase_id = $1`, purchaseID) + require.NoError(t, err) + + var execID, errText string + require.NoError(t, pool.QueryRow(ctx, ` + INSERT INTO purchase_executions (status, scheduled_date) VALUES ('failed', NOW()) + RETURNING execution_id::text, error`).Scan(&execID, &errText)) + assert.Equal(t, "", errText, "an omitted error defaults to ''") + _, err = pool.Exec(ctx, `DELETE FROM purchase_executions WHERE execution_id = $1`, execID) + require.NoError(t, err) + + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_history (account_id, purchase_id, timestamp, provider, service, region, resource_type, term, payment, ramp_step) + VALUES ('test-account', 'ri-146-explicit-null', NOW(), 'aws', 'ec2', 'us-east-1', 'm5.large', 1, 'all-upfront', NULL)`) + m146RequireNotNullViolation(t, err, "explicit NULL ramp_step") + _, err = pool.Exec(ctx, `INSERT INTO purchase_executions (status, scheduled_date, error) VALUES ('failed', NOW(), NULL)`) + m146RequireNotNullViolation(t, err, "explicit NULL error") +} + +// m146AssertUpdatedAtTriggerLive checks the migration re-enabled +// update_purchase_executions_updated_at: an ordinary update stamps a fresh +// updated_at again. +func m146AssertUpdatedAtTriggerLive(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id string, before time.Time) { + t.Helper() + var enabled string + require.NoError(t, pool.QueryRow(ctx, ` + SELECT tgenabled::text FROM pg_trigger + WHERE tgrelid = 'purchase_executions'::regclass AND tgname = 'update_purchase_executions_updated_at'`).Scan(&enabled)) + assert.Equal(t, "O", enabled, "the updated_at trigger is enabled after the migration") + + _, err := pool.Exec(ctx, `UPDATE purchase_executions SET error = 'later' WHERE execution_id = $1`, id) + require.NoError(t, err) + _, after := m146Execution(ctx, t, pool, id) + assert.True(t, after.After(before), "a real update stamps updated_at again") +} + +// TestMigration146_ConcurrentNullWriter covers a writer still running an older +// build (or a hand-written INSERT) that has a NULL ramp_step / error insert in +// flight when the migration starts. The migration must wait for it, then +// backfill its row and succeed. Without the up-front table lock, the backfill +// UPDATE does not see the uncommitted row, SET NOT NULL waits for the writer, +// then finds the NULL and fails the whole migration. +func TestMigration146_ConcurrentNullWriter(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + container := testhelpers.RequireEmptyDatabase(ctx, t) + pool := container.DB.Pool() + migrationsPath := getMigrationsPath() + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, m146Version))) + + writer, err := pool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = writer.Rollback(ctx) }() + _, err = writer.Exec(ctx, ` + INSERT INTO purchase_history (account_id, purchase_id, timestamp, provider, service, region, resource_type, term, payment, ramp_step) + VALUES ('test-account', 'ri-146-race', NOW(), 'aws', 'ec2', 'us-east-1', 'm5.large', 1, 'all-upfront', NULL)`) + require.NoError(t, err) + var raceExec string + require.NoError(t, writer.QueryRow(ctx, ` + INSERT INTO purchase_executions (status, scheduled_date, error) VALUES ('failed', NOW(), NULL) + RETURNING execution_id::text`).Scan(&raceExec)) + + done := make(chan error, 1) + go func() { done <- migrations.MigrateToVersion(ctx, pool, migrationsPath, m146Version) }() + + // Wait until the migration is blocked on one of the two tables, so the + // writer commits while the migration is mid-flight. + require.Eventually(t, func() bool { + var waiting int + if err := pool.QueryRow(ctx, ` + SELECT count(*) FROM pg_locks + WHERE NOT granted + AND relation IN ('purchase_history'::regclass, 'purchase_executions'::regclass)`).Scan(&waiting); err != nil { + return false + } + return waiting > 0 + }, time.Minute, 20*time.Millisecond, "the migration should wait on the in-flight writer") + + require.NoError(t, writer.Commit(ctx)) + require.NoError(t, <-done, "the migration must absorb a NULL committed while it waited") + + assert.Equal(t, m146Ptr(0), m146RampStep(ctx, t, pool, "ri-146-race")) + gotErr, _ := m146Execution(ctx, t, pool, raceExec) + assert.Equal(t, m146Ptr(""), gotErr) + nullable, _ := m146Column(ctx, t, pool, "purchase_history", "ramp_step") + assert.False(t, nullable) + nullable, _ = m146Column(ctx, t, pool, "purchase_executions", "error") + assert.False(t, nullable) +}