diff --git a/cmd/cleanup-lambda/main_integration_test.go b/cmd/cleanup-lambda/main_integration_test.go index 4bb7d3fb..c70390cd 100644 --- a/cmd/cleanup-lambda/main_integration_test.go +++ b/cmd/cleanup-lambda/main_integration_test.go @@ -17,7 +17,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/testhelpers" "github.com/stretchr/testify/require" @@ -31,7 +30,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"} ) // TestMain stops the PostgreSQL container that every test in this package shares diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 06e9f874..95c47496 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -141,12 +141,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 @@ -433,10 +428,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 @@ -1190,14 +1182,10 @@ func addNonCompletedRowToSummary(summary *HistorySummary, status string, n int) summary.TotalFailed += n case "expired": summary.TotalExpired += n - 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. default: return false } diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 61747740..5bd10fc0 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 44a6955c..9a369f74 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 45dbee3b..eb703027 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 7d7a3245..0c4b2682 100644 --- a/internal/api/handler_purchases_submit_dedupe_integration_test.go +++ b/internal/api/handler_purchases_submit_dedupe_integration_test.go @@ -118,7 +118,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 2bda5e71..a1865edb 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 0210854d..f37fcd1c 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 7f6b5605..47eb6c10 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 aa24c0ab..59de164f 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 911f015e..bc434bca 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, @@ -1658,7 +1658,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, @@ -1683,7 +1683,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, @@ -1725,7 +1725,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, @@ -1746,7 +1746,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, @@ -1770,7 +1770,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, @@ -1857,7 +1857,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, @@ -1892,7 +1892,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, @@ -1919,7 +1919,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, @@ -2160,7 +2160,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, @@ -2191,10 +2191,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 @@ -2265,11 +2263,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 @@ -2282,7 +2275,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 d53b7a88..658e088c 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_history_summary_integration_test.go b/internal/config/store_postgres_history_summary_integration_test.go index a7afb777..9b8204e1 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{ diff --git a/internal/config/store_postgres_ladder_resolve.go b/internal/config/store_postgres_ladder_resolve.go index 187e3878..b1f5388f 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 b16b9d96..7ed2bda6 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 78bf31ba..06f1d248 100644 --- a/internal/config/store_postgres_ladder_resolve_test.go +++ b/internal/config/store_postgres_ladder_resolve_test.go @@ -11,7 +11,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 25a76d93..994abe91 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 1725ee56..0c0261d5 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 40e76a31..87d985b5 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 f37fcd1f..00747bce 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 d9cda88d..39e6a98b 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 1e18b71c..00ff0b02 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 49b698a5..b4fbb4e9 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 4b43d6a0..88389415 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 00000000..6bb808a1 --- /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 00000000..37e94613 --- /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 00000000..3da2100f --- /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 50978324..abdc0914 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 36cbe15a..7b9f32c8 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 dae1b24c..33536f54 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, "", ""},