From 62feb4fc5eae65ec8dddb5ee5ed693a08a1669d3 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 04:14:14 +0000 Subject: [PATCH 1/2] fix(db): contract the canceled rename: normalize, narrow the status CHECKs, stop using cancelled_by Migration 000144 is the contract half of 000089. Under a table lock it normalizes every legacy 'cancelled' status in purchase_executions and ri_exchange_history, drains legacy-only actors into canceled_by, and narrows both status CHECKs to the 000089 lists minus 'cancelled'. The code stops reading and writing purchase_executions.cancelled_by: reads project canceled_by, the cancel transition and SetCancelledBy write only canceled_by, and SavePurchaseExecution writes canceled_by, keeping an actor already on the row (the value reads saw before). The LegacyStatusCanceled constant and every dual-spelling branch are gone. The cancelled_by column is kept. The previous build still reads COALESCE(canceled_by, cancelled_by) and writes cancelled_by, and it keeps serving during a rolling deploy and after a code-only rollback, so dropping the column in the same release would break every execution read on it. A trigger mirrors any legacy-only actor write into canceled_by until a follow-up migration drops the trigger and column. Refs #47 Co-Authored-By: Claude Opus 5.5 --- cmd/cleanup-lambda/main_integration_test.go | 3 +- internal/api/handler_history.go | 20 +- internal/api/handler_history_test.go | 13 +- internal/api/handler_purchases.go | 18 +- ...purchases_cancel_actor_integration_test.go | 25 +- ...urchases_submit_dedupe_integration_test.go | 2 +- internal/api/handler_purchases_test.go | 61 ----- internal/api/plan_health.go | 6 +- internal/api/plan_health_test.go | 9 +- internal/config/interfaces.go | 9 +- internal/config/store_postgres.go | 65 +++-- internal/config/store_postgres_db_test.go | 11 - .../config/store_postgres_ladder_resolve.go | 4 +- ...ostgres_ladder_resolve_integration_test.go | 1 - .../store_postgres_ladder_resolve_test.go | 2 +- .../config/store_postgres_pgxmock_test.go | 146 ++++++----- .../store_postgres_plan_schedule_clock.go | 2 +- .../config/store_postgres_ramp_baseline.go | 2 +- ...postgres_ramp_baseline_integration_test.go | 3 +- .../store_postgres_ramp_baseline_test.go | 2 +- ...ore_postgres_ramp_step_integration_test.go | 2 +- ...ore_postgres_retention_integration_test.go | 3 +- internal/config/types.go | 23 +- ...000144_contract_canceled_spelling.down.sql | 33 +++ .../000144_contract_canceled_spelling.up.sql | 70 +++++ .../000144_contract_canceled_spelling_test.go | 243 ++++++++++++++++++ internal/purchase/ladder_reconcile.go | 2 +- .../ladder_reconcile_integration_test.go | 1 - internal/purchase/ladder_reconcile_test.go | 1 - 29 files changed, 505 insertions(+), 277 deletions(-) create mode 100644 internal/database/postgres/migrations/000144_contract_canceled_spelling.down.sql create mode 100644 internal/database/postgres/migrations/000144_contract_canceled_spelling.up.sql create mode 100644 internal/database/postgres/migrations/000144_contract_canceled_spelling_test.go diff --git a/cmd/cleanup-lambda/main_integration_test.go b/cmd/cleanup-lambda/main_integration_test.go index 4f0afbcde..6f8951e65 100644 --- a/cmd/cleanup-lambda/main_integration_test.go +++ b/cmd/cleanup-lambda/main_integration_test.go @@ -15,7 +15,6 @@ import ( "testing" "time" - "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/database" "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" @@ -30,7 +29,7 @@ var ( cleanupDeletable = []string{"completed", "partially_completed", "expired", "revocation_requested"} // Live (an executor, scheduler or reaper may still act on it), or counted // by the health score and left to CleanupOldExecutions: always kept. - cleanupKept = []string{"pending", "notified", "approved", "running", "scheduled", "paused", "failed", "canceled", config.LegacyStatusCanceled} + cleanupKept = []string{"pending", "notified", "approved", "running", "scheduled", "paused", "failed", "canceled"} ) func cleanupTestDB(ctx context.Context, t *testing.T) *database.Connection { diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 40e9fb678..79d24b6b4 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -131,12 +131,7 @@ var actionableExecutionStatuses = []string{"pending", "notified", "scheduled", " // partial-failure marker and is flagged IsAuditGap so its execution-level // dollars are excluded from the dashboard totals — the committed dollars are // already counted via the per-rec purchase_history rows that succeeded. -// Both the US-spelling status (config.StatusCanceled) and the legacy British -// spelling (config.LegacyStatusCanceled) are listed: during the expand-contract -// rename (migration 000089) old code may still write the legacy value before -// the rolling deploy completes. The contract migration (#1278) normalizes the -// data once the deploy is verified stable; drop the legacy entry here then. -var terminalExecutionStatuses = []string{"completed", "partially_completed", "failed", "expired", config.StatusCanceled, config.LegacyStatusCanceled} +var terminalExecutionStatuses = []string{"completed", "partially_completed", "failed", "expired", config.StatusCanceled} // approvalExpiryWindow is how long a pending approval stays actionable // before the History view flips it to "expired". Aligns with the @@ -423,10 +418,7 @@ func annotateHistoryRowByStatus(row *config.PurchaseHistoryRecord, exec config.P row.StatusDescription = exec.Error case "expired": row.StatusDescription = "approval link expired (not approved within 7 days)" - case config.StatusCanceled, config.LegacyStatusCanceled: - // The legacy British spelling is still matched during the - // expand-contract rename (migration 000089) until the contract - // migration (#1278) normalizes and drops it. + case config.StatusCanceled: annotateCancelled(row, exec, approver) default: // In-flight (approved/running/scheduled/paused) and audit-gap @@ -1156,14 +1148,10 @@ func summarizePurchaseHistory(purchases []config.PurchaseHistoryRecord) HistoryS case "expired": summary.TotalExpired++ continue - case config.StatusCanceled, config.LegacyStatusCanceled: + case config.StatusCanceled: // A canceled purchase represents zero committed spend and zero // realized savings (issue #736). Exclude from all dollar KPIs and - // from TotalCompleted -- the money was never committed. The legacy - // British spelling is matched alongside the US one during the - // expand-contract rename (migration 000089) so legacy rows can't - // inflate KPIs mid-deploy; the contract migration (#1278) drops it - // once the deploy is stable. + // from TotalCompleted -- the money was never committed. continue } summary.TotalCompleted++ diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 617477408..5bd10fc0e 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -1775,16 +1775,9 @@ func TestSummarizePurchaseHistory_CancelledExcludedFromKPIs(t *testing.T) { // One pending row that should be counted as pending, not completed. {Status: "pending", UpfrontCost: new(float64(999.0)), EstimatedSavings: 99.0}, // Two canceled rows — the regression case from issue #736. Neither must - // appear in the dollar KPIs or TotalCompleted. One uses the new US - // spelling (config.StatusCanceled) and one the legacy British spelling - // (config.LegacyStatusCanceled): during the expand-contract rename - // (migration 000089) a mixed fleet still emits the legacy spelling, so - // the dual-spelling read path must exclude BOTH. The constant carries - // the legacy value without a literal the US-locale misspell linter would - // flag (and without a nolint). Drop the legacy fixture once the contract - // migration (#1278) normalizes the data. + // appear in the dollar KPIs or TotalCompleted. {Status: config.StatusCanceled, UpfrontCost: new(float64(500.0)), EstimatedSavings: 50.0}, - {Status: config.LegacyStatusCanceled, UpfrontCost: new(float64(750.0)), EstimatedSavings: 75.0}, + {Status: config.StatusCanceled, UpfrontCost: new(float64(750.0)), EstimatedSavings: 75.0}, } summary := summarizePurchaseHistory(purchases) @@ -1921,7 +1914,7 @@ func TestSummarizePurchaseHistory_CancelPendingDoesNotChangeKPIs(t *testing.T) { // After: same rows plus one canceled execution (the pending that got canceled). withCancelled := append(baseline, config.PurchaseHistoryRecord{ //nolint:gocritic - Status: "cancelled", //nolint:misspell // DB schema value 'cancelled' -- see migration 000001_initial_schema.up.sql + Status: config.StatusCanceled, UpfrontCost: new(float64(999.0)), EstimatedSavings: 99.0, }) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index ce5212ded..9f7629d61 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -770,27 +770,11 @@ func (h *Handler) cancelOrRecoverExecution(ctx context.Context, executionID stri if getErr != nil { return nil, fmt.Errorf("disable plan: failed to get execution %s after conflict: %w", executionID, getErr) } - // Accept both spellings: during the expand-contract rename (migration - // 000089) a concurrent legacy cancel may have written the legacy value - // before the rolling deploy completes. The contract migration (#1278) - // normalizes the data, after which LegacyStatusCanceled can be removed. - if existing.Status != config.StatusCanceled && existing.Status != config.LegacyStatusCanceled { + if existing.Status != config.StatusCanceled { return nil, NewClientError(409, fmt.Sprintf( "execution %s cannot be canceled (status=%s)", executionID, existing.Status)) } - // Normalize the response Status so the idempotent recovery path returns the - // canonical US spelling even when the DB row still carries the legacy value. - // Without this, a caller that received an in-flight 200 from this branch - // would see status="canceled" while a caller that hit the happy-path - // transition above would see status="canceled" for the same execution_id - // during the rolling deploy. The legacy value is preserved in storage -- - // only the in-memory copy returned to the handler is normalized -- so the - // contract migration's authoritative status backfill still observes every - // legacy row. - if existing.Status == config.LegacyStatusCanceled { - existing.Status = config.StatusCanceled - } if delErr := h.config.DeleteSuppressionsByExecution(ctx, executionID); delErr != nil { return nil, fmt.Errorf("cancel execution %s: failed to release suppressions: %w", executionID, delErr) } 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_submit_dedupe_integration_test.go b/internal/api/handler_purchases_submit_dedupe_integration_test.go index 5f89a65d7..f56ddd70a 100644 --- a/internal/api/handler_purchases_submit_dedupe_integration_test.go +++ b/internal/api/handler_purchases_submit_dedupe_integration_test.go @@ -122,7 +122,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/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 2bda5e715..a1865edbd 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -2019,67 +2019,6 @@ func TestHandler_deletePlannedPurchase_ConflictRetryRunningReturns409(t *testing assert.Contains(t, ce.message, "running", "error must include actual status") } -// TestHandler_deletePlannedPurchase_ConflictRetryLegacyCanceledNormalizes is -// the regression guard for the CR outside-diff comment on PR #1277: when the -// CAS conflict recovery branch finds the row in the LEGACY British spelling -// (the row was canceled by old code during the rolling deploy window), the -// API response Status must be normalized to the canonical US spelling so a -// caller doesn't observe two different status values for the same execution -// depending on which code instance handled the request. The stored row is -// untouched -- only the in-memory copy returned to the handler is normalized. -func TestHandler_deletePlannedPurchase_ConflictRetryLegacyCanceledNormalizes(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - mockAuth := new(MockAuthService) - t.Cleanup(func() { mockStore.AssertExpectations(t) }) - t.Cleanup(func() { mockAuth.AssertExpectations(t) }) - - adminSession := &Session{ - UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", - Email: "admin@example.com", - } - - planID := "12121212-1212-1212-1212-121212121212" - execID := "13131313-1313-1313-1313-131313131313" - - conflictErr := fmt.Errorf("%w: execution %s cannot transition", config.ErrExecutionNotInExpectedStatus, execID) - - // Old code already canceled this row during the deploy window: the DB - // holds the legacy British spelling. The handler must accept it (idempotent - // recovery) but normalize the response to the canonical US spelling. - existingExec := &config.PurchaseExecution{ - ExecutionID: execID, - PlanID: planID, - Status: config.LegacyStatusCanceled, - } - plan := &config.PurchasePlan{ - ID: planID, - Name: "Legacy Cancel Plan", - Enabled: true, - } - - mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(nil, conflictErr) - mockStore.On("GetExecutionByID", ctx, execID).Return(existingExec, nil) - mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) - mockStore.On("UpdatePurchasePlan", ctx, mock.MatchedBy(func(p *config.PurchasePlan) bool { - return p.ID == planID && !p.Enabled - })).Return(nil) - - handler := &Handler{config: mockStore, auth: mockAuth} - - req := &events.LambdaFunctionURLRequest{ - Headers: map[string]string{"Authorization": "Bearer admin-token"}, - } - result, err := handler.deletePlannedPurchase(ctx, req, execID) - require.NoError(t, err) - assert.Equal(t, config.StatusCanceled, result.Status, - "legacy 'cancelled' must be normalized to the canonical US 'canceled' in the response") - assert.NotEqual(t, config.LegacyStatusCanceled, result.Status, - "the response must never leak the legacy spelling once the recovery branch ran") -} - // TestHandler_deletePlannedPurchase_BackendErrorReturns5xx is the regression // guard for CodeRabbit round-2 finding #1 (same class as PR #1276): a // TransitionExecutionStatus error that is NOT ErrExecutionNotInExpectedStatus diff --git a/internal/api/plan_health.go b/internal/api/plan_health.go index 0210854d5..f37fcd1cb 100644 --- a/internal/api/plan_health.go +++ b/internal/api/plan_health.go @@ -222,11 +222,9 @@ func failedExecutionsFactor(counts config.ExecutionStatusCounts) (PlanHealthFact } // canceledExecutionsFactor: -5 per canceled execution in the last -// planHealthLookbackDays, capped at 4 (-20 max). Counts both spellings of -// "canceled" (config.StatusCanceled and the legacy -// config.LegacyStatusCanceled) so pre-#1278 rows aren't invisible to scoring. +// planHealthLookbackDays, capped at 4 (-20 max). func canceledExecutionsFactor(counts config.ExecutionStatusCounts) (PlanHealthFactor, bool) { - count := counts[config.StatusCanceled] + counts[config.LegacyStatusCanceled] + count := counts[config.StatusCanceled] if count == 0 { return PlanHealthFactor{}, false } diff --git a/internal/api/plan_health_test.go b/internal/api/plan_health_test.go index 7f6b5605a..47eb6c10d 100644 --- a/internal/api/plan_health_test.go +++ b/internal/api/plan_health_test.go @@ -149,7 +149,7 @@ func TestComputePlanHealth_FailedExecutionsPenaltyCapsAtFour(t *testing.T) { assert.Equal(t, 100-40, score) } -func TestComputePlanHealth_CanceledExecutionsPenaltyCapsAtFourAndCountsBothSpellings(t *testing.T) { +func TestComputePlanHealth_CanceledExecutionsPenaltyCapsAtFour(t *testing.T) { now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) plan := config.PurchasePlan{ Enabled: true, @@ -160,11 +160,10 @@ func TestComputePlanHealth_CanceledExecutionsPenaltyCapsAtFourAndCountsBothSpell StartDate: now.AddDate(0, 0, -1), }, } - // 3 canonical + 2 legacy-spelled = 5 total canceled; penalty caps at - // 4 * 5 = 20 but the note reports the true count of 5. + // 5 canceled; penalty caps at 4 * 5 = 20 but the note reports the true + // count of 5. execCounts := counts(map[string]int{ - config.StatusCanceled: 3, - config.LegacyStatusCanceled: 2, + config.StatusCanceled: 5, }) score, factors := computePlanHealth(plan, now, execCounts, config.RampStepBlock{}) diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 9a078709a..4459f0acb 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -251,11 +251,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 668d1bd27..ca9e39ebd 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1083,12 +1083,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. + // This is the value reads saw before migration 000144, when the save wrote + // the legacy column and reads took COALESCE(canceled_by, 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, @@ -1107,7 +1111,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, @@ -1239,8 +1243,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) } @@ -1319,8 +1323,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 @@ -1328,7 +1331,7 @@ func (s *PostgresStore) transitionExecutionStatus(ctx context.Context, q executi RETURNING 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, COALESCE(canceled_by, cancelled_by) AS 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, @@ -1377,16 +1380,13 @@ 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). 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 @@ -1545,7 +1545,7 @@ func (s *PostgresStore) GetExecutionsByStatuses(ctx context.Context, statuses [] SELECT 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, COALESCE(canceled_by, cancelled_by) AS 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, @@ -1659,7 +1659,7 @@ func (s *PostgresStore) GetPlannedExecutions(ctx context.Context, statuses []str SELECT 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, COALESCE(canceled_by, cancelled_by) AS 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, @@ -1684,7 +1684,7 @@ func (s *PostgresStore) GetStaleApprovedExecutions(ctx context.Context, olderTha SELECT 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, COALESCE(canceled_by, cancelled_by) AS 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, @@ -1726,7 +1726,7 @@ func (s *PostgresStore) ListStuckExecutions(ctx context.Context, statuses []stri SELECT 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, COALESCE(canceled_by, cancelled_by) AS 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, @@ -1747,7 +1747,7 @@ func (s *PostgresStore) GetPendingExecutions(ctx context.Context) ([]PurchaseExe SELECT 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, COALESCE(canceled_by, cancelled_by) AS 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, @@ -1771,7 +1771,7 @@ func (s *PostgresStore) GetPendingExecutionsTx(ctx context.Context, tx pgx.Tx) ( SELECT 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, COALESCE(canceled_by, cancelled_by) AS 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, @@ -1858,7 +1858,7 @@ func (s *PostgresStore) ListRecentSubmitsTx(ctx context.Context, tx pgx.Tx, crea SELECT 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, COALESCE(canceled_by, cancelled_by) AS 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, @@ -1893,7 +1893,7 @@ func getExecutionByIDOn(ctx context.Context, q executionQuerier, executionID str SELECT 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, COALESCE(canceled_by, cancelled_by) AS 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, @@ -1920,7 +1920,7 @@ func (s *PostgresStore) GetExecutionByPlanAndDate(ctx context.Context, planID st SELECT 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, COALESCE(canceled_by, cancelled_by) AS 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, @@ -2161,7 +2161,7 @@ func (s *PostgresStore) GetScheduledExecutionsDue(ctx context.Context) ([]Purcha SELECT 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, COALESCE(canceled_by, cancelled_by) AS 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, @@ -2192,10 +2192,8 @@ func (s *PostgresStore) GetScheduledExecutionsDue(ctx context.Context) ([]Purcha // clock here because a completed row executed on (or very near) the day // it was scheduled for. // -// 2. Canceled-state cleanup: `status IN ('cancelled', 'canceled') AND -// updated_at < NOW() - retention`. Both spellings are included to cover -// legacy rows ('cancelled') and new code rows ('canceled', canonical US -// spelling per StatusCanceled / migration 000089). +// 2. Canceled-state cleanup: `status = 'canceled' AND updated_at < NOW() - +// retention`. // // This branch retains on `updated_at`, NOT `scheduled_date`, and must // stay that way: CountExecutionsByPlanAndStatus windows the plan health @@ -2266,11 +2264,6 @@ func (s *PostgresStore) GetScheduledExecutionsDue(ctx context.Context) ([]Purcha // are safe from expiry-based cleanup -- which, per the reachability note // above, is every row current code writes. func (s *PostgresStore) CleanupOldExecutions(ctx context.Context, retentionDays int) (int64, error) { - // Branch 2 lists both spellings: 'cancelled' (legacy, pre-migration - // 000089 rows) and 'canceled' (canonical, written by new code after the - // follow-up to #1277). #1278 will drop 'cancelled' once all rows are - // normalized. - // // Branch 3's exclusion is parameterized from // HealthScoredExecutionStatuses instead of repeating a literal list, so // a status added to the plan health score extends this exclusion @@ -2283,7 +2276,7 @@ func (s *PostgresStore) CleanupOldExecutions(ctx context.Context, retentionDays AND scheduled_date < NOW() - INTERVAL '1 day' * $1 ) OR ( - status IN ('cancelled', 'canceled') + status = 'canceled' AND updated_at < NOW() - INTERVAL '1 day' * $1 ) OR ( diff --git a/internal/config/store_postgres_db_test.go b/internal/config/store_postgres_db_test.go index 4761b8f85..583545239 100644 --- a/internal/config/store_postgres_db_test.go +++ b/internal/config/store_postgres_db_test.go @@ -1289,17 +1289,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_ladder_resolve.go b/internal/config/store_postgres_ladder_resolve.go index aea265e56..b92b30790 100644 --- a/internal/config/store_postgres_ladder_resolve.go +++ b/internal/config/store_postgres_ladder_resolve.go @@ -61,7 +61,7 @@ type LadderResolveResult struct { // still move money, so it blocks the resolve. func ladderResolvableNodeStatus(status string) bool { switch status { - case StatusFailed, StatusCanceled, LegacyStatusCanceled, StatusExpired, StatusCompleted, StatusPartiallyCompleted: + case StatusFailed, StatusCanceled, StatusExpired, StatusCompleted, StatusPartiallyCompleted: return true } return false @@ -71,7 +71,7 @@ func ladderResolvableNodeStatus(status string) bool { // the provider. func ladderNodeProvenUnsent(n LadderChainNode) bool { switch n.Status { - case StatusCanceled, LegacyStatusCanceled, StatusExpired: + case StatusCanceled, StatusExpired: return true case StatusFailed: return n.NoSubmission diff --git a/internal/config/store_postgres_ladder_resolve_integration_test.go b/internal/config/store_postgres_ladder_resolve_integration_test.go index b16b9d966..7ed2bda6c 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_ladder_resolve_test.go b/internal/config/store_postgres_ladder_resolve_test.go index ba4c4287d..aeb09f332 100644 --- a/internal/config/store_postgres_ladder_resolve_test.go +++ b/internal/config/store_postgres_ladder_resolve_test.go @@ -9,7 +9,7 @@ import ( // The chain-node allowlist fails closed: a live status or one added later is // not resolvable (the status CHECK keeps such rows out of the database tests). func TestLadderResolvableNodeStatus(t *testing.T) { - for _, s := range []string{StatusFailed, StatusCanceled, LegacyStatusCanceled, StatusExpired, StatusCompleted, StatusPartiallyCompleted} { + for _, s := range []string{StatusFailed, StatusCanceled, StatusExpired, StatusCompleted, StatusPartiallyCompleted} { assert.True(t, ladderResolvableNodeStatus(s), s) } for _, s := range []string{"pending", "notified", "scheduled", "approved", "running", "", "future-status"} { diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 25a76d932..994abe913 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -882,51 +882,29 @@ func TestPGXMock_GetPlannedExecutions_ProjectsAllScanColumns(t *testing.T) { assert.NoError(t, mock.ExpectationsWereMet()) } -// TestPGXMock_GetExecutionByID_ProjectsCoalescedCancelledBy guards the #1277 / -// #1453 read-side follow-up: CancelExecutionAtomic and -// CancelScheduledExecutionAtomic write the canceling actor to the NEW -// canceled_by column, but migration 000089's expand-contract contract states -// every SELECT must read back COALESCE(canceled_by, cancelled_by) so a row -// written by either the old or the new column is attributed correctly. Before -// this fix GetExecutionByID selected the bare legacy cancelled_by, so an -// actor written only to canceled_by (the atomic-cancel paths) scanned back as -// NULL and the History page lost the attribution. -// -// The mock uses regexp query matching, so ExpectQuery requires the issued -// SELECT to contain the COALESCE expression; a bare `cancelled_by` projection -// (the pre-fix query) does not match, the mock returns no rows, and -// GetExecutionByID surfaces ErrNotFound instead of the expected execution -- -// that is how this test fails against the pre-fix code and passes once the -// projection is fixed. -func TestPGXMock_GetExecutionByID_ProjectsCoalescedCancelledBy(t *testing.T) { - mock := newMock(t) - store := storeWith(mock) - ctx := context.Background() - - recsJSON, _ := json.Marshal([]RecommendationRecord{}) - now := time.Now().Truncate(time.Second) - cols := []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", "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", - } - // Simulates the exact row shape the atomic-cancel paths produce: the - // actor lives only in canceled_by (legacy cancelled_by is NULL). What - // Postgres would actually return for COALESCE(canceled_by, cancelled_by) - // is the canceled_by value, so the mocked "cancelled_by" scan slot below - // carries that same value to stand in for the server-side COALESCE result. - rows := pgxmock.NewRows(cols).AddRow( - "plan-1", "exec-1", "canceled", 1, now, +// executionRowCols is the scan shape of every purchase_executions read. +var executionRowCols = []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", "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", + "idempotency_key", + "scheduled_execution_at", + "no_submission", "created_by_system", +} + +func canceledExecutionRow(t *testing.T, actor string) *pgxmock.Rows { + t.Helper() + recsJSON, err := json.Marshal([]RecommendationRecord{}) + require.NoError(t, err) + return pgxmock.NewRows(executionRowCols).AddRow( + "plan-1", "exec-1", StatusCanceled, 1, time.Now().Truncate(time.Second), sql.NullTime{}, "tok-123", recsJSON, 100.0, 200.0, sql.NullTime{}, "", sql.NullTime{}, - nil, "", nil, strPtr("cancelling-actor@example.com"), 100, + nil, "", nil, strPtr(actor), 100, nil, nil, 0, sql.NullTime{}, nil, sql.NullTime{}, nil, @@ -934,14 +912,63 @@ func TestPGXMock_GetExecutionByID_ProjectsCoalescedCancelledBy(t *testing.T) { sql.NullTime{}, false, false, ) - mock.ExpectQuery(`COALESCE\(canceled_by,\s*cancelled_by\)`). - WithArgs(pgxmock.AnyArg()). - WillReturnRows(rows) +} + +// TestPGXMock_ExecutionCancelActorUsesOnlyCanceledBy pins the code half of +// the 000089 contract (issue #47): reads, cancel transitions, SetCancelledBy +// and the full-row save use canceled_by alone and never name the legacy +// double-l column, so the follow-up migration can drop that column without +// breaking a build that runs this code. The matcher rejects any statement that +// mentions the legacy column, so each call fails against the pre-#47 queries +// (COALESCE projections, dual writes, the save writing only the legacy column). +// +// The save keeps an actor already on the row (COALESCE(existing, $17)): reads +// used COALESCE(canceled_by, cancelled_by) and the save wrote only the legacy +// column, so a save from a stale copy could never erase canceled_by. Writing +// $17 straight into canceled_by would let it. +func TestPGXMock_ExecutionCancelActorUsesOnlyCanceledBy(t *testing.T) { + legacyColumn := "cancel" + "led_by" + mock, err := pgxmock.NewPool(pgxmock.QueryMatcherOption(pgxmock.QueryMatcherFunc(func(expected, actual string) error { + if strings.Contains(actual, legacyColumn) { + return errors.New("statement still references the legacy cancel actor column: " + actual) + } + return pgxmock.QueryMatcherRegexp.Match(expected, actual) + }))) + require.NoError(t, err) + store := storeWith(mock) + ctx := context.Background() + mock.ExpectQuery(`approved_by, canceled_by, capacity_percent`). + WithArgs("exec-1"). + WillReturnRows(canceledExecutionRow(t, "reader@example.com")) exec, err := store.GetExecutionByID(ctx, "exec-1") require.NoError(t, err) - require.NotNil(t, exec.CancelledBy, "actor written to canceled_by must round-trip via the COALESCE read projection") - assert.Equal(t, "cancelling-actor@example.com", *exec.CancelledBy) + require.NotNil(t, exec.CancelledBy) + assert.Equal(t, "reader@example.com", *exec.CancelledBy) + + 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(canceledExecutionRow(t, "canceler@example.com")) + returned, err := store.TransitionExecutionStatus(ctx, "exec-1", []string{"pending"}, StatusCanceled, &actor) + require.NoError(t, err) + require.NotNil(t, returned.CancelledBy) + assert.Equal(t, "canceler@example.com", *returned.CancelledBy) + + 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(29)...). + 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()) } @@ -3180,18 +3207,17 @@ func TestPGXMock_CancelPendingExchangesByOrigin_Ladder_WritesCanonicalStatus(t * } // TestPGXMock_CleanupOldExecutions_IncludesCanonicalStatus verifies that the -// cleanup DELETE includes 'canceled' (canonical) in the terminal-status filter, -// so Plans-canceled rows written by new code are not missed. -// Regression guard for the #1277 follow-up Defect 2 fix. +// cleanup DELETE includes 'canceled' in the terminal-status filter, so +// Plans-canceled rows are not missed. Regression guard for the #1277 +// follow-up Defect 2 fix. func TestPGXMock_CleanupOldExecutions_IncludesCanonicalStatus(t *testing.T) { mock := newMock(t) store := storeWith(mock) ctx := context.Background() - // The regex pins on both 'cancelled' (legacy) and 'canceled' (canonical) - // appearing together in the SQL. If either is missing the mock won't - // match, causing an "unexpected query" error from ExpectationsWereMet. - mock.ExpectExec(`'cancelled'.*'canceled'`). + // If 'canceled' is missing the mock won't match, causing an + // "unexpected query" error from ExpectationsWereMet. + mock.ExpectExec(`status = 'canceled'`). WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("DELETE", 4)) @@ -3213,7 +3239,7 @@ func TestPGXMock_CleanupOldExecutions_IncludesCanonicalStatus(t *testing.T) { // regressed instead of just "the SQL changed". func TestPGXMock_CleanupOldExecutions_RetainsCanceledOnUpdatedAt(t *testing.T) { anchors := map[string]string{ - "canceled branch retains on updated_at": `(?s)status IN \('cancelled', 'canceled'\)\s+AND updated_at`, + "canceled branch retains on updated_at": `(?s)status = 'canceled'\s+AND updated_at`, "completed branch retains on scheduled_date": `(?s)status = 'completed'\s+AND scheduled_date`, "expires_at branch skips scored statuses": `(?s)status <> ALL\(\$2\)\s+AND expires_at IS NOT NULL`, } @@ -3259,7 +3285,7 @@ func TestPGXMock_CleanupOldExecutions_ExcludesEveryHealthScoredStatus(t *testing assert.NoError(t, mock.ExpectationsWereMet()) // Guards the slice's contents independently of how the query uses it. assert.ElementsMatch(t, - []string{StatusFailed, StatusCanceled, LegacyStatusCanceled}, + []string{StatusFailed, StatusCanceled}, HealthScoredExecutionStatuses) } @@ -3282,13 +3308,13 @@ func TestPGXMock_CountExecutionsByPlanAndStatus_AggregatesInSQL(t *testing.T) { rows := pgxmock.NewRows([]string{"plan_id", "status", "count"}). AddRow("plan-a", "failed", 7). AddRow("plan-a", StatusCanceled, 2). - AddRow("plan-b", LegacyStatusCanceled, 1) + AddRow("plan-b", StatusCanceled, 1) mock.ExpectQuery(`(?s)SELECT plan_id, status, COUNT\(\*\).*FROM purchase_executions.*status = ANY\(\$1\).*plan_id IS NOT NULL.*updated_at >= \$2.*GROUP BY plan_id, status`). WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()). WillReturnRows(rows) counts, err := store.CountExecutionsByPlanAndStatus(ctx, - []string{"failed", StatusCanceled, LegacyStatusCanceled}, + []string{"failed", StatusCanceled}, time.Now().AddDate(0, 0, -90)) require.NoError(t, err) @@ -3296,7 +3322,7 @@ func TestPGXMock_CountExecutionsByPlanAndStatus_AggregatesInSQL(t *testing.T) { // though DefaultListLimit-style paging would be the temptation here. assert.Equal(t, 7, counts["plan-a"]["failed"]) assert.Equal(t, 2, counts["plan-a"][StatusCanceled]) - assert.Equal(t, 1, counts["plan-b"][LegacyStatusCanceled]) + assert.Equal(t, 1, counts["plan-b"][StatusCanceled]) // Absent statuses read as a zero count rather than needing a guard. assert.Equal(t, 0, counts["plan-b"]["failed"]) assert.Equal(t, 0, counts["plan-missing"]["failed"]) diff --git a/internal/config/store_postgres_plan_schedule_clock.go b/internal/config/store_postgres_plan_schedule_clock.go index 1725ee56d..0c0261d57 100644 --- a/internal/config/store_postgres_plan_schedule_clock.go +++ b/internal/config/store_postgres_plan_schedule_clock.go @@ -129,7 +129,7 @@ func managedPlanNextRoot(plan *PurchasePlan, rows []PurchaseExecution) (*Purchas if row.StepNumber != plan.RampSchedule.CurrentStep+1 { continue } - if row.Status == StatusCanceled || row.Status == LegacyStatusCanceled { + if row.Status == StatusCanceled { blocked = blocked || managedRootPriorAction(row) continue } diff --git a/internal/config/store_postgres_ramp_baseline.go b/internal/config/store_postgres_ramp_baseline.go index 40e76a31a..87d985b51 100644 --- a/internal/config/store_postgres_ramp_baseline.go +++ b/internal/config/store_postgres_ramp_baseline.go @@ -131,7 +131,7 @@ func (s *PostgresStore) ListPlanCommittedTx(ctx context.Context, tx pgx.Tx, plan // capacity. See ListPlanCommittedTx for the rule. func holdsCapacity(e *PurchaseExecution) bool { switch e.Status { - case StatusCanceled, LegacyStatusCanceled, StatusExpired: + case StatusCanceled, StatusExpired: return false case StatusFailed: return !e.NoSubmission 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_baseline_test.go b/internal/config/store_postgres_ramp_baseline_test.go index d9cda88d9..39e6a98b7 100644 --- a/internal/config/store_postgres_ramp_baseline_test.go +++ b/internal/config/store_postgres_ramp_baseline_test.go @@ -41,7 +41,7 @@ func TestSumPlanCommitted_StatusSet(t *testing.T) { assert.Equal(t, 20, got[key].Count, "status %q holds capacity", status) } - for _, status := range []string{StatusCanceled, LegacyStatusCanceled, StatusExpired} { + for _, status := range []string{StatusCanceled, StatusExpired} { got, err := sumPlanCommitted([]PurchaseExecution{baselineExec("e", 1, status, rec)}) require.NoError(t, err) assert.Empty(t, got, "status %q holds nothing", status) 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 49b698a5a..b4fbb4e91 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") // This is the other production writer, cmd/cleanup-lambda/main.go. tag, err := f.store.db.Exec(ctx, `DELETE FROM purchase_executions WHERE expires_at < NOW()`) require.NoError(t, err) diff --git a/internal/config/types.go b/internal/config/types.go index 4b43d6a0f..88389415a 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -439,18 +439,12 @@ type PurchaseExecution struct { NoSubmission bool `json:"no_submission,omitempty" dynamodbav:"no_submission,omitempty"` } -// StatusCanceled is the canonical US-spelling status value new code writes. +// StatusCanceled is the only canceled status value. Migration 000144 (the +// contract half of the 000089 rename) normalized every legacy British-spelled +// row to it and narrowed the status CHECK constraints so no other spelling can +// be stored. const StatusCanceled = "canceled" -// LegacyStatusCanceled is the British-spelling status value old code writes -// during the expand-contract rename (migration 000089). It is constructed by -// concatenation rather than a single literal so the US-locale misspell linter -// does not flag it -- this lets the dual-spelling read paths reference the -// legacy value without a //nolint:misspell directive. The CONTRACT migration -// (#1278) normalizes all rows to StatusCanceled once old code is gone, after -// which every reference to this constant can be deleted. -const LegacyStatusCanceled = "cancel" + "led" - // StatusFailed is the terminal status written when an execution's purchase // attempt errors out, and by the stuck-purchase reaper (internal/purchase/ // reaper.go) for rows left in approved/running past its threshold. @@ -487,8 +481,6 @@ const StatusExpired = "expired" // Canceled counts as settled but neither succeeded nor stuck: to cancel a // step's row is an operator deciding that unit will not buy, and that decision // is what releases a ramp an unrecoverable account would otherwise freeze. -// Both spellings are listed for the duration of the expand-contract rename -// (migration 000089, contract in #1278), same as HealthScoredExecutionStatuses. // // RampStepSucceededStatuses is the same predicate migration 000098 spells out // inline as status IN ('completed','partially_completed') when it decides which @@ -498,7 +490,7 @@ const StatusExpired = "expired" // "bought" means. var ( RampStepSucceededStatuses = []string{StatusCompleted, StatusPartiallyCompleted} - RampStepSettledStatuses = []string{StatusCompleted, StatusPartiallyCompleted, StatusCanceled, LegacyStatusCanceled} + RampStepSettledStatuses = []string{StatusCompleted, StatusPartiallyCompleted, StatusCanceled} RampStepStuckStatuses = []string{StatusFailed, StatusExpired} ) @@ -532,13 +524,9 @@ type RampStepBlock struct { // Keeping one exported slice rather than a literal list on each side is what // makes that agreement structural: adding a status to the score // automatically extends the sweep's exclusion, so the two cannot drift. -// -// Both spellings of canceled are present for the duration of the -// expand-contract rename (migration 000089, contract in #1278). var HealthScoredExecutionStatuses = []string{ StatusFailed, StatusCanceled, - LegacyStatusCanceled, } // StatusRevocationRequested records that an operator asked for a completed @@ -568,7 +556,6 @@ var TerminalExecutionStatuses = []string{ StatusPartiallyCompleted, StatusFailed, StatusCanceled, - LegacyStatusCanceled, StatusExpired, StatusRevocationRequested, } diff --git a/internal/database/postgres/migrations/000144_contract_canceled_spelling.down.sql b/internal/database/postgres/migrations/000144_contract_canceled_spelling.down.sql new file mode 100644 index 000000000..6bb808a17 --- /dev/null +++ b/internal/database/postgres/migrations/000144_contract_canceled_spelling.down.sql @@ -0,0 +1,33 @@ +-- Rollback 000144: back to the 000089 EXPAND state. +-- +-- Widen both status CHECKs again first, so nothing the data steps below touch +-- can violate them. Rows stay 'canceled': the widened CHECKs accept both +-- spellings and every build since #1277 reads both, so converting them back is +-- unnecessary. +-- +-- Then drop the mirror trigger and copy canceled_by into the legacy +-- cancelled_by wherever it is missing, so the column holds what a build that +-- still dual-writes it would have left there. + +LOCK TABLE purchase_executions, 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')); + +DROP TRIGGER IF EXISTS purchase_executions_mirror_legacy_cancel_actor ON purchase_executions; +DROP FUNCTION IF EXISTS purchase_executions_mirror_legacy_cancel_actor(); + +UPDATE purchase_executions +SET cancelled_by = canceled_by +WHERE cancelled_by IS NULL + AND canceled_by IS NOT NULL; diff --git a/internal/database/postgres/migrations/000144_contract_canceled_spelling.up.sql b/internal/database/postgres/migrations/000144_contract_canceled_spelling.up.sql new file mode 100644 index 000000000..37e946132 --- /dev/null +++ b/internal/database/postgres/migrations/000144_contract_canceled_spelling.up.sql @@ -0,0 +1,70 @@ +-- Migration 000144: 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. +-- +-- The legacy cancelled_by column is NOT dropped yet. The build that runs this +-- migration still has old instances serving during a rolling deploy, and a +-- code-only rollback (rollback.yml) can bring the previous build back with no +-- schema change. That previous build reads COALESCE(canceled_by, cancelled_by) +-- and writes cancelled_by, so dropping the column now would break every +-- execution read on those instances. The build that ships this migration no +-- longer reads or writes cancelled_by; the column drop is a later migration, +-- shipped once no older build can run. +-- +-- Until then a trigger keeps canceled_by authoritative: whenever a write leaves +-- canceled_by NULL but sets cancelled_by (what an older build's +-- SavePurchaseExecution does), it copies cancelled_by into canceled_by. That +-- makes canceled_by equal to COALESCE(canceled_by, cancelled_by), the value +-- older builds read, so new code can read canceled_by alone. +-- +-- Concurrency: both tables are locked up front, so no writer can commit a row +-- between the normalize/drain UPDATEs and the narrowed CHECKs (which would fail +-- the migration) or between the drain and the trigger (which would leave an +-- undrained actor). Writers wait for the migration and then see the trigger and +-- the narrowed CHECKs. golang-migrate runs this file in one transaction. + +LOCK TABLE purchase_executions, ri_exchange_history IN ACCESS EXCLUSIVE MODE; + +-- 1. Mirror legacy actor writes into canceled_by from now on. +CREATE OR REPLACE FUNCTION purchase_executions_mirror_legacy_cancel_actor() +RETURNS TRIGGER LANGUAGE plpgsql AS $$ +BEGIN + NEW.canceled_by := NEW.cancelled_by; + RETURN NEW; +END $$; + +DROP TRIGGER IF EXISTS purchase_executions_mirror_legacy_cancel_actor ON purchase_executions; +CREATE TRIGGER purchase_executions_mirror_legacy_cancel_actor + BEFORE INSERT OR UPDATE ON purchase_executions + FOR EACH ROW + WHEN (NEW.canceled_by IS NULL AND NEW.cancelled_by IS NOT NULL) + EXECUTE FUNCTION purchase_executions_mirror_legacy_cancel_actor(); + +-- 2. 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'; +UPDATE purchase_executions +SET canceled_by = cancelled_by +WHERE canceled_by IS NULL + AND cancelled_by IS NOT NULL; + +-- 3. 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' + )); + +-- 4. 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/000144_contract_canceled_spelling_test.go b/internal/database/postgres/migrations/000144_contract_canceled_spelling_test.go new file mode 100644 index 000000000..3da2100fe --- /dev/null +++ b/internal/database/postgres/migrations/000144_contract_canceled_spelling_test.go @@ -0,0 +1,243 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "regexp" + "sync" + "sync/atomic" + "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 ( + contract144 = 144 + legacyStatus144 = "cancel" + "led" + legacyActor144 = "cancel" + "led_by" +) + +// legacyInCheck144 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 legacyInCheck144 = regexp.MustCompile(`'` + legacyStatus144 + `'::[a-z ]+, `) + +const insertLegacyExecution144 = ` + INSERT INTO purchase_executions (status, scheduled_date, canceled_by, ` + legacyActor144 + `) + VALUES ($1, NOW(), $2, $3) RETURNING execution_id::text` + +func insertExecution144(ctx context.Context, t *testing.T, pool *pgxpool.Pool, status string, canceledBy, legacyBy *string) string { + t.Helper() + var id string + require.NoError(t, pool.QueryRow(ctx, insertLegacyExecution144, status, canceledBy, legacyBy).Scan(&id)) + return id +} + +func insertExchange144(ctx context.Context, pool *pgxpool.Pool, status string) (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) + VALUES ('123456789012', 'us-east-1', ARRAY['ri-1'], 'm5.large', 1, + 'offer-1', 'm5.xlarge', 1, $1) + RETURNING id::text`, status).Scan(&id) + return id, err +} + +func statusCheckDef144(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 actors144(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id string) (status string, canceledBy, legacyBy *string) { + t.Helper() + require.NoError(t, pool.QueryRow(ctx, + `SELECT status, canceled_by, `+legacyActor144+` FROM purchase_executions WHERE execution_id = $1`, id, + ).Scan(&status, &canceledBy, &legacyBy)) + return status, canceledBy, legacyBy +} + +func requireCheckViolation144(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 s144(v string) *string { return &v } + +// TestMigration_000144_ContractCanceledSpelling locks down the contract half of +// the 000089 rename (issue #47): legacy status rows are normalized, legacy-only +// actors are drained into canceled_by, both status CHECKs lose exactly the +// legacy spelling, writes from an older build that still sets the legacy actor +// column keep canceled_by authoritative, and the down restores the 000089 state. +func TestMigration_000144_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, contract144) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + peBefore := statusCheckDef144(ctx, t, pool, "purchase_executions_status_check") + riBefore := statusCheckDef144(ctx, t, pool, "ri_exchange_history_status_check") + require.Len(t, legacyInCheck144.FindAllString(peBefore, -1), 1, peBefore) + require.Len(t, legacyInCheck144.FindAllString(riBefore, -1), 1, riBefore) + + legacyOnly := insertExecution144(ctx, t, pool, legacyStatus144, nil, s144("legacy@example.com")) + // Already canonical status, actor only in the legacy column: only the + // drain reaches it (the status UPDATE does not touch the row). + drainOnly := insertExecution144(ctx, t, pool, canceled144, nil, s144("drain@example.com")) + both := insertExecution144(ctx, t, pool, canceled144, s144("new@example.com"), s144("new@example.com")) + conflicting := insertExecution144(ctx, t, pool, legacyStatus144, s144("kept@example.com"), s144("other@example.com")) + failed := insertExecution144(ctx, t, pool, "failed", nil, nil) + exchange, err := insertExchange144(ctx, pool, legacyStatus144) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, contract144)) + + // Data: every legacy status normalized, every legacy-only actor drained, + // canceled_by kept where both columns were set, other rows untouched. + for id, want := range map[string]struct{ status, actor string }{ + legacyOnly: {canceled144, "legacy@example.com"}, + drainOnly: {canceled144, "drain@example.com"}, + both: {canceled144, "new@example.com"}, + conflicting: {canceled144, "kept@example.com"}, + } { + status, canceledBy, _ := actors144(ctx, t, pool, id) + assert.Equal(t, want.status, status, id) + require.NotNil(t, canceledBy, id) + assert.Equal(t, want.actor, *canceledBy, id) + } + status, canceledBy, _ := actors144(ctx, t, pool, failed) + assert.Equal(t, "failed", status) + assert.Nil(t, canceledBy) + var exchangeStatus string + require.NoError(t, pool.QueryRow(ctx, `SELECT status FROM ri_exchange_history WHERE id = $1`, exchange).Scan(&exchangeStatus)) + assert.Equal(t, canceled144, exchangeStatus) + + // Schema: each CHECK is its 000089 definition minus the legacy spelling, + // nothing else, and rejects the legacy spelling. + assert.Equal(t, legacyInCheck144.ReplaceAllString(peBefore, ""), + statusCheckDef144(ctx, t, pool, "purchase_executions_status_check")) + assert.Equal(t, legacyInCheck144.ReplaceAllString(riBefore, ""), + statusCheckDef144(ctx, t, pool, "ri_exchange_history_status_check")) + _, err = pool.Exec(ctx, insertLegacyExecution144, legacyStatus144, nil, nil) + requireCheckViolation144(t, err, "purchase_executions_status_check") + _, err = insertExchange144(ctx, pool, legacyStatus144) + requireCheckViolation144(t, err, "ri_exchange_history_status_check") + + // An older build still writes the legacy actor column (its save path + // writes only that column). canceled_by must end up with the value that + // build reads, COALESCE(canceled_by, legacy), on insert and on update. + lateInsert := insertExecution144(ctx, t, pool, canceled144, nil, s144("late@example.com")) + _, canceledBy, _ = actors144(ctx, t, pool, lateInsert) + require.NotNil(t, canceledBy, "a legacy-only insert must be mirrored into canceled_by") + assert.Equal(t, "late@example.com", *canceledBy) + _, err = pool.Exec(ctx, `UPDATE purchase_executions SET `+legacyActor144+` = 'late-update@example.com' WHERE execution_id = $1`, failed) + require.NoError(t, err) + _, canceledBy, _ = actors144(ctx, t, pool, failed) + require.NotNil(t, canceledBy, "a legacy-only update must be mirrored into canceled_by") + assert.Equal(t, "late-update@example.com", *canceledBy) + _, err = pool.Exec(ctx, `UPDATE purchase_executions SET `+legacyActor144+` = 'stale@example.com' WHERE execution_id = $1`, both) + require.NoError(t, err) + _, canceledBy, _ = actors144(ctx, t, pool, both) + assert.Equal(t, "new@example.com", *canceledBy, "a legacy write never overrides an actor already in canceled_by") + + // New code writes canceled_by only. + newOnly := insertExecution144(ctx, t, pool, canceled144, s144("current@example.com"), nil) + + // Down: back to the 000089 CHECKs, trigger gone, legacy column backfilled. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + assert.Equal(t, peBefore, statusCheckDef144(ctx, t, pool, "purchase_executions_status_check")) + assert.Equal(t, riBefore, statusCheckDef144(ctx, t, pool, "ri_exchange_history_status_check")) + insertExecution144(ctx, t, pool, legacyStatus144, nil, nil) + _, err = insertExchange144(ctx, pool, legacyStatus144) + require.NoError(t, err, "down must accept the legacy spelling again") + _, _, legacyBy := actors144(ctx, t, pool, newOnly) + require.NotNil(t, legacyBy, "down must backfill the legacy actor column") + assert.Equal(t, "current@example.com", *legacyBy) + afterDown := insertExecution144(ctx, t, pool, canceled144, nil, s144("x@example.com")) + _, canceledBy, _ = actors144(ctx, t, pool, afterDown) + assert.Nil(t, canceledBy, "down must drop the mirror trigger") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, contract144), "up must re-apply cleanly after down") +} + +// canceled144 is the canonical spelling, kept local so this frozen +// migration test does not move if the config constant ever does. +const canceled144 = "canceled" + +// TestMigration_000144_ConcurrentLegacyWritersKeepAttribution races writers +// shaped like an older build (legacy actor column only) against the migration. +// Whichever side of the migration a write lands on, canceled_by must hold its +// actor afterwards: rows committed before the migration are drained, rows +// written after it are mirrored by the trigger, and the table lock leaves no +// gap between the two. +func TestMigration_000144_ConcurrentLegacyWritersKeepAttribution(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousMigrationVersion(t, contract144))) + + var migrated atomic.Bool + var afterMigration atomic.Int64 + stop := make(chan struct{}) + errs := make(chan error, 8) + var wg sync.WaitGroup + for range 4 { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + } + done := migrated.Load() + if _, err := pool.Exec(ctx, insertLegacyExecution144, canceled144, nil, "racer@example.com"); err != nil { + errs <- err + return + } + if done { + afterMigration.Add(1) + } + } + }() + } + + time.Sleep(200 * time.Millisecond) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, contract144)) + migrated.Store(true) + require.Eventually(t, func() bool { return afterMigration.Load() >= 20 }, time.Minute, 10*time.Millisecond) + close(stop) + wg.Wait() + close(errs) + for err := range errs { + require.NoError(t, err, "a legacy writer must not fail across the migration") + } + + var total, undrained int + require.NoError(t, pool.QueryRow(ctx, ` + SELECT count(*), count(*) FILTER (WHERE canceled_by IS NULL) + FROM purchase_executions WHERE `+legacyActor144+` IS NOT NULL`).Scan(&total, &undrained)) + require.Positive(t, total) + assert.Zero(t, undrained, "every legacy-only actor must be in canceled_by, of %d rows", total) +} diff --git a/internal/purchase/ladder_reconcile.go b/internal/purchase/ladder_reconcile.go index 509783241..abdc09145 100644 --- a/internal/purchase/ladder_reconcile.go +++ b/internal/purchase/ladder_reconcile.go @@ -80,7 +80,7 @@ func decideFiredTranche(chain []config.LadderChainNode, visible func(executionID return ladderVerdict{note: "failed without a no_submission proof (outcome unknown)"} } return failedIfPredecessorsProven(chain, ladderReasonNoSubmission) - case config.StatusCanceled, config.LegacyStatusCanceled: + case config.StatusCanceled: return failedIfPredecessorsProven(chain, ladderReasonApprovalCanceled) case config.StatusExpired: return failedIfPredecessorsProven(chain, ladderReasonApprovalExpired) diff --git a/internal/purchase/ladder_reconcile_integration_test.go b/internal/purchase/ladder_reconcile_integration_test.go index 36cbe15a1..7b9f32c81 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/internal/purchase/ladder_reconcile_test.go b/internal/purchase/ladder_reconcile_test.go index dae1b24c8..33536f543 100644 --- a/internal/purchase/ladder_reconcile_test.go +++ b/internal/purchase/ladder_reconcile_test.go @@ -33,7 +33,6 @@ func TestDecideFiredTranche(t *testing.T) { {"first attempt reaped, later attempts proven", []config.LadderChainNode{chainNode("e1", "failed", false), chainNode("e2", "failed", true), chainNode("e3", "failed", true)}, visibleAlways, "", ""}, {"every attempt proven", []config.LadderChainNode{chainNode("e1", "failed", true), chainNode("e2", "failed", true)}, visibleAlways, ladder.TrancheStatusFailed, "no_submission"}, {"canceled tail", []config.LadderChainNode{chainNode("e1", "canceled", false)}, visibleAlways, ladder.TrancheStatusFailed, "approval_canceled"}, - {"legacy spelling tail", []config.LadderChainNode{chainNode("e1", config.LegacyStatusCanceled, false)}, visibleAlways, ladder.TrancheStatusFailed, "approval_canceled"}, {"expired tail after unproven attempt", []config.LadderChainNode{chainNode("e1", "failed", false), chainNode("e2", "expired", false)}, visibleAlways, "", ""}, {"proof flag ignored on a non-failed row", []config.LadderChainNode{chainNode("e1", "running", true)}, visibleAlways, "", ""}, {"partially completed", []config.LadderChainNode{chainNode("e1", "partially_completed", false)}, visibleAlways, "", ""}, From fb1d6549dd32625b12e64659543d5044573d49c4 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sun, 25 Oct 2026 16:11:47 +0000 Subject: [PATCH 2/2] fix(config): drop the legacy canceled spelling from the history summary test The test came in from main (#1200) and seeds executions with LegacyStatusCanceled and queries it as an execution status. This branch removes that constant, and migration 000144 rejects the 'cancelled' status, so the seed would fail the narrowed CHECK. Seed and query only the canonical spelling. Co-Authored-By: Claude Opus 5.5 --- .../config/store_postgres_history_summary_integration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/config/store_postgres_history_summary_integration_test.go b/internal/config/store_postgres_history_summary_integration_test.go index a7afb7774..9b8204e12 100644 --- a/internal/config/store_postgres_history_summary_integration_test.go +++ b/internal/config/store_postgres_history_summary_integration_test.go @@ -80,7 +80,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) @@ -188,7 +188,7 @@ func TestIntegration_HistorySummaryAggregate_MatchesRowFetch(t *testing.T) { "all": {Provider: "aws", AccountIDs: []string{f.accountA}, Start: &start, End: &end}, } 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{