diff --git a/internal/api/handler_purchases_cancel_actor_integration_test.go b/internal/api/handler_purchases_cancel_actor_integration_test.go index 45dbee3b6..eb7030270 100644 --- a/internal/api/handler_purchases_cancel_actor_integration_test.go +++ b/internal/api/handler_purchases_cancel_actor_integration_test.go @@ -39,7 +39,7 @@ func TestDeletePlannedPurchase_RecordsCancelActor(t *testing.T) { require.Equal(t, config.StatusCanceled, row.Status) require.NotNil(t, row.CancelledBy, "cancel actor must be readable after a UI cancel") require.Equal(t, "creator@example.com", *row.CancelledBy) - requireRawCancelColumns(ctx, t, f, execID, "creator@example.com") + requireRawCanceledBy(ctx, t, f, execID, "creator@example.com") // The record TransitionExecutionStatus returns must use the same projection. directID := uuid.NewString() @@ -55,7 +55,7 @@ func TestDeletePlannedPurchase_RecordsCancelActor(t *testing.T) { // TestTransitionExecutionStatus_NonCancelLeavesCancelColumnsNull pins the // cancel-only guard: a non-cancel transition with an actor stamps -// transitioned_by but must not write either cancel column. +// transitioned_by but must not write canceled_by. func TestTransitionExecutionStatus_NonCancelLeavesCancelColumnsNull(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) t.Cleanup(cancel) @@ -72,26 +72,19 @@ func TestTransitionExecutionStatus_NonCancelLeavesCancelColumnsNull(t *testing.T _, err := f.store.TransitionExecutionStatus(ctx, execID, []string{"pending"}, config.StatusFailed, &creator) require.NoError(t, err) - var canceledBy, cancelledBy *string + var canceledBy *string require.NoError(t, f.pool.QueryRow(ctx, - "SELECT canceled_by, "+legacyCancelColumn+" FROM purchase_executions WHERE execution_id=$1", execID, - ).Scan(&canceledBy, &cancelledBy)) + "SELECT canceled_by FROM purchase_executions WHERE execution_id=$1", execID, + ).Scan(&canceledBy)) require.Nil(t, canceledBy, "canceled_by must stay NULL on a non-cancel transition") - require.Nil(t, cancelledBy, legacyCancelColumn+" must stay NULL on a non-cancel transition") } -func requireRawCancelColumns(ctx context.Context, t *testing.T, f *createConcurrencyFixture, execID, want string) { +func requireRawCanceledBy(ctx context.Context, t *testing.T, f *createConcurrencyFixture, execID, want string) { t.Helper() - var canceledBy, cancelledBy *string + var canceledBy *string require.NoError(t, f.pool.QueryRow(ctx, - "SELECT canceled_by, "+legacyCancelColumn+" FROM purchase_executions WHERE execution_id=$1", execID, - ).Scan(&canceledBy, &cancelledBy)) + "SELECT canceled_by FROM purchase_executions WHERE execution_id=$1", execID, + ).Scan(&canceledBy)) require.NotNil(t, canceledBy, "canceled_by column") - require.NotNil(t, cancelledBy, legacyCancelColumn+" column") require.Equal(t, want, *canceledBy) - require.Equal(t, want, *cancelledBy) } - -// legacyCancelColumn is the pre-migration-000089 British spelling, split so the -// misspell linter does not flag the column name. -const legacyCancelColumn = "cancel" + "led_by" diff --git a/internal/api/handler_purchases_legacy_cancel_integration_test.go b/internal/api/handler_purchases_legacy_cancel_integration_test.go new file mode 100644 index 000000000..47ae8d90d --- /dev/null +++ b/internal/api/handler_purchases_legacy_cancel_integration_test.go @@ -0,0 +1,77 @@ +//go:build integration + +package api + +import ( + "context" + "os" + "strconv" + "strings" + "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/stretchr/testify/require" +) + +// versionBeforeCanceledContract is the schema a deployment is at when the +// build under test starts: the highest migration below the canceled contract +// (the file ending _contract_canceled_spelling.up.sql), which narrows the +// status CHECKs. The cutoff is read from that file, so a renumber needs no edit +// here. The new build must read and write correctly there, because the +// migration and the release roll out in either order. +func versionBeforeCanceledContract(t *testing.T, dir string) uint { + t.Helper() + entries, err := os.ReadDir(dir) + require.NoError(t, err) + prefix := func(name string) (uint, bool) { + v, err := strconv.ParseUint(strings.SplitN(name, "_", 2)[0], 10, 32) + return uint(v), err == nil + } + var contract uint + for _, e := range entries { + if v, ok := prefix(e.Name()); ok && strings.HasSuffix(e.Name(), "_contract_canceled_spelling.up.sql") { + contract = v + } + } + require.NotZero(t, contract, "no *_contract_canceled_spelling.up.sql migration in %s", dir) + var best uint + for _, e := range entries { + if v, ok := prefix(e.Name()); ok && v < contract && v > best { + best = v + } + } + require.NotZero(t, best, "no migration below %d", contract) + return best +} + +// TestCancelOrRecoverExecution_LegacyCanceledRowBeforeContractMigration runs +// the cancel-race recovery on the schema before 000157 against a row an older +// build canceled (legacy status spelling, actor only in the legacy column). The +// build and the migration roll out in either order, so the retry must see the +// race as already won, return the canonical status and keep the actor. +func TestCancelOrRecoverExecution_LegacyCanceledRowBeforeContractMigration(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + t.Cleanup(cancel) + container := testhelpers.RequireEmptyDatabase(ctx, t) + pool := container.DB.Pool() + path := getMigrationsPath() + before := versionBeforeCanceledContract(t, path) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, before)) + store := config.NewPostgresStore(container.DB) + + var execID string + require.NoError(t, pool.QueryRow(ctx, ` + INSERT INTO purchase_executions (status, scheduled_date, `+"cancel"+`led_by) + VALUES ($1, NOW(), 'legacy@example.com') RETURNING execution_id::text`, + config.LegacyStatusCanceled).Scan(&execID)) + + h := &Handler{config: store} + got, err := h.cancelOrRecoverExecution(ctx, execID, nil) + require.NoError(t, err, "a legacy canceled row is an already-won cancel race, not a 409") + require.Equal(t, config.StatusCanceled, got.Status, "the idempotent path returns the canonical spelling") + require.NotNil(t, got.CancelledBy) + require.Equal(t, "legacy@example.com", *got.CancelledBy) +} diff --git a/internal/api/handler_purchases_submit_dedupe_integration_test.go b/internal/api/handler_purchases_submit_dedupe_integration_test.go index 04e92181b..49499b27b 100644 --- a/internal/api/handler_purchases_submit_dedupe_integration_test.go +++ b/internal/api/handler_purchases_submit_dedupe_integration_test.go @@ -119,7 +119,7 @@ func TestPersistExecution_RetryAfterRowLeftPending(t *testing.T) { }{ {"pending", true}, {"notified", true}, {"approved", true}, {"running", true}, {"completed", true}, {"partially_completed", true}, - {config.StatusFailed, false}, {config.StatusCanceled, false}, {config.LegacyStatusCanceled, false}, {config.StatusExpired, false}, + {config.StatusFailed, false}, {config.StatusCanceled, false}, {config.StatusExpired, false}, } { t.Run(tc.status, func(t *testing.T) { recs, key := submitFixture(t) diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 7401b9def..93d9d114c 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -289,11 +289,10 @@ type StoreInterface interface { // (and writes nothing) when the row has left those statuses, so a cancel or // approve that lands first is never overwritten. RescheduleAwaitingExecution(ctx context.Context, executionID string, scheduledDate time.Time) (bool, error) - // SetCancelledBy stamps canceled_by and the legacy cancelled_by column on - // an execution without overwriting any other columns. Used after - // TransitionExecutionStatus to fold the actor attribution into the same - // logical write without the full-row SavePurchaseExecution clobber risk - // (Finding #5 / PR #889). + // SetCancelledBy stamps canceled_by on an execution without overwriting + // any other columns. Used after TransitionExecutionStatus to fold the actor + // attribution into the same logical write without the full-row + // SavePurchaseExecution clobber risk (Finding #5 / PR #889). SetCancelledBy(ctx context.Context, executionID string, cancelledBy string) error // LinkRetryExecutionAtomic stamps retry_execution_id on a failed // execution row without a full-row overwrite, conditional on diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 7195d312d..acea5efb5 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1169,12 +1169,16 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, capacityPercent = 100 } + // canceled_by keeps an actor already on the row: a save from a copy read + // before a concurrent cancel must not erase that cancel's attribution. Reads + // take COALESCE(canceled_by, cancelled_by), so a legacy actor on cancelled_by + // is carried into canceled_by here. This save no longer writes cancelled_by. query := ` INSERT INTO purchase_executions ( plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token_hash, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + cloud_account_id, source, approved_by, canceled_by, capacity_percent, created_by_user_id, retry_execution_id, retry_attempt_n, approval_token_expires_at, executed_by_user_id, executed_at, pre_approval_skip_reason, @@ -1193,7 +1197,7 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, cloud_account_id = $14, source = $15, approved_by = $16, - cancelled_by = $17, + canceled_by = COALESCE(purchase_executions.canceled_by, $17), capacity_percent = $18, approval_token_expires_at = $22, executed_by_user_id = $23, @@ -1334,8 +1338,8 @@ func (s *PostgresStore) RotatePendingApprovalToken(ctx context.Context, executio // actor is the UUID of the user performing the transition (nil for system-initiated paths); // it is stamped onto transitioned_by and transitioned_at is always set to NOW(). // A transition to StatusCanceled also stamps the actor's email onto canceled_by -// and the legacy cancelled_by (issue #119), matching what the other cancel paths -// record and what History reads. +// (issue #119), matching what the other cancel paths record and what History +// reads. func (s *PostgresStore) TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string) (*PurchaseExecution, error) { return s.transitionExecutionStatus(ctx, s.db, executionID, fromStatuses, toStatus, actor, nil, nil) } @@ -1414,8 +1418,7 @@ func (s *PostgresStore) transitionExecutionStatus(ctx context.Context, q executi UPDATE purchase_executions SET status = $2, updated_at = NOW(), transitioned_by = $4, transitioned_at = NOW(), - canceled_by = COALESCE((SELECT email FROM users WHERE id = $5), canceled_by), - cancelled_by = COALESCE((SELECT email FROM users WHERE id = $5), cancelled_by) + canceled_by = COALESCE((SELECT email FROM users WHERE id = $5), canceled_by) WHERE execution_id = $1 AND status = ANY($3) AND ($6::boolean IS NULL OR (recommendations IN ('[]'::jsonb, 'null'::jsonb)) = $6) AND ($7::text IS NULL OR (approval_token_hash = $7 @@ -1472,16 +1475,14 @@ func transitionRejection(ctx context.Context, q executionQuerier, executionID st return fmt.Errorf("%w: execution %s cannot transition from %q to %q", ErrExecutionNotInExpectedStatus, executionID, existing.Status, toStatus) } -// SetCancelledBy stamps both the canceled_by and legacy cancelled_by columns -// for a single execution without touching any other column. This avoids the -// full-row overwrite that a SavePurchaseExecution follow-up would perform, -// eliminating the lost-update window between TransitionExecutionStatus and -// the attribution write (Finding #5). Writing both columns keeps this path -// symmetric with CancelExecutionAtomic/CancelScheduledExecutionAtomic (which -// write canceled_by only) so revoke-attribution keeps working unchanged if a -// future contract migration (#1278) drops the legacy column. +// SetCancelledBy stamps canceled_by for a single execution without touching +// any other column. This avoids the full-row overwrite that a +// SavePurchaseExecution follow-up would perform, eliminating the lost-update +// window between TransitionExecutionStatus and the attribution write +// (Finding #5). It no longer writes the legacy cancelled_by column; reads take +// COALESCE(canceled_by, cancelled_by). func (s *PostgresStore) SetCancelledBy(ctx context.Context, executionID, cancelledBy string) error { - q := `UPDATE purchase_executions SET canceled_by = $2, cancelled_by = $2, updated_at = NOW() + q := `UPDATE purchase_executions SET canceled_by = $2, updated_at = NOW() WHERE execution_id = $1` _, err := s.db.Exec(ctx, q, executionID, cancelledBy) return err diff --git a/internal/config/store_postgres_cancel_actor_pgxmock_test.go b/internal/config/store_postgres_cancel_actor_pgxmock_test.go new file mode 100644 index 000000000..5d3725c39 --- /dev/null +++ b/internal/config/store_postgres_cancel_actor_pgxmock_test.go @@ -0,0 +1,147 @@ +package config + +import ( + "context" + "errors" + "strings" + "sync" + "testing" + "time" + + "github.com/pashagolub/pgxmock/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// legacyCancelActorColumn is the pre-migration-000089 column, split so the +// US-locale misspell linter does not flag it; it is the value under test. +const legacyCancelActorColumn = "cancel" + "led_by" + +const coalescedActorProjection = "COALESCE(canceled_by, " + legacyCancelActorColumn + ") AS " + legacyCancelActorColumn + +var executionProjectionCols = []string{ + "plan_id", "execution_id", "status", "step_number", "scheduled_date", + "notification_sent", "approval_token", "recommendations", + "total_upfront_cost", "estimated_savings", "completed_at", "error", "expires_at", + "cloud_account_id", "source", "approved_by", legacyCancelActorColumn, "capacity_percent", + "created_by_user_id", "retry_execution_id", "retry_attempt_n", + "approval_token_expires_at", + "executed_by_user_id", "executed_at", "pre_approval_skip_reason", + "idempotency_key", "scheduled_execution_at", "no_submission", "created_by_system", +} + +// TestPGXMock_ExecutionProjectionsReadCoalescedActor pins the read half of the +// 000089 contract while old builds can still write the legacy column: every +// statement that projects a purchase_executions row must read +// COALESCE(canceled_by, ), so an actor stored in either column +// scans back. A bare canceled_by projection loses the attribution of legacy +// rows (written to the legacy column only) until the follow-up drain runs. +func TestPGXMock_ExecutionProjectionsReadCoalescedActor(t *testing.T) { + var mu sync.Mutex + var projections, bare []string + mock, err := pgxmock.NewPool(pgxmock.QueryMatcherOption(pgxmock.QueryMatcherFunc(func(_, actual string) error { + if !strings.Contains(actual, "capacity_percent") { + return nil + } + mu.Lock() + defer mu.Unlock() + projections = append(projections, actual) + if !strings.Contains(actual, coalescedActorProjection) { + bare = append(bare, actual) + } + return nil + }))) + require.NoError(t, err) + mock.MatchExpectationsInOrder(false) + store := storeWith(mock) + ctx := context.Background() + empty := func() *pgxmock.Rows { return pgxmock.NewRows(executionProjectionCols) } + // Two rows of expectations per call: a zero-row transition is followed by + // a probe SELECT. + expect := func() { + mock.ExpectQuery(".").WillReturnRows(empty()) + mock.ExpectQuery(".").WillReturnRows(empty()).Maybe() + } + + expect() + _, _ = store.GetExecutionsByStatusesFiltered(ctx, ExecutionsByStatusesQuery{Statuses: []string{"pending"}}) + expect() + _, _ = store.GetPlannedExecutions(ctx, []string{"scheduled"}, 10) + expect() + _, _ = store.GetStaleApprovedExecutions(ctx, time.Hour) + expect() + _, _ = store.ListStuckExecutions(ctx, []string{"approved"}, time.Hour) + expect() + _, _ = store.GetPendingExecutions(ctx) + expect() + _, _ = store.GetExecutionByID(ctx, "exec-1") + expect() + _, _ = store.GetExecutionByPlanAndDate(ctx, "plan-1", time.Now()) + expect() + _, _ = store.GetScheduledExecutionsDue(ctx) + actor := "11111111-1111-1111-1111-111111111111" + expect() + _, _ = store.TransitionExecutionStatus(ctx, "exec-1", []string{"pending"}, StatusCanceled, &actor) + mock.ExpectBegin() + mock.ExpectExec(".").WillReturnResult(pgxmock.NewResult("SELECT", 1)) + expect() + tx, err := mock.Begin(ctx) + require.NoError(t, err) + _, _ = store.ListRecentSubmitsTx(ctx, tx, "creator", time.Now()) + + mu.Lock() + defer mu.Unlock() + assert.GreaterOrEqual(t, len(projections), 10, "every purchase_executions projection must be exercised") + assert.Empty(t, bare, "projections that do not read COALESCE(canceled_by, legacy actor)") +} + +// TestPGXMock_ExecutionCancelActorWritesOnlyCanceledBy pins the write half: +// the save, the cancel transition and SetCancelledBy write canceled_by alone +// and never the legacy column (RETURNING projections still read it). The +// matcher rejects any INSERT or UPDATE statement whose write part names the legacy +// column, so each call fails against the dual-write queries. +// +// The save keeps an actor already on the row (COALESCE(existing, $17)): a save +// from a copy read before a concurrent cancel must not erase that actor, and a +// legacy-only actor read through COALESCE is carried into canceled_by. +func TestPGXMock_ExecutionCancelActorWritesOnlyCanceledBy(t *testing.T) { + var dualWrites []string + mock, err := pgxmock.NewPool(pgxmock.QueryMatcherOption(pgxmock.QueryMatcherFunc(func(expected, actual string) error { + write, _, _ := strings.Cut(actual, "RETURNING") + verb := strings.ToUpper(strings.Fields(write + " x")[0]) + if (verb == "INSERT" || verb == "UPDATE") && strings.Contains(write, legacyCancelActorColumn) { + dualWrites = append(dualWrites, actual) + return errors.New("write still names the legacy cancel actor column: " + actual) + } + return pgxmock.QueryMatcherRegexp.Match(expected, actual) + }))) + require.NoError(t, err) + store := storeWith(mock) + ctx := context.Background() + + actor := "11111111-1111-1111-1111-111111111111" + mock.ExpectQuery(`(?s)SET status = \$2.*canceled_by = COALESCE\(\(SELECT email FROM users WHERE id = \$5\), canceled_by\)\s+WHERE`). + WithArgs(anyArgsCfg(7)...). + WillReturnRows(pgxmock.NewRows(executionProjectionCols)) + // The empty result row makes the store probe the row afterwards; that + // probe and its error are not under test. + mock.ExpectQuery(`FROM purchase_executions`).WillReturnRows(pgxmock.NewRows(executionProjectionCols)).Maybe() + _, _ = store.TransitionExecutionStatus(ctx, "exec-1", []string{"pending"}, StatusCanceled, &actor) + require.Empty(t, dualWrites, "the cancel transition must write canceled_by only") + + mock.ExpectExec(`SET canceled_by = \$2, updated_at = NOW\(\)`). + WithArgs("exec-1", "revoker@example.com"). + WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + require.NoError(t, store.SetCancelledBy(ctx, "exec-1", "revoker@example.com")) + + mock.ExpectBegin() + mock.ExpectExec(`(?s)approved_by, canceled_by, capacity_percent.*canceled_by = COALESCE\(purchase_executions\.canceled_by, \$17\)`). + WithArgs(anyArgsCfg(30)...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + require.NoError(t, store.SavePurchaseExecution(ctx, &PurchaseExecution{ + ExecutionID: "exec-1", PlanID: "plan-1", Status: StatusCanceled, ScheduledDate: time.Now(), + })) + + assert.NoError(t, mock.ExpectationsWereMet()) +} diff --git a/internal/config/store_postgres_db_test.go b/internal/config/store_postgres_db_test.go index 36f906411..b2036d604 100644 --- a/internal/config/store_postgres_db_test.go +++ b/internal/config/store_postgres_db_test.go @@ -1288,17 +1288,6 @@ func TestPostgresStoreDB_CleanupOldExecutions_RetainsCanceledInsideHealthWindow( shouldSurvive: true, why: "expires_at branch must not purge what the canceled branch retains", }, - { - // Deliberately the LegacyStatusCanceled constant, i.e. the - // pre-migration-000089 double-l DB value. The label avoids - // spelling it out only to keep the misspell linter quiet. - name: "legacy-spelling-canceled-today-old-scheduled-date", - status: LegacyStatusCanceled, - scheduledDays: -40, - updatedDays: 0, - shouldSurvive: true, - why: "the legacy spelling is retained on updated_at too", - }, { // The `failed` counterpart of the canceled case above, and the // reason branch 3's exclusion is sourced from diff --git a/internal/config/store_postgres_delete_account_integration_test.go b/internal/config/store_postgres_delete_account_integration_test.go index e39f74001..e87d3d45a 100644 --- a/internal/config/store_postgres_delete_account_integration_test.go +++ b/internal/config/store_postgres_delete_account_integration_test.go @@ -11,6 +11,7 @@ import ( "context" "errors" "regexp" + "slices" "sort" "testing" "time" @@ -76,13 +77,23 @@ func accountExists(ctx context.Context, t *testing.T, store *PostgresStore, id s return n == 1 } +// storableTerminalExecutionStatuses is TerminalExecutionStatuses minus the +// legacy canceled spelling, which migration 000157 forbids in the status CHECK +// on a fully migrated schema. Legacy rows are covered against the schema before +// 000157 in TestPostgresStore_NewCodeAgainstPreContractSchema. +func storableTerminalExecutionStatuses() []string { + return slices.DeleteFunc(slices.Clone(TerminalExecutionStatuses), func(s string) bool { + return s == LegacyStatusCanceled + }) +} + // An account whose executions are all terminal is deletable, and the history // rows survive with their status intact and the account reference detached. func TestDeleteCloudAccount_TerminalExecutionsAreDetached(t *testing.T) { ctx := context.Background() store := setupLadderStore(ctx, t) - for _, status := range TerminalExecutionStatuses { + for _, status := range storableTerminalExecutionStatuses() { t.Run(status, func(t *testing.T) { acct := seedDeleteAccount(ctx, t, store) execID := seedDeleteAccountExec(ctx, t, store, acct, status) @@ -158,7 +169,7 @@ func TestCountLiveExecutionsForAccount(t *testing.T) { for _, status := range liveExecutionStatuses { seedDeleteAccountExec(ctx, t, store, acct, status) } - for _, status := range TerminalExecutionStatuses { + for _, status := range storableTerminalExecutionStatuses() { seedDeleteAccountExec(ctx, t, store, acct, status) } seedDeleteAccountExec(ctx, t, store, other, "running") @@ -191,7 +202,7 @@ func TestTerminalExecutionStatuses_CoverSchema(t *testing.T) { } require.NotEmpty(t, schema, "could not parse %q", def) - classified := append(append([]string{}, TerminalExecutionStatuses...), liveExecutionStatuses...) + classified := append(append([]string{}, storableTerminalExecutionStatuses()...), liveExecutionStatuses...) sort.Strings(schema) sort.Strings(classified) assert.Equal(t, schema, classified) diff --git a/internal/config/store_postgres_history_summary_integration_test.go b/internal/config/store_postgres_history_summary_integration_test.go index 19cdee86d..b4754760f 100644 --- a/internal/config/store_postgres_history_summary_integration_test.go +++ b/internal/config/store_postgres_history_summary_integration_test.go @@ -88,7 +88,7 @@ func (f *historySummaryFixture) seed(ctx context.Context, t *testing.T) { recA := []RecommendationRecord{{Provider: "aws", CloudAccountID: &a}} mixed := []RecommendationRecord{{Provider: "aws", CloudAccountID: &a}, {Provider: "azure", CloudAccountID: &b}} statuses := []string{"pending", "notified", "scheduled", "approved", "running", "paused", "failed", "expired", - StatusCanceled, LegacyStatusCanceled, "partially_completed"} + StatusCanceled, "partially_completed"} for i, st := range statuses { at := f.base.Add(time.Duration(i) * 36 * time.Hour) f.saveExecution(ctx, t, at, st, "", &a, nil, recA) @@ -197,7 +197,7 @@ func TestIntegration_HistorySummaryAggregate_MatchesRowFetch(t *testing.T) { } actionableStatuses := []string{"pending", "notified", "scheduled", "approved", "running", "paused"} statuses := []string{"pending", "notified", "scheduled", "approved", "running", "paused", - "completed", "partially_completed", "failed", "expired", StatusCanceled, LegacyStatusCanceled} + "completed", "partially_completed", "failed", "expired", StatusCanceled} for name, filter := range filters { t.Run(name, func(t *testing.T) { agg, err := f.store.HistorySummaryAggregate(ctx, HistorySummaryFilter{ diff --git a/internal/config/store_postgres_ladder_resolve_integration_test.go b/internal/config/store_postgres_ladder_resolve_integration_test.go index c1cf301bf..78fb16904 100644 --- a/internal/config/store_postgres_ladder_resolve_integration_test.go +++ b/internal/config/store_postgres_ladder_resolve_integration_test.go @@ -91,7 +91,6 @@ func TestResolveLadderTrancheFailed_ProvenChain(t *testing.T) { for name, nodes := range map[string][]resolveNode{ "no_submission head": {{"failed", true, time.Minute}}, "canceled after proof": {{"failed", true, time.Hour}, {"canceled", false, time.Minute}}, - "legacy spelling": {{LegacyStatusCanceled, false, time.Minute}}, "expired": {{"expired", false, time.Minute}}, } { t.Run(name, func(t *testing.T) { diff --git a/internal/config/store_postgres_legacy_canceled_integration_test.go b/internal/config/store_postgres_legacy_canceled_integration_test.go new file mode 100644 index 000000000..59e430d1b --- /dev/null +++ b/internal/config/store_postgres_legacy_canceled_integration_test.go @@ -0,0 +1,182 @@ +//go:build integration + +package config + +import ( + "context" + "os" + "strconv" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" +) + +// legacyActorColumn is the pre-migration-000089 column, split so the +// US-locale misspell linter does not flag it. +const legacyActorColumn = "cancel" + "led_by" + +// versionBeforeCanceledContract is the schema a deployment is at when the +// build under test starts: the highest migration below the canceled contract +// (the file ending _contract_canceled_spelling.up.sql), which narrows the +// status CHECKs. The cutoff is read from that file, so a renumber needs no edit +// here. The new build must read and write correctly there, because the +// migration and the release roll out in either order. +func versionBeforeCanceledContract(t *testing.T) uint { + t.Helper() + return versionBelowCanceledContract(t, getTestMigrationsPath()) +} + +func versionBelowCanceledContract(t *testing.T, dir string) uint { + t.Helper() + entries, err := os.ReadDir(dir) + require.NoError(t, err) + prefix := func(name string) (uint, bool) { + v, err := strconv.ParseUint(strings.SplitN(name, "_", 2)[0], 10, 32) + return uint(v), err == nil + } + var contract uint + for _, e := range entries { + if v, ok := prefix(e.Name()); ok && strings.HasSuffix(e.Name(), "_contract_canceled_spelling.up.sql") { + contract = v + } + } + require.NotZero(t, contract, "no *_contract_canceled_spelling.up.sql migration in %s", dir) + var best uint + for _, e := range entries { + if v, ok := prefix(e.Name()); ok && v < contract && v > best { + best = v + } + } + require.NotZero(t, best, "no migration below %d", contract) + return best +} + +// TestPostgresStore_NewCodeAgainstPreContractSchema runs the new store code on +// the schema before 000157 with rows an older build wrote: the legacy status +// spelling and an actor only in the legacy column. Every reader must treat them as +// canceled with their actor, and the new writers must not lose them. +func TestPostgresStore_NewCodeAgainstPreContractSchema(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + container := testhelpers.RequireEmptyDatabase(ctx, t) + pool := container.DB.Pool() + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getTestMigrationsPath(), versionBeforeCanceledContract(t))) + store := NewPostgresStore(container.DB) + + acct := &CloudAccount{Name: "Legacy Cancel", Provider: "aws", ExternalID: "ext-legacy-cancel", Enabled: true} + require.NoError(t, store.CreateCloudAccount(ctx, acct)) + plan := &PurchasePlan{ + Name: "Legacy Cancel Plan", Services: map[string]ServiceConfig{}, + RampSchedule: RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, CurrentStep: 2, TotalSteps: 4}, + } + require.NoError(t, store.CreatePurchasePlan(ctx, plan)) + + legacyRow := func(updatedAt time.Time, step int, actor string) string { + t.Helper() + var id string + require.NoError(t, pool.QueryRow(ctx, ` + INSERT INTO purchase_executions (plan_id, status, step_number, scheduled_date, cloud_account_id, `+legacyActorColumn+`, updated_at) + VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING execution_id::text`, + plan.ID, LegacyStatusCanceled, step, updatedAt, acct.ID, actor, updatedAt).Scan(&id)) + return id + } + rawActors := func(id string) (canceledBy, legacyBy *string) { + t.Helper() + require.NoError(t, pool.QueryRow(ctx, + `SELECT canceled_by, `+legacyActorColumn+` FROM purchase_executions WHERE execution_id = $1`, id).Scan(&canceledBy, &legacyBy)) + return canceledBy, legacyBy + } + now := time.Now().UTC().Truncate(time.Second) + legacy := legacyRow(now, 3, "legacy@example.com") + + t.Run("get reads the actor through COALESCE", func(t *testing.T) { + got, err := store.GetExecutionByID(ctx, legacy) + require.NoError(t, err) + assert.Equal(t, LegacyStatusCanceled, got.Status) + require.NotNil(t, got.CancelledBy, "a legacy-column-only actor must scan back") + assert.Equal(t, "legacy@example.com", *got.CancelledBy) + }) + + t.Run("history list and summary include the legacy status", func(t *testing.T) { + statuses := []string{StatusCanceled, LegacyStatusCanceled} + page, err := store.ListHistoryExecutionsPage(ctx, HistoryExecutionsPageQuery{Statuses: statuses, Limit: 10}) + require.NoError(t, err) + require.Len(t, page, 1) + require.NotNil(t, page[0].CancelledBy) + assert.Equal(t, "legacy@example.com", *page[0].CancelledBy) + + groups, err := store.HistorySummaryAggregate(ctx, HistorySummaryFilter{ExecutionStatuses: statuses}) + require.NoError(t, err) + require.Len(t, groups.Executions, 1) + assert.Equal(t, LegacyStatusCanceled, groups.Executions[0].Status) + assert.Equal(t, 1, groups.Executions[0].Count) + }) + + t.Run("a legacy row is terminal and holds no capacity or ramp step", func(t *testing.T) { + live, err := store.CountLiveExecutionsForAccount(ctx, acct.ID) + require.NoError(t, err) + assert.Zero(t, live, "a legacy canceled row must not block deleting the account") + + assert.False(t, holdsCapacity(&PurchaseExecution{Status: LegacyStatusCanceled})) + + tx, err := pool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = tx.Rollback(ctx) }() + occupied, err := store.OccupiedRampStepsInRangeTx(ctx, tx, plan.ID, 3, 3) + require.NoError(t, err) + assert.Empty(t, occupied, "a step whose only unit is legacy canceled settled without buying") + }) + + t.Run("save keeps a legacy actor readable and never erases an actor", func(t *testing.T) { + got, err := store.GetExecutionByID(ctx, legacy) + require.NoError(t, err) + got.Status = StatusCanceled + require.NoError(t, store.SavePurchaseExecution(ctx, got)) + canceledBy, legacyBy := rawActors(legacy) + require.NotNil(t, canceledBy, "the save carries the legacy actor into canceled_by") + assert.Equal(t, "legacy@example.com", *canceledBy) + require.NotNil(t, legacyBy, "the legacy column is left as the older build wrote it") + assert.Equal(t, "legacy@example.com", *legacyBy) + reread, err := store.GetExecutionByID(ctx, legacy) + require.NoError(t, err) + require.NotNil(t, reread.CancelledBy) + assert.Equal(t, "legacy@example.com", *reread.CancelledBy) + + // A save from a copy read before a concurrent cancel stamped the actor. + stale := *got + stale.CancelledBy = nil + _, err = pool.Exec(ctx, `UPDATE purchase_executions SET canceled_by = 'concurrent@example.com' WHERE execution_id = $1`, legacy) + require.NoError(t, err) + require.NoError(t, store.SavePurchaseExecution(ctx, &stale)) + canceledBy, _ = rawActors(legacy) + require.NotNil(t, canceledBy) + assert.Equal(t, "concurrent@example.com", *canceledBy, "a stale save must not erase the actor") + + // A stale copy that carries a DIFFERENT actor must not replace the + // stored one either: the stored actor wins over the saved $17. + other := *got + otherActor := "stale-copy@example.com" + other.CancelledBy = &otherActor + require.NoError(t, store.SavePurchaseExecution(ctx, &other)) + canceledBy, _ = rawActors(legacy) + require.NotNil(t, canceledBy) + assert.Equal(t, "concurrent@example.com", *canceledBy, "a stale save with another actor must not replace the stored actor") + }) + + t.Run("cleanup removes a legacy row after retention", func(t *testing.T) { + old := legacyRow(now.AddDate(0, 0, -31), 1, "old@example.com") + deleted, err := store.CleanupOldExecutions(ctx, 30) + require.NoError(t, err) + assert.EqualValues(t, 1, deleted) + _, err = store.GetExecutionByID(ctx, old) + assert.ErrorIs(t, err, ErrNotFound) + _, err = store.GetExecutionByID(ctx, legacy) + assert.NoError(t, err, "a fresh legacy row survives") + }) +} diff --git a/internal/config/store_postgres_ramp_baseline_integration_test.go b/internal/config/store_postgres_ramp_baseline_integration_test.go index f37fcd1f9..00747bcee 100644 --- a/internal/config/store_postgres_ramp_baseline_integration_test.go +++ b/internal/config/store_postgres_ramp_baseline_integration_test.go @@ -71,9 +71,8 @@ func TestListPlanCommittedTx_RealRowShapes(t *testing.T) { require.NoError(t, f.store.SavePurchaseExecution(ctx, retry)) f.rowWithRecs(ctx, t, 2, a2, StatusCompleted, false, r2) - // Step 3: canceled, legacy-canceled and a failed row proven unsubmitted hold nothing. + // Step 3: canceled and a failed row proven unsubmitted hold nothing. f.rowWithRecs(ctx, t, 3, a1, StatusCanceled, false, r1) - f.rowWithRecs(ctx, t, 3, a1, LegacyStatusCanceled, false, r1) f.rowWithRecs(ctx, t, 3, a2, StatusFailed, true, r2) got := f.committed(ctx, t) diff --git a/internal/config/store_postgres_ramp_step_integration_test.go b/internal/config/store_postgres_ramp_step_integration_test.go index 1e18b71c0..00ff0b021 100644 --- a/internal/config/store_postgres_ramp_step_integration_test.go +++ b/internal/config/store_postgres_ramp_step_integration_test.go @@ -544,7 +544,7 @@ func TestRampStepStatusListsPartitionEveryWrittenStatus(t *testing.T) { written := []string{ "pending", "notified", "scheduled", "approved", "running", "paused", StatusCompleted, StatusPartiallyCompleted, StatusFailed, StatusExpired, - StatusCanceled, LegacyStatusCanceled, + StatusCanceled, } // In-flight statuses hold a step open on purpose and are deliberately not // reported as stuck; they are listed here so adding a status to the schema diff --git a/internal/config/store_postgres_retention_integration_test.go b/internal/config/store_postgres_retention_integration_test.go index 99922d956..a6eaa5e91 100644 --- a/internal/config/store_postgres_retention_integration_test.go +++ b/internal/config/store_postgres_retention_integration_test.go @@ -30,7 +30,6 @@ func TestRampRetention_OldCleanupWritersPreserveHeldRows(t *testing.T) { {StatusFailed, false, `[]`}, {StatusFailed, true, `[]`}, {StatusCanceled, false, `[]`}, - {LegacyStatusCanceled, false, `[]`}, {StatusExpired, false, `[]`}, } for _, tc := range cases { @@ -48,7 +47,7 @@ func TestRampRetention_OldCleanupWritersPreserveHeldRows(t *testing.T) { require.Equal(t, holdsCapacity(&PurchaseExecution{Status: "future-status"}), future) deleted, err := f.store.CleanupOldExecutions(ctx, 30) require.NoError(t, err) - require.EqualValues(t, 3, deleted, "only canceled spellings and expired are eligible and unheld") + require.EqualValues(t, 2, deleted, "only canceled and expired are eligible and unheld") // A raw expires_at DELETE, the shape the deleted cmd/cleanup-lambda ran // (#357) and an operator could still run by hand: the ramp ledger trigger, // not the caller's predicate, must keep held rows. diff --git a/internal/database/postgres/migrations/000157_contract_canceled_spelling.down.sql b/internal/database/postgres/migrations/000157_contract_canceled_spelling.down.sql new file mode 100644 index 000000000..058ce0cc8 --- /dev/null +++ b/internal/database/postgres/migrations/000157_contract_canceled_spelling.down.sql @@ -0,0 +1,20 @@ +-- Rollback 000157: re-widen both status CHECKs to the 000089 EXPAND state. +-- +-- Rows stay 'canceled': the widened CHECKs accept both spellings and every +-- build since #1277 reads both, so converting them back is unnecessary. + +LOCK TABLE purchase_executions IN ACCESS EXCLUSIVE MODE; +LOCK TABLE ri_exchange_history IN ACCESS EXCLUSIVE MODE; + +ALTER TABLE purchase_executions DROP CONSTRAINT IF EXISTS purchase_executions_status_check; +ALTER TABLE purchase_executions ADD CONSTRAINT purchase_executions_status_check + CHECK (status IN ( + 'pending','notified','approved','running','completed', + 'partially_completed','failed', + 'cancelled','canceled', + 'expired','paused','revocation_requested','scheduled' + )); + +ALTER TABLE ri_exchange_history DROP CONSTRAINT IF EXISTS ri_exchange_history_status_check; +ALTER TABLE ri_exchange_history ADD CONSTRAINT ri_exchange_history_status_check + CHECK (status IN ('pending', 'processing', 'completed', 'failed', 'cancelled', 'canceled')); diff --git a/internal/database/postgres/migrations/000157_contract_canceled_spelling.up.sql b/internal/database/postgres/migrations/000157_contract_canceled_spelling.up.sql new file mode 100644 index 000000000..f75960c50 --- /dev/null +++ b/internal/database/postgres/migrations/000157_contract_canceled_spelling.up.sql @@ -0,0 +1,55 @@ +-- Migration 000157: CONTRACT half of the 000089 'cancelled' -> 'canceled' +-- rename (issue #47), part 1 of 2. +-- +-- 000089 (EXPAND) widened both status CHECKs to accept both spellings, added +-- canceled_by next to the legacy cancelled_by, and left normalization to this +-- step. Every build since #1277 writes only 'canceled', so the legacy set of +-- status rows is final and can be normalized and forbidden here. +-- +-- This release still reads both spellings of the status and both actor +-- columns (COALESCE(canceled_by, cancelled_by)). It stops writing the legacy +-- actor column, but older builds that still write it keep working, so this +-- migration neither drains cancelled_by nor mirrors it. The drain, the strict +-- reads and the cancelled_by drop belong to a follow-up, shipped once this +-- migration has run everywhere and no build older than this release can be +-- rolled back to. +-- +-- Concurrency: both tables are locked up front, so no writer can commit a row +-- between the normalize UPDATEs and the narrowed CHECKs (which would fail the +-- migration). golang-migrate runs this file in one transaction. +-- +-- updated_at is the plan-health and retention clock. The row triggers stamp it +-- on every UPDATE, so they are disabled around the normalization: a pure +-- spelling change must not make old rows look fresh. DISABLE TRIGGER is +-- transactional; if any step fails, the whole file rolls back with the +-- triggers still enabled. + +LOCK TABLE purchase_executions IN ACCESS EXCLUSIVE MODE; +LOCK TABLE ri_exchange_history IN ACCESS EXCLUSIVE MODE; + +ALTER TABLE purchase_executions DISABLE TRIGGER update_purchase_executions_updated_at; +ALTER TABLE ri_exchange_history DISABLE TRIGGER ri_exchange_updated_at; + +-- Normalize the data BEFORE narrowing, or the narrowed CHECKs reject the +-- surviving legacy rows. +UPDATE purchase_executions SET status = 'canceled' WHERE status = 'cancelled'; +UPDATE ri_exchange_history SET status = 'canceled' WHERE status = 'cancelled'; + +ALTER TABLE purchase_executions ENABLE TRIGGER update_purchase_executions_updated_at; +ALTER TABLE ri_exchange_history ENABLE TRIGGER ri_exchange_updated_at; + +-- Narrow purchase_executions: the 000089 list without 'cancelled'. +ALTER TABLE purchase_executions DROP CONSTRAINT IF EXISTS valid_status; +ALTER TABLE purchase_executions DROP CONSTRAINT IF EXISTS purchase_executions_status_check; +ALTER TABLE purchase_executions ADD CONSTRAINT purchase_executions_status_check + CHECK (status IN ( + 'pending','notified','approved','running','completed', + 'partially_completed','failed', + 'canceled', + 'expired','paused','revocation_requested','scheduled' + )); + +-- Narrow ri_exchange_history: the 000089 list without 'cancelled'. +ALTER TABLE ri_exchange_history DROP CONSTRAINT IF EXISTS ri_exchange_history_status_check; +ALTER TABLE ri_exchange_history ADD CONSTRAINT ri_exchange_history_status_check + CHECK (status IN ('pending', 'processing', 'completed', 'failed', 'canceled')); diff --git a/internal/database/postgres/migrations/000157_contract_canceled_spelling_test.go b/internal/database/postgres/migrations/000157_contract_canceled_spelling_test.go new file mode 100644 index 000000000..c270c273f --- /dev/null +++ b/internal/database/postgres/migrations/000157_contract_canceled_spelling_test.go @@ -0,0 +1,277 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "regexp" + "testing" + "time" + + "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" +) + +// The legacy spellings are built by concatenation so the US-locale misspell +// linter does not flag them; they are the values under test. +const ( + contract157 = 157 + legacyStatus157 = "cancel" + "led" + legacyActor157 = "cancel" + "led_by" + canceled157 = "canceled" +) + +// legacyInCheck157 matches the legacy spelling's entry in a status CHECK +// definition as pg_get_constraintdef renders it (the cast follows the column +// type: text or character varying). +var legacyInCheck157 = regexp.MustCompile(`'` + legacyStatus157 + `'::[a-z ]+, `) + +const insertExecution157 = ` + INSERT INTO purchase_executions (status, scheduled_date, ` + legacyActor157 + `, updated_at) + VALUES ($1, NOW(), $2, $3) RETURNING execution_id::text` + +func seedExecution157(ctx context.Context, t *testing.T, pool *pgxpool.Pool, status string, legacyBy *string, updatedAt time.Time) string { + t.Helper() + var id string + require.NoError(t, pool.QueryRow(ctx, insertExecution157, status, legacyBy, updatedAt).Scan(&id)) + return id +} + +func insertExchange157(ctx context.Context, pool *pgxpool.Pool, status string, updatedAt time.Time) (string, error) { + var id string + err := pool.QueryRow(ctx, ` + INSERT INTO ri_exchange_history + (account_id, region, source_ri_ids, source_instance_type, source_count, + target_offering_id, target_instance_type, target_count, status, updated_at) + VALUES ('123456789012', 'us-east-1', ARRAY['ri-1'], 'm5.large', 1, + 'offer-1', 'm5.xlarge', 1, $1, $2) + RETURNING id::text`, status, updatedAt).Scan(&id) + return id, err +} + +func statusCheckDef157(ctx context.Context, t *testing.T, pool *pgxpool.Pool, constraint string) string { + t.Helper() + var def string + require.NoError(t, pool.QueryRow(ctx, + `SELECT pg_get_constraintdef(oid) FROM pg_constraint WHERE conname = $1`, constraint).Scan(&def)) + return def +} + +func updatedAt157(ctx context.Context, t *testing.T, pool *pgxpool.Pool, table, id string) time.Time { + t.Helper() + idCol := map[string]string{"purchase_executions": "execution_id", "ri_exchange_history": "id"}[table] + var ts time.Time + require.NoError(t, pool.QueryRow(ctx, + `SELECT updated_at FROM `+table+` WHERE `+idCol+` = $1`, id).Scan(&ts)) + return ts +} + +// readActor157 reads the actor the way every projection does. +func readActor157(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id string) (status string, actor *string) { + t.Helper() + require.NoError(t, pool.QueryRow(ctx, + `SELECT status, COALESCE(canceled_by, `+legacyActor157+`) FROM purchase_executions WHERE execution_id = $1`, id, + ).Scan(&status, &actor)) + return status, actor +} + +func requireCheckViolation157(t *testing.T, err error, constraint string) { + t.Helper() + var pgErr *pgconn.PgError + require.ErrorAs(t, err, &pgErr) + assert.Equal(t, "23514", pgErr.Code) + assert.Equal(t, constraint, pgErr.ConstraintName) +} + +func s157(v string) *string { return &v } + +// TestMigration_000157_ContractCanceledSpelling locks down the contract half of +// the 000089 rename (issue #47): legacy status rows are normalized without +// restamping updated_at (the plan-health and retention clock), both status +// CHECKs lose exactly the legacy spelling, the down restores the widened +// CHECKs exactly, and the write statements of the build before this release +// (which still write the legacy actor column) keep working against the +// narrowed schema. +func TestMigration_000157_ContractCanceledSpelling(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + prev := previousMigrationVersion(t, contract157) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + peBefore := statusCheckDef157(ctx, t, pool, "purchase_executions_status_check") + riBefore := statusCheckDef157(ctx, t, pool, "ri_exchange_history_status_check") + require.Len(t, legacyInCheck157.FindAllString(peBefore, -1), 1, peBefore) + require.Len(t, legacyInCheck157.FindAllString(riBefore, -1), 1, riBefore) + + old := time.Now().UTC().AddDate(0, 0, -90).Truncate(time.Microsecond) + legacyExec := seedExecution157(ctx, t, pool, legacyStatus157, s157("legacy@example.com"), old) + failedExec := seedExecution157(ctx, t, pool, "failed", nil, old) + legacyExchange, err := insertExchange157(ctx, pool, legacyStatus157, old) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, contract157)) + + // Data: the legacy status is normalized and the legacy actor column is + // left alone (this release still reads it); other rows are untouched. + status, actor := readActor157(ctx, t, pool, legacyExec) + assert.Equal(t, canceled157, status) + require.NotNil(t, actor) + assert.Equal(t, "legacy@example.com", *actor) + status, _ = readActor157(ctx, t, pool, failedExec) + assert.Equal(t, "failed", status) + var exchangeStatus string + require.NoError(t, pool.QueryRow(ctx, `SELECT status FROM ri_exchange_history WHERE id = $1`, legacyExchange).Scan(&exchangeStatus)) + assert.Equal(t, canceled157, exchangeStatus) + + // updated_at is the plan-health and retention clock: a spelling change + // must not make 90-day-old rows look fresh. Each table has its own + // updated_at trigger, so each is asserted on its own. + assert.True(t, old.Equal(updatedAt157(ctx, t, pool, "purchase_executions", legacyExec)), + "purchase_executions.updated_at must survive the normalization") + assert.True(t, old.Equal(updatedAt157(ctx, t, pool, "ri_exchange_history", legacyExchange)), + "ri_exchange_history.updated_at must survive the normalization") + var enabled int + require.NoError(t, pool.QueryRow(ctx, ` + SELECT count(*) FROM pg_trigger + WHERE tgname IN ('update_purchase_executions_updated_at', 'ri_exchange_updated_at') + AND tgenabled = 'O'`).Scan(&enabled)) + assert.Equal(t, 2, enabled, "both updated_at triggers must be enabled again") + + // Schema: each CHECK is its pre-migration definition minus the legacy + // spelling, nothing else, and rejects the legacy spelling. + assert.Equal(t, legacyInCheck157.ReplaceAllString(peBefore, ""), + statusCheckDef157(ctx, t, pool, "purchase_executions_status_check")) + assert.Equal(t, legacyInCheck157.ReplaceAllString(riBefore, ""), + statusCheckDef157(ctx, t, pool, "ri_exchange_history_status_check")) + _, err = pool.Exec(ctx, insertExecution157, legacyStatus157, nil, old) + requireCheckViolation157(t, err, "purchase_executions_status_check") + _, err = insertExchange157(ctx, pool, legacyStatus157, old) + requireCheckViolation157(t, err, "ri_exchange_history_status_check") + + requireOldBuildWritesStillWork157(ctx, t, pool) + + // Down: the widened CHECKs come back byte for byte. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + assert.Equal(t, peBefore, statusCheckDef157(ctx, t, pool, "purchase_executions_status_check")) + assert.Equal(t, riBefore, statusCheckDef157(ctx, t, pool, "ri_exchange_history_status_check")) + seedExecution157(ctx, t, pool, legacyStatus157, nil, old) + _, err = insertExchange157(ctx, pool, legacyStatus157, old) + require.NoError(t, err, "down must accept the legacy spelling again") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, contract157), "up must re-apply cleanly after down") +} + +// Write statements copied from origin/main (internal/config/store_postgres.go +// at the time of #1253): the build that is live when this migration lands +// writes the legacy actor column, so none of them may hit the narrowed CHECKs +// and the actor must read back through COALESCE over both columns. +// +//nolint:misspell // the SQL deliberately names the legacy British column under test +const ( + oldSaveExecution157 = ` + INSERT INTO purchase_executions ( + plan_id, execution_id, status, step_number, scheduled_date, + notification_sent, approval_token_hash, recommendations, + total_upfront_cost, estimated_savings, completed_at, error, expires_at, + cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + created_by_user_id, retry_execution_id, retry_attempt_n, + approval_token_expires_at, + executed_by_user_id, executed_at, pre_approval_skip_reason, + idempotency_key, scheduled_execution_at, no_submission, created_by_system + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23, $24, $25, $26, $27, $28, $29) + ON CONFLICT (execution_id) DO UPDATE SET + status = $3, + notification_sent = $6, + approval_token_hash = $7, + recommendations = $8, + total_upfront_cost = $9, + estimated_savings = $10, + completed_at = $11, + error = $12, + expires_at = $13, + cloud_account_id = $14, + source = $15, + approved_by = $16, + cancelled_by = $17, + capacity_percent = $18, + approval_token_expires_at = $22, + executed_by_user_id = $23, + executed_at = $24, + pre_approval_skip_reason = $25, + scheduled_execution_at = $27, + no_submission = CASE WHEN $3 = 'failed' + THEN purchase_executions.no_submission OR $28 + ELSE false END, + updated_at = NOW()` + oldSetCancelledBy157 = `UPDATE purchase_executions SET canceled_by = $2, cancelled_by = $2, updated_at = NOW() + WHERE execution_id = $1` + oldTransition157 = ` + UPDATE purchase_executions + SET status = $2, updated_at = NOW(), + transitioned_by = $4, transitioned_at = NOW(), + canceled_by = COALESCE((SELECT email FROM users WHERE id = $5), canceled_by), + cancelled_by = COALESCE((SELECT email FROM users WHERE id = $5), cancelled_by) + WHERE execution_id = $1 AND status = ANY($3)` + oldCancelAllExchanges157 = ` + UPDATE ri_exchange_history + SET status = $1, updated_at = NOW() + WHERE status = 'pending'` +) + +func requireOldBuildWritesStillWork157(ctx context.Context, t *testing.T, pool *pgxpool.Pool) { + t.Helper() + now := time.Now().UTC() + var execID string + require.NoError(t, pool.QueryRow(ctx, `SELECT gen_random_uuid()::text`).Scan(&execID)) + save := func(status, actor string) error { + _, err := pool.Exec(ctx, oldSaveExecution157, + nil, execID, status, 1, now, nil, nil, []byte(`[]`), 0, 0, nil, "", nil, + nil, "web", nil, actor, 100, nil, nil, 0, nil, nil, nil, nil, nil, nil, false, false) + return err + } + + // SavePurchaseExecutionTx: insert, then the ON CONFLICT update, both with + // the canonical status and the legacy actor column only. + require.NoError(t, save("pending", ""), "old upsert insert path") + require.NoError(t, save(canceled157, "old-save@example.com"), "old upsert update path") + status, actor := readActor157(ctx, t, pool, execID) + assert.Equal(t, canceled157, status) + require.NotNil(t, actor, "an actor written to the legacy column must read back") + assert.Equal(t, "old-save@example.com", *actor) + + // SetCancelledBy. + ct, err := pool.Exec(ctx, oldSetCancelledBy157, execID, "set-by@example.com") + require.NoError(t, err) + require.EqualValues(t, 1, ct.RowsAffected()) + _, actor = readActor157(ctx, t, pool, execID) + require.NotNil(t, actor) + assert.Equal(t, "set-by@example.com", *actor) + + // transitionExecutionStatus dual write. + var userID string + require.NoError(t, pool.QueryRow(ctx, ` + INSERT INTO users (email, password_hash, group_ids) VALUES ('transition@example.com', 'x', ARRAY[gen_random_uuid()]) RETURNING id::text`).Scan(&userID)) + transitionID := seedExecution157(ctx, t, pool, "pending", nil, now) + ct, err = pool.Exec(ctx, oldTransition157, transitionID, canceled157, []string{"pending"}, userID, userID) + require.NoError(t, err) + require.EqualValues(t, 1, ct.RowsAffected()) + status, actor = readActor157(ctx, t, pool, transitionID) + assert.Equal(t, canceled157, status) + require.NotNil(t, actor) + assert.Equal(t, "transition@example.com", *actor) + + // CancelAllPendingExchanges. + exchangeID, err := insertExchange157(ctx, pool, "pending", now) + require.NoError(t, err) + _, err = pool.Exec(ctx, oldCancelAllExchanges157, canceled157) + require.NoError(t, err) + var exchangeStatus string + require.NoError(t, pool.QueryRow(ctx, `SELECT status FROM ri_exchange_history WHERE id = $1`, exchangeID).Scan(&exchangeStatus)) + assert.Equal(t, canceled157, exchangeStatus) +} diff --git a/internal/purchase/ladder_reconcile_integration_test.go b/internal/purchase/ladder_reconcile_integration_test.go index 595cd2740..0228e92f6 100644 --- a/internal/purchase/ladder_reconcile_integration_test.go +++ b/internal/purchase/ladder_reconcile_integration_test.go @@ -127,7 +127,6 @@ func TestReconcileLadderTranches_Mappings(t *testing.T) { {"failed without proof (reaper / outcome unknown) keeps netting", "failed", false, true, "fired", ""}, {"proof flag on a running row is ignored", "running", true, true, "fired", ""}, {"canceled approval", "canceled", false, true, "failed", "approval_canceled"}, - {"legacy canceled spelling", config.LegacyStatusCanceled, false, true, "failed", "approval_canceled"}, {"expired approval", "expired", false, true, "failed", "approval_expired"}, {"partially completed stays fired", "partially_completed", false, true, "fired", ""}, {"completed and visible", "completed", false, true, "completed", "commitment_visible"}, diff --git a/known_issues/41_cancel_actor_strict_reads.md b/known_issues/41_cancel_actor_strict_reads.md new file mode 100644 index 000000000..d7dd2fa1a --- /dev/null +++ b/known_issues/41_cancel_actor_strict_reads.md @@ -0,0 +1,15 @@ +# Known Issues: execution reads that skip the legacy cancel actor (#47) + +> **Audit status (2026-11-04):** `1 needs triage ยท 0 resolved` + +## LOW: two ramp reads select canceled_by without the legacy cancelled_by fallback + +**Files:** `internal/config/store_postgres_ramp_baseline.go` (`ListPlanCommittedTx`), `internal/config/store_postgres_ramp_step.go` (`ListExecutionsForPlanStepTx`) + +**Description:** Every other execution projection reads the cancel actor as `COALESCE(canceled_by, cancelled_by)`. These two select `canceled_by` alone. A legacy row whose actor was only ever written to `cancelled_by` (by builds before #1253) comes back with no actor here. Both callers decide by `status`, not by the actor, so no ramp, capacity or purchase decision changes. Only the `CancelledBy` field on the returned rows is empty. + +**Why deferred:** Pre-existing, from before #1253, and it doesn't affect any decision. The final step of the rename drains `cancelled_by` into `canceled_by` before dropping the column, and that fixes these rows at the source. + +**Issue:** #1747 (follow-up of #47 and #1253) + +**Status:** needs triage