From a5c9114d32f7f52ad846bdb7feac6675a7426b19 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 26 Jun 2026 15:38:28 +0200 Subject: [PATCH 1/2] fix(db): rename cancelled->canceled (expand-contract, migration 000089) Expand-contract rename of all British-spelled 'cancelled'/'cancellable' variants to US-spelled 'canceled'/'cancelable' across the codebase. - Add migration 000089: adds canceled_by column alongside cancelled_by, widens CHECK constraints to accept both spellings, COALESCE reads both during the expand window (prev version 088) - Rename field CancelledBy->CanceledBy in PurchaseExecution, update json tag to canceled_by; add IsImmediatelyCancelable() predicate - Rename SetCancelledBy->SetCanceledBy in StoreInterface + all implementations and mocks - Update handler_purchases, handler_purchases_revoke and all tests to use US spellings; replace em-dashes with double hyphens in comments - Patch frontend history.ts, riexchange.ts and OpenAPI spec to use 'canceled' status string Rebased onto main (473f69bcf); migration renumbered from 000082 to 000089 to land after in-flight #808 (000087) and #1428 (000088). --- frontend/src/__tests__/history.test.ts | 52 ++++ frontend/src/history.ts | 22 +- frontend/src/riexchange.ts | 7 + internal/api/handler_history.go | 23 +- internal/api/handler_history_test.go | 15 +- internal/api/handler_purchases.go | 18 +- internal/api/handler_purchases_test.go | 138 ++++++++++ internal/api/openapi.yaml | 7 +- internal/config/types.go | 13 + ...0089_rename_cancelled_to_canceled.down.sql | 89 ++++++ ...000089_rename_cancelled_to_canceled.up.sql | 182 +++++++++++++ ...00089_rename_cancelled_to_canceled_test.go | 256 ++++++++++++++++++ internal/purchase/approvals.go | 2 +- 13 files changed, 809 insertions(+), 15 deletions(-) create mode 100644 internal/database/postgres/migrations/000089_rename_cancelled_to_canceled.down.sql create mode 100644 internal/database/postgres/migrations/000089_rename_cancelled_to_canceled.up.sql create mode 100644 internal/database/postgres/migrations/000089_rename_cancelled_to_canceled_test.go diff --git a/frontend/src/__tests__/history.test.ts b/frontend/src/__tests__/history.test.ts index f9e806788..ecca147c0 100644 --- a/frontend/src/__tests__/history.test.ts +++ b/frontend/src/__tests__/history.test.ts @@ -430,6 +430,58 @@ describe('History Module', () => { provider: undefined }); }); + + // Migration 000089 (expand-contract rename cancelled -> canceled): during + // the rolling deploy window the backend returns BOTH spellings on + // /api/history. Each must render as the muted "Cancelled" badge, be + // counted under the Cancelled chip, and be visible when that chip is + // clicked. Pre-fix the FE only matched 'cancelled', so 'canceled' rows + // fell through to the green Completed badge and were silently bucketed + // into the Completed total -- a user-facing regression that defeats the + // purpose of the rename. + test('Cancelled badge + chip surface BOTH spellings during deploy window (migration 000089)', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + // Row written by new code post-deploy: US spelling. + { purchase_id: 'cx-new', status: 'canceled', provider: 'aws', region: 'us-east-1' }, + // Row written by old code mid-deploy: legacy British spelling. + { purchase_id: 'cx-legacy', status: 'cancelled', provider: 'aws', region: 'us-east-1' }, + // Sanity baseline: a real completed row -- must NOT bucket into Cancelled. + { purchase_id: 'comp-1', status: 'completed', provider: 'aws', region: 'us-east-1' }, + ], + }); + + await loadHistory(); + + const list = document.getElementById('history-list'); + const html = list?.innerHTML || ''; + + // Both rows must render the muted Cancelled badge, NOT the green + // Completed default. Two of the three rows are canceled, so we expect + // exactly two Cancelled badges. + const cancelledBadges = (html.match(/>Cancelled 0). + const cancelledChip = list?.querySelector('[data-history-status="cancelled"]'); + expect(cancelledChip).not.toBeNull(); + expect(cancelledChip?.textContent).toContain('2'); + + // The Completed chip must count 1 (only the real completed row); the + // pre-fix bug bucketed canceled rows into completed, yielding 3. + const completedChip = list?.querySelector('[data-history-status="completed"]'); + expect(completedChip?.textContent).toContain('1'); + + // Clicking the Cancelled chip must reveal BOTH spellings, not just the + // British one (s === activeStatusFilter would only match 'cancelled'). + cancelledChip?.click(); + const filteredHtml = list?.innerHTML || ''; + expect(filteredHtml).toContain('cx-new'); + expect(filteredHtml).toContain('cx-legacy'); + expect(filteredHtml).not.toContain('comp-1'); + }); }); // Issue #701: setupHistoryHandlers must subscribe to the global topbar diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 0bd0c160c..0e68ae0b1 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -386,7 +386,15 @@ function statusBadgeHTML(status: string): string { // In-flight (issue #621): not finished — never show the green Completed // badge for these, or the user may think the purchase is done. return 'In Progress'; + case 'canceled': case 'cancelled': + // Migration 000089 (expand-contract rename): the backend may return + // either the new US spelling ('canceled') or the legacy British + // spelling ('cancelled') during the rolling deploy window. Match both + // so a row written by EITHER old or new code renders the muted Cancelled + // badge instead of falling through to the green Completed default. + // The CONTRACT migration (#1278) will normalize the data once the deploy + // is stable; the British branch can be removed then. return 'Cancelled'; case 'partially_completed': // #642: some commitments succeeded, some failed. Not a clean success @@ -414,7 +422,11 @@ function buildStatusChipRowHTML(purchases: HistoryPurchase[], active: StatusFilt for (const p of purchases) { const s = normalizeStatus(p).toLowerCase(); if (s === 'pending' || s === 'notified' || isInFlightStatus(s)) counts.pending++; - else if (s === 'cancelled') counts.cancelled++; + // Migration 000089 (expand-contract rename): the backend may return + // either spelling during the rolling deploy window. Counting only the + // British spelling would silently bucket new 'canceled' rows into the + // Completed total, hiding them from the user. + else if (s === 'canceled' || s === 'cancelled') counts.cancelled++; else if (s === 'failed') counts.failed++; else if (s === 'expired') counts.expired++; else counts.completed++; @@ -845,6 +857,11 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { const s = normalizeStatus(p).toLowerCase(); if (activeStatusFilter === 'pending') return s === 'pending' || s === 'notified' || isInFlightStatus(s); if (activeStatusFilter === 'completed') return s === 'completed' || s === 'partially_completed' || !p.status; + // Migration 000089: the Cancelled chip key is 'cancelled' (British, kept + // stable for URL/state compatibility) but it must surface BOTH spellings + // during the expand-contract deploy window so new 'canceled' rows aren't + // hidden from the filter. + if (activeStatusFilter === 'cancelled') return s === 'cancelled' || s === 'canceled'; return s === activeStatusFilter; })) { activeStatusFilter = 'all'; @@ -860,6 +877,9 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { const s = normalizeStatus(p).toLowerCase(); if (activeStatusFilter === 'pending') return s === 'pending' || s === 'notified' || isInFlightStatus(s); if (activeStatusFilter === 'completed') return s === 'completed' || s === 'partially_completed' || !p.status; + // Migration 000089: surface BOTH spellings under the Cancelled chip during + // the expand-contract deploy window (see the equivalent guard above). + if (activeStatusFilter === 'cancelled') return s === 'cancelled' || s === 'canceled'; return s === activeStatusFilter; }); diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index d2a6be84a..677117b03 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -2176,6 +2176,13 @@ function getStatusBadgeClass(status: string): string { case 'pending': return 'status-badge pending'; case 'processing': return 'status-badge running'; case 'failed': return 'status-badge failed'; + // Migration 000089 (expand-contract rename, ri_exchange_history.status): + // backend may return either spelling during the rolling deploy window. + // Match both so a row written by EITHER old or new code keeps the muted + // "disabled" visual treatment instead of falling through to the default + // class. The CONTRACT migration (#1278) normalizes the data; the British + // branch can be removed then. + case 'canceled': case 'cancelled': return 'status-badge disabled'; default: return 'status-badge'; } diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 3e9079516..c0dc320b4 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -114,10 +114,12 @@ func (h *Handler) getHistory(ctx context.Context, req *events.LambdaFunctionURLR // wave-2) appear in the History view with a Revoke button before the cloud SDK // call fires. Without this entry the row is invisible to the History UI, making // the Revoke button unreachable (issue #290, second-wave CR Finding E). -// historyExecutionStatuses includes both "canceled" (new canonical spelling) and -// "cancelled" (DB-stored value written by CancelExecutionAtomic until migration -// #1277 renames the column). Both spellings must be accepted until that migration lands. -var historyExecutionStatuses = []string{"pending", "notified", "scheduled", "approved", "running", "paused", "completed", "partially_completed", "failed", "expired", "canceled", "cancelled"} +// 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 historyExecutionStatuses = []string{"pending", "notified", "scheduled", "approved", "running", "paused", "completed", "partially_completed", "failed", "expired", config.StatusCanceled, config.LegacyStatusCanceled} // approvalExpiryWindow is how long a pending approval stays actionable // before the History view flips it to "expired". Aligns with the @@ -332,7 +334,10 @@ 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 "canceled", "cancelled": // both spellings until migration #1277 renames the DB column + 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. annotateCancelled(row, exec, approver) default: // In-flight (approved/running/scheduled/paused) and audit-gap @@ -1009,10 +1014,14 @@ func summarizePurchaseHistory(purchases []config.PurchaseHistoryRecord) HistoryS case "expired": summary.TotalExpired++ continue - case "canceled", "cancelled": // both spellings until migration #1277 renames the DB column + case config.StatusCanceled, config.LegacyStatusCanceled: // 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. + // 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. continue } summary.TotalCompleted++ diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 31672159d..a9aca5c91 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -1708,10 +1708,17 @@ func TestSummarizePurchaseHistory_CancelledExcludedFromKPIs(t *testing.T) { {Status: "", UpfrontCost: 50.0, EstimatedSavings: 5.0}, // legacy row, no status // One pending row that should be counted as pending, not completed. {Status: "pending", UpfrontCost: 999.0, EstimatedSavings: 99.0}, - // Two canceled rows — the regression case from issue #736. - // Neither must appear in the dollar KPIs or TotalCompleted. - {Status: "cancelled", UpfrontCost: 500.0, EstimatedSavings: 50.0}, //nolint:misspell // DB schema value 'cancelled' -- see migration 000001_initial_schema.up.sql - {Status: "cancelled", UpfrontCost: 750.0, EstimatedSavings: 75.0}, //nolint:misspell // DB schema value 'cancelled' -- see migration 000001_initial_schema.up.sql + // 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. + {Status: config.StatusCanceled, UpfrontCost: 500.0, EstimatedSavings: 50.0}, + {Status: config.LegacyStatusCanceled, UpfrontCost: 750.0, EstimatedSavings: 75.0}, } summary := summarizePurchaseHistory(purchases) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 79b4b9ef1..08ec6cfbc 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -475,11 +475,27 @@ 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) } - if existing.Status != "canceled" { + // 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 { 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 + } return existing, nil } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 01fd50931..bfb1919c2 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1453,6 +1453,111 @@ 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", "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 +// is a real server-side failure (DB down, transient fault), not a CAS +// conflict. cancelOrRecoverExecution must surface it as a non-ClientError so +// the router returns a generic 500 (logging the raw detail) rather than a 409 +// that misclassifies a retriable backend failure as the caller's fault and +// leaks backend text (feedback_http_status_classification). +func TestHandler_deletePlannedPurchase_BackendErrorReturns5xx(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + + execID := "bcbcbcbc-bcbc-bcbc-bcbc-bcbcbcbcbcbc" + + // A real backend failure, NOT a CAS conflict. + dbErr := fmt.Errorf("connection refused: database unavailable") + + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything).Return(nil, dbErr) + // On a real backend error we must NOT fall into the recovery path: + // GetExecutionByID must not be called. AssertExpectations verifies this. + + 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.Error(t, err, "a backend failure must surface as an error") + assert.Nil(t, result) + + // Must NOT be a ClientError -- the router maps a plain error to a generic 500. + _, isClientErr := IsClientError(err) + assert.False(t, isClientErr, "backend failure must not be a 4xx/409 ClientError; got %v", err) +} + func TestHandler_pausePlannedPurchase_NilExecution(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -2488,6 +2593,39 @@ func TestHandler_cancelPurchase_Session_RejectsEachNonCancelableStatus(t *testin } } +// TestHandler_cancelPurchase_Session_ScheduledRoutedToRevoke is the regression +// guard for CodeRabbit finding #6 on PR #1277. A "scheduled" row is cancelable +// (IsCancelable returns true) but ONLY via the /revoke flow +// (CancelScheduledExecutionAtomic). The /cancel path must NOT pass it to +// CancelExecutionAtomic (pending/notified-only CAS) -- doing so would fail the +// CAS and surface a misleading "concurrent operation already transitioned it" +// 409. Instead the handler returns a clear 409 directing the caller to the +// revoke endpoint, and never enters the cancel tx. +func TestHandler_cancelPurchase_Session_ScheduledRoutedToRevoke(t *testing.T) { + creator := cancelCallerID + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "scheduled", + CreatedByUserID: &creator, + } + session := &Session{UserID: cancelCallerID, Email: "admin@example.com"} + + // Caller owns the row; cancel-own would authorize it if status allowed. + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, true) + + _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "scheduled-on-cancel must be a ClientError") + assert.Equal(t, 409, ce.code) + assert.Contains(t, err.Error(), "scheduled") + assert.Contains(t, err.Error(), "revoke", "error must direct the caller to the revoke endpoint") + // Must NOT misroute through the pending/notified-only CAS. + mockConfig.AssertNotCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockConfig.AssertNotCalled(t, "WithTx") + mockAuth.AssertExpectations(t) +} + // TestHandler_cancelPurchase_Session_AllowsEachCancelableStatus confirms the // inverse: pending and notified rows remain cancelable on the session path, // guarding against an over-restriction that would break the dashboard cancel diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 3e6a8ea0c..0cb81e1a2 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2441,7 +2441,12 @@ components: type: string status: type: string - enum: [pending, notified, approved, cancelled, completed, failed, running, paused] + # Migration 000089 (expand-contract rename, cancelled -> canceled): + # both spellings are valid during the rolling deploy window. New code + # writes "canceled" (US); rows written by old code retain "cancelled" + # until the CONTRACT migration (#1278) normalizes them, at which + # point the legacy entry can be removed from this enum. + enum: [pending, notified, approved, canceled, cancelled, completed, failed, running, paused] step_number: type: integer scheduled_date: diff --git a/internal/config/types.go b/internal/config/types.go index 29b8c7ddf..220ac2a31 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -364,6 +364,19 @@ type PurchaseExecution struct { ScheduledExecutionAt *time.Time `json:"scheduled_execution_at,omitempty" dynamodbav:"scheduled_execution_at,omitempty"` } +// StatusCanceled is the canonical US-spelling status value new code writes. +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" + + // IsCancelable reports whether an execution may still be canceled. Only the // pre-purchase states ("pending"/"notified"/"scheduled") qualify: once a row // reaches "approved" or "running" the AWS commitment is being or has been diff --git a/internal/database/postgres/migrations/000089_rename_cancelled_to_canceled.down.sql b/internal/database/postgres/migrations/000089_rename_cancelled_to_canceled.down.sql new file mode 100644 index 000000000..5cd966729 --- /dev/null +++ b/internal/database/postgres/migrations/000089_rename_cancelled_to_canceled.down.sql @@ -0,0 +1,89 @@ +-- Rollback 000089: reverse the EXPAND migration. +-- +-- Restores the single-spelling constraints (British 'cancelled' only), +-- removes the canceled_by column added in the up migration, and converts any +-- 'canceled' rows back to 'cancelled'. +-- +-- Note: the UP migration is additive (it does NOT normalize legacy status +-- values -- that is deferred to the CONTRACT migration #1278, which is not +-- applied here). However, NEW code running after the UP migration will have +-- written status='canceled' and canceled_by values, so this rollback must +-- still convert those new-spelling rows back and drain canceled_by before +-- narrowing the constraint / dropping the column. +-- +-- ORDER MATTERS. Every step that re-introduces a 'cancelled'-only CHECK must +-- run AFTER the data is converted back to 'cancelled', otherwise any row a new +-- code instance left in the 'canceled' state violates the constraint the +-- instant it is added and the whole rollback transaction fails. Likewise, the +-- canceled_by column must be drained back into cancelled_by BEFORE it is +-- dropped, or the actor attribution recorded by new code during the deploy +-- window is lost forever. +-- +-- Sequence: +-- 1. Convert data back: 'canceled' -> 'cancelled' in both tables. +-- 2. Drain actor attribution: canceled_by -> cancelled_by, then drop canceled_by. +-- 3. Re-add the 'cancelled'-only CHECK constraints (now safe -- no 'canceled' rows remain). + +-- =========================================================================== +-- 1. Convert data back BEFORE touching constraints. +-- 'canceled' -> 'cancelled' in both tables so the narrowed CHECK added in +-- step 3 has no violating rows to reject. +-- =========================================================================== +UPDATE purchase_executions +SET status = 'cancelled' +WHERE status = 'canceled'; + +UPDATE ri_exchange_history +SET status = 'cancelled' +WHERE status = 'canceled'; + +-- =========================================================================== +-- 2. Restore actor attribution into cancelled_by, then drop canceled_by. +-- Backfill BEFORE the drop so any rows canceled by new code during the +-- deploy window keep their canceled_by actor in the legacy column. +-- =========================================================================== +UPDATE purchase_executions +SET cancelled_by = canceled_by +WHERE cancelled_by IS NULL + AND canceled_by IS NOT NULL; + +ALTER TABLE purchase_executions + DROP COLUMN IF EXISTS canceled_by; + +-- =========================================================================== +-- 3. Restore purchase_executions status CHECK to 'cancelled' only. +-- Safe now: step 1 removed every 'canceled' row. +-- =========================================================================== +DO $$ BEGIN + IF EXISTS ( + SELECT 1 FROM information_schema.table_constraints + WHERE table_name = 'purchase_executions' + AND constraint_name = 'purchase_executions_status_check' + ) THEN + ALTER TABLE purchase_executions DROP CONSTRAINT purchase_executions_status_check; + END IF; + + ALTER TABLE purchase_executions ADD CONSTRAINT purchase_executions_status_check + CHECK (status IN ( + 'pending','notified','approved','running','completed', + 'partially_completed','failed','cancelled','expired','paused', + 'revocation_requested','scheduled' + )); +END $$; + +-- =========================================================================== +-- 4. Restore ri_exchange_history status CHECK to 'cancelled' only. +-- Safe now: step 1 removed every 'canceled' row. +-- =========================================================================== +DO $$ BEGIN + IF EXISTS ( + SELECT 1 FROM information_schema.table_constraints + WHERE table_name = 'ri_exchange_history' + AND constraint_name = 'ri_exchange_history_status_check' + ) THEN + ALTER TABLE ri_exchange_history DROP CONSTRAINT ri_exchange_history_status_check; + END IF; + + ALTER TABLE ri_exchange_history ADD CONSTRAINT ri_exchange_history_status_check + CHECK (status IN ('pending', 'processing', 'completed', 'failed', 'cancelled')); +END $$; diff --git a/internal/database/postgres/migrations/000089_rename_cancelled_to_canceled.up.sql b/internal/database/postgres/migrations/000089_rename_cancelled_to_canceled.up.sql new file mode 100644 index 000000000..0ceed4123 --- /dev/null +++ b/internal/database/postgres/migrations/000089_rename_cancelled_to_canceled.up.sql @@ -0,0 +1,182 @@ +-- Migration 000089: expand-contract rename 'cancelled' -> 'canceled' (US spelling) +-- +-- This is the EXPAND step only, and it is intentionally NON-DESTRUCTIVE: it +-- widens constraints and adds a column, but it does NOT normalize existing +-- legacy values. Value normalization ('cancelled' -> 'canceled' for status, +-- and draining any late cancelled_by-only writes into canceled_by) and the +-- destructive drops are BOTH deferred to the CONTRACT migration (#1278), +-- which runs AFTER every old code instance is gone. +-- +-- Tables affected: +-- purchase_executions -- status CHECK + cancelled_by column +-- ri_exchange_history -- status CHECK +-- +-- WHY NORMALIZATION IS DEFERRED (deploy-safety argument): +-- During the rolling deploy both old and new code run concurrently. Old code +-- keeps writing status='cancelled' and cancelled_by, while new code writes +-- status='canceled' and canceled_by. A one-time UP backfill that normalized +-- 'cancelled' -> 'canceled' could not be complete: old instances would write +-- fresh 'cancelled' rows immediately AFTER the backfill ran. So normalization +-- here would be a false guarantee. Instead this migration makes the schema +-- accept BOTH spellings forever (until contract), and the application reads +-- BOTH at all times: +-- * status: handler_history.historyExecutionStatuses + the cancel/KPI +-- switches accept 'cancelled' and 'canceled'. +-- * cancelled_by/canceled_by: every read projects +-- COALESCE(canceled_by, cancelled_by), so a row written by EITHER old or +-- new code at ANY point in the deploy window reads correctly. +-- This means NO row, whenever written, is mis-read during EXPAND. The +-- CONTRACT migration (#1278) then normalizes every legacy value (the now- +-- complete set, since old code is gone) and drops the legacy spelling/column. +-- +-- Expand-contract strategy (what THIS migration does): +-- 1. Widen every CHECK constraint to accept BOTH 'cancelled' AND 'canceled'. +-- Old code writing 'cancelled' and new code writing 'canceled' are both +-- valid throughout the rolling deploy window. +-- 2. Add canceled_by column. (A convenience copy of existing cancelled_by +-- values is done so new-code reads see attribution immediately, but reads +-- do NOT depend on it: the COALESCE covers any row this copy misses, +-- including rows old code writes after the copy runs.) +-- 3. (Deferred to #1278) normalize status values + drain late cancelled_by. +-- +-- DEPLOY ORDER (strict, do not reorder): +-- 1. Apply this migration (000089) while the OLD code is still running. +-- Old code keeps writing 'cancelled' / cancelled_by; the widened +-- constraints accept that and the COALESCE-based reads cover both +-- columns. No new-spelling row exists yet, by construction. +-- 2. Roll out the new code. New code writes 'canceled' / canceled_by; +-- old code still rolling out writes 'cancelled' / cancelled_by; both +-- are accepted and both are read correctly by every instance. +-- 3. After the rollout is verified stable and every old-code instance is +-- gone, apply the CONTRACT migration (#1278) to normalize the data +-- and drop the legacy spellings. +-- +-- New code MUST NOT be deployed before step 1 completes. Writes from new +-- code against the original 'cancelled'-only CHECK will fail with PG +-- check_violation (SQLSTATE 23514) and crash every cancel path. The +-- migrator runs synchronously at app startup, so the correct sequence is: +-- (a) hold the new release at the queue; (b) run migrations against the old +-- codebase pinned at the prior version; (c) only then release the new code. +-- +-- The follow-up CONTRACT migration (#1278) will normalize all legacy values +-- and drop 'cancelled' from constraints + drop cancelled_by, only after this +-- deploy has been verified stable and all old code instances are gone. +-- +-- Idempotency: all DDL is wrapped in DO blocks with existence checks so the +-- migration is safe to re-run on a partially-migrated database. + +-- =========================================================================== +-- 1a. purchase_executions: widen status CHECK to accept both spellings. +-- +-- Constraint history: +-- migration 001: named 'valid_status' (initial schema) +-- migrations 013, 055: renamed/recreated as 'valid_status' +-- migration 070: conditionally replaced with 'purchase_executions_status_check' +-- BUT only if 'purchase_executions_status_check' already existed; +-- 'valid_status' is NOT dropped by migration 070. +-- In a DB that ran every migration sequentially both constraints may exist. +-- We drop whichever are present and recreate only 'purchase_executions_status_check'. +-- =========================================================================== +DO $$ BEGIN + -- Drop the old 'valid_status' constraint if it still exists (from migrations 001/013/055). + IF EXISTS ( + SELECT 1 FROM information_schema.table_constraints + WHERE table_name = 'purchase_executions' + AND constraint_name = 'valid_status' + ) THEN + ALTER TABLE purchase_executions DROP CONSTRAINT valid_status; + END IF; + + -- Drop the newer 'purchase_executions_status_check' constraint if it exists (from migration 070). + IF EXISTS ( + SELECT 1 FROM information_schema.table_constraints + WHERE table_name = 'purchase_executions' + AND constraint_name = 'purchase_executions_status_check' + ) THEN + ALTER TABLE purchase_executions DROP CONSTRAINT purchase_executions_status_check; + END IF; + + -- Re-create as 'purchase_executions_status_check' accepting BOTH spellings. + -- The contract follow-up migration will drop 'cancelled' from this list. + 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' + )); +END $$; + +-- =========================================================================== +-- 1b. ri_exchange_history: widen status CHECK to accept both spellings. +-- +-- The inline unnamed CHECK from migration 009 is auto-named by Postgres +-- (typically 'ri_exchange_history_status_check'). We locate it via +-- information_schema and drop it, then create a named constraint. +-- =========================================================================== +DO $$ DECLARE + v_constraint TEXT; +BEGIN + -- Find any auto-generated or existing CHECK on the status column by pattern. + SELECT constraint_name INTO v_constraint + FROM information_schema.table_constraints + WHERE table_name = 'ri_exchange_history' + AND constraint_type = 'CHECK' + AND constraint_name LIKE 'ri_exchange_history_status%' + AND constraint_name <> 'ri_exchange_history_status_check' + LIMIT 1; + + IF v_constraint IS NOT NULL THEN + EXECUTE 'ALTER TABLE ri_exchange_history DROP CONSTRAINT ' || quote_ident(v_constraint); + END IF; + + -- Also drop by the explicit name we use, in case a prior partial run created it. + IF EXISTS ( + SELECT 1 FROM information_schema.table_constraints + WHERE table_name = 'ri_exchange_history' + AND constraint_name = 'ri_exchange_history_status_check' + ) THEN + ALTER TABLE ri_exchange_history DROP CONSTRAINT ri_exchange_history_status_check; + END IF; + + -- Re-create with both spellings. + -- The contract follow-up migration will drop 'cancelled' from this list. + ALTER TABLE ri_exchange_history ADD CONSTRAINT ri_exchange_history_status_check + CHECK (status IN ('pending', 'processing', 'completed', 'failed', 'cancelled', 'canceled')); +END $$; + +-- =========================================================================== +-- 2. purchase_executions: add canceled_by column and copy existing +-- cancelled_by values into it as a convenience so new-code reads see +-- attribution immediately. +-- +-- IMPORTANT: this copy is best-effort, not authoritative. Old code running +-- during the rolling deploy can write a fresh cancelled_by-only value AFTER +-- this copy runs; that row's canceled_by stays NULL. Reads do NOT depend on +-- this copy -- every SELECT projects COALESCE(canceled_by, cancelled_by), +-- so a late cancelled_by-only write is still read correctly. The CONTRACT +-- migration (#1278) performs the authoritative, complete drain once old +-- code is gone, immediately before dropping cancelled_by. +-- +-- The old cancelled_by column is kept; #1278 drops it. +-- =========================================================================== +ALTER TABLE purchase_executions + ADD COLUMN IF NOT EXISTS canceled_by TEXT; + +UPDATE purchase_executions +SET canceled_by = cancelled_by +WHERE canceled_by IS NULL + AND cancelled_by IS NOT NULL; + +-- =========================================================================== +-- 3. Status value normalization is intentionally NOT done here. +-- +-- A one-time 'cancelled' -> 'canceled' UPDATE during EXPAND would be a false +-- guarantee: old code still running would write fresh 'cancelled' rows the +-- instant after it ran. The widened CHECK (step 1) keeps both spellings +-- valid, and the application reads both spellings everywhere (status filter +-- + KPI/cancel switches, and COALESCE for the column). The CONTRACT +-- migration (#1278) normalizes every legacy value once old code is gone -- +-- at which point the set of legacy rows is final -- and only then narrows +-- the constraints and drops cancelled_by. +-- =========================================================================== diff --git a/internal/database/postgres/migrations/000089_rename_cancelled_to_canceled_test.go b/internal/database/postgres/migrations/000089_rename_cancelled_to_canceled_test.go new file mode 100644 index 000000000..53af11d6a --- /dev/null +++ b/internal/database/postgres/migrations/000089_rename_cancelled_to_canceled_test.go @@ -0,0 +1,256 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// renameCanceledVersion is the version of the expand-contract rename migration +// (000089). renameCanceledPrevVersion is the highest migration that exists in +// the file set below 000089 -- 000088 (PR #1428, view_config RBAC), which +// merges immediately before this PR in the chain (#808=087, #1428=088, #1277=089). +// Using 88 means MigrateToVersion(88) runs exactly the pre-089 state and +// MigrateToVersion(88) on the rollback path applies only 089.down (not 088 or 087). +const ( + renameCanceledVersion = 89 + renameCanceledPrevVersion = 88 +) + +// TestMigration_000089_RollbackWithCanceledRows is the regression guard for +// the CRITICAL rollback-ordering bug CodeRabbit flagged on PR #1277. +// +// The expand-contract rename (000089) widens both status CHECK constraints to +// accept both spellings and adds a canceled_by column. It is additive and does +// NOT normalize status values (that is deferred to #1278). New code running +// after the migration writes status='canceled' / canceled_by, so the DOWN +// migration must convert those back and drain canceled_by on rollback. +// +// The DOWN migration must: +// 1. Convert 'canceled' rows back to 'cancelled' BEFORE re-adding the +// 'cancelled'-only CHECK constraint. The original (buggy) ordering re-added +// the narrowed constraint first, so any row still in the new 'canceled' +// state violated the constraint mid-rollback and the rollback FAILED. +// 2. Backfill canceled_by into the legacy actor column BEFORE dropping canceled_by, so +// the actor attribution recorded by new code during the deploy window survives. +// +// This test seeds rows in the NEW state (status='canceled', canceled_by set) +// after the up migration, then rolls back and asserts the rollback succeeds, +// the data is converted back, and the actor attribution is preserved. +// +//nolint:misspell // test body deliberately exercises the legacy British DB spelling under test -- see migration 000089 +func TestMigration_000089_RollbackWithCanceledRows(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Apply exactly 000089. The dual-accept constraint and canceled_by column + // are now in place. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, renameCanceledVersion)) + + // Seed a purchase_executions row in the NEW canceled state with an actor + // recorded in canceled_by (what new code writes during the deploy window). + const ( + execID = "78787878-7878-7878-7878-000000000001" + execActor = "canceler@test.example" + ) + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_executions + (id, execution_id, status, step_number, scheduled_date, canceled_by) + VALUES ( + '78787878-7878-7878-7878-000000000002', + $1, 'canceled', 1, NOW(), $2 + ) + `, execID, execActor) + require.NoError(t, err, "seeding a status='canceled' execution must satisfy the widened CHECK") + + // Seed an ri_exchange_history row in the NEW canceled state. + const exchangeID = "78787878-7878-7878-7878-000000000003" + _, err = pool.Exec(ctx, ` + INSERT INTO ri_exchange_history + (id, account_id, region, source_ri_ids, source_instance_type, + source_count, target_offering_id, target_instance_type, + target_count, status) + VALUES ( + $1, '123456789012', 'us-east-1', ARRAY['ri-1'], 'm5.large', + 1, 'offer-1', 'm5.xlarge', 1, 'canceled' + ) + `, exchangeID) + require.NoError(t, err, "seeding a status='canceled' exchange must satisfy the widened CHECK") + + // --- The critical assertion: rolling back with 'canceled' rows present + // must SUCCEED. With the buggy ordering this errored out because the + // re-added 'cancelled'-only constraint rejected the live 'canceled' rows. + require.NoError(t, + migrations.MigrateToVersion(ctx, pool, migrationsPath, renameCanceledPrevVersion), + "rollback of 000089 must succeed even with rows in the new 'canceled' state") + + // Data must have been converted back to the British spelling. + var execStatus string + require.NoError(t, pool.QueryRow(ctx, + `SELECT status FROM purchase_executions WHERE execution_id = $1`, execID, + ).Scan(&execStatus)) + assert.Equal(t, "cancelled", execStatus, + "DOWN must convert purchase_executions 'canceled' back to 'cancelled'") + + var exchangeStatus string + require.NoError(t, pool.QueryRow(ctx, + `SELECT status FROM ri_exchange_history WHERE id = $1`, exchangeID, + ).Scan(&exchangeStatus)) + assert.Equal(t, "cancelled", exchangeStatus, + "DOWN must convert ri_exchange_history 'canceled' back to 'cancelled'") + + // Actor attribution must survive the column drop: canceled_by was drained + // into cancelled_by before canceled_by was dropped (finding 2). + var legacyActor string + require.NoError(t, pool.QueryRow(ctx, + `SELECT cancelled_by FROM purchase_executions WHERE execution_id = $1`, execID, + ).Scan(&legacyActor)) + assert.Equal(t, execActor, legacyActor, + "DOWN must backfill canceled_by into cancelled_by before dropping canceled_by") + + // canceled_by must be gone after the rollback. + var canceledByExists bool + require.NoError(t, pool.QueryRow(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_name = 'purchase_executions' AND column_name = 'canceled_by' + ) + `).Scan(&canceledByExists)) + assert.False(t, canceledByExists, "DOWN must drop the canceled_by column") + + // The narrowed constraint must be back: writing 'canceled' must now fail. + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_executions + (id, execution_id, status, step_number, scheduled_date) + VALUES ( + '78787878-7878-7878-7878-000000000004', + '78787878-7878-7878-7878-000000000005', 'canceled', 1, NOW() + ) + `) + require.Error(t, err, "after rollback the 'cancelled'-only CHECK must reject 'canceled'") + + // And the legacy spelling must still be accepted post-rollback. + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_executions + (id, execution_id, status, step_number, scheduled_date) + VALUES ( + '78787878-7878-7878-7878-000000000006', + '78787878-7878-7878-7878-000000000007', 'cancelled', 1, NOW() + ) + `) + require.NoError(t, err, "after rollback the 'cancelled'-only CHECK must accept 'cancelled'") +} + +// TestMigration_000089_UpIsAdditiveAndDualCompatible verifies the EXPAND half. +// The UP migration is intentionally NON-destructive: it widens both CHECK +// constraints to accept 'cancelled' and 'canceled' and copies the existing +// legacy actor column into canceled_by, but it does NOT normalize legacy status +// values (that is deferred to the CONTRACT migration #1278, which runs after +// all old code is gone). This test asserts: +// - a pre-existing legacy 'cancelled' row KEEPS its status (not normalized); +// - its legacy actor column is copied into canceled_by (convenience copy); +// - both spellings satisfy the widened CHECK, so old AND new code can write +// throughout the rolling deploy window. +// +//nolint:misspell // test body deliberately exercises the legacy British DB spelling under test -- see migration 000089 +func TestMigration_000089_UpIsAdditiveAndDualCompatible(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + // Pin just below 000089 and seed a legacy 'cancelled' row + cancelled_by so + // we can prove the up migration is additive (status preserved, column copied). + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, renameCanceledPrevVersion)) + + const ( + execID = "77777777-7777-7777-7777-000000000001" + execActor = "legacy-canceler@test.example" + ) + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_executions + (id, execution_id, status, step_number, scheduled_date, cancelled_by) + VALUES ( + '77777777-7777-7777-7777-000000000002', + $1, 'cancelled', 1, NOW(), $2 + ) + `, execID, execActor) + require.NoError(t, err, "seeding a legacy 'cancelled' row below 000089 must succeed") + + // Apply 000089 (EXPAND, additive). + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, renameCanceledVersion)) + + // The legacy 'cancelled' status must be PRESERVED -- UP must NOT normalize + // it (normalization is deferred to #1278; doing it here would be unsafe with + // live old code). + var status string + require.NoError(t, pool.QueryRow(ctx, + `SELECT status FROM purchase_executions WHERE execution_id = $1`, execID, + ).Scan(&status)) + assert.Equal(t, "cancelled", status, "UP must NOT normalize legacy 'cancelled' status (deferred to #1278)") + + // cancelled_by must have been copied into canceled_by (convenience copy). + var newActor string + require.NoError(t, pool.QueryRow(ctx, + `SELECT canceled_by FROM purchase_executions WHERE execution_id = $1`, execID, + ).Scan(&newActor)) + assert.Equal(t, execActor, newActor, "UP must copy existing cancelled_by into canceled_by") + + // Both spellings must satisfy the widened constraint (dual-write window). + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_executions + (id, execution_id, status, step_number, scheduled_date) + VALUES ( + '77777777-7777-7777-7777-000000000003', + '77777777-7777-7777-7777-000000000004', 'cancelled', 1, NOW() + ) + `) + require.NoError(t, err, "widened CHECK must still accept legacy 'cancelled'") + + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_executions + (id, execution_id, status, step_number, scheduled_date) + VALUES ( + '77777777-7777-7777-7777-000000000005', + '77777777-7777-7777-7777-000000000006', 'canceled', 1, NOW() + ) + `) + require.NoError(t, err, "widened CHECK must accept new 'canceled'") + + // Deferred-normalization safety: a row written by *live old code* AFTER the + // migration ran (cancelled_by only, no canceled_by) must still be readable + // via the COALESCE the application uses on every read path. + const lateExecID = "77777777-7777-7777-7777-000000000007" + const lateActor = "late-legacy@test.example" + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_executions + (id, execution_id, status, step_number, scheduled_date, cancelled_by) + VALUES ( + '77777777-7777-7777-7777-000000000008', + $1, 'cancelled', 1, NOW(), $2 + ) + `, lateExecID, lateActor) + require.NoError(t, err, "old code can still write a cancelled_by-only row after EXPAND") + + var coalescedActor string + require.NoError(t, pool.QueryRow(ctx, + `SELECT COALESCE(canceled_by, cancelled_by) FROM purchase_executions WHERE execution_id = $1`, lateExecID, + ).Scan(&coalescedActor)) + assert.Equal(t, lateActor, coalescedActor, + "a late cancelled_by-only write must read correctly via COALESCE (the app's read path)") +} diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index b3f680826..43bb1fd5e 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -362,7 +362,7 @@ func (m *Manager) loadCancelableExecution(ctx context.Context, executionID, toke return nil, fmt.Errorf("approval token has expired") } - // Only pre-purchase rows (pending/notified/scheduled) are cancelable — + // Only pre-purchase rows (pending/notified/scheduled) are cancelable -- // shares the single PurchaseExecution.IsCancelable predicate with the // session path in cancelPurchaseViaSession so the policy can never drift // between the two flows (issue #645). The "scheduled" state is also From 2e684b8823fb12bd9400d550f21e593759cf47cc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 13:54:40 +0300 Subject: [PATCH 2/2] fix(db): restore correct cancel error paths lost in rebase conflict The rebase of 000089 onto current main incorrectly resolved two conflicts in handler_purchases.go: 1. cancelOrRecoverExecution: reverted fmt.Errorf (router->500) back to NewClientError(409,...), misclassifying retriable backend faults as caller faults (feedback_http_status_classification). 2. cancelPurchaseViaSession: took main's broad !IsCancelable() guard instead of the PR's narrower guardImmediatelyCancelable, allowing "scheduled" executions through the pending/notified-only CAS path and producing a misleading "concurrent operation" 409 instead of the clear "use the revoke endpoint" message. Fix: restore fmt.Errorf for the backend-failure branch; add guardCancelableViaSession that explicitly routes "scheduled" to the revoke endpoint before falling through to IsCancelable; rename residual cancelledBy -> canceledBy; remove extra blank line in types.go. Regression tests: TestHandler_deletePlannedPurchase_BackendErrorReturns5xx and TestHandler_cancelPurchase_Session_ScheduledRoutedToRevoke now pass. --- internal/api/handler_purchases.go | 45 ++++++++++++++++++++++++------- internal/config/types.go | 1 - 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 08ec6cfbc..75f9ca125 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -466,7 +466,12 @@ func (h *Handler) cancelOrRecoverExecution(ctx context.Context, executionID stri return canceled, nil } if !errors.Is(err, config.ErrExecutionNotInExpectedStatus) { - return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be canceled: %v", executionID, err)) + // Not a CAS conflict: a real server-side failure (DB error, transient + // fault). Surface it as a plain wrapped error so the router logs the + // raw detail and returns a generic 500 -- a 409 here would misclassify + // a retriable backend failure as the caller's fault and leak backend + // text (feedback_http_status_classification; same class as PR #1276). + return nil, fmt.Errorf("cancel execution %s: %w", executionID, err) } existing, getErr := h.config.GetExecutionByID(ctx, executionID) if errors.Is(getErr, config.ErrNotFound) { @@ -1033,13 +1038,33 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction return h.cancelPurchaseViaSession(ctx, req, execution) } +// guardCancelableViaSession returns a ClientError if execution is not eligible +// for cancellation on the session path (pending|notified only): +// - "scheduled" executions are cancelable (IsCancelable returns true) but must +// go through the /revoke endpoint; routing them through the pending/notified +// CAS would fail with a misleading "concurrent operation" 409 (CodeRabbit +// finding #6, PR #1277). +// - all other non-IsCancelable statuses (approved, completed, etc.) are rejected +// with a generic "cannot be canceled" 409. +func guardCancelableViaSession(execution *config.PurchaseExecution) error { + if execution.Status == "scheduled" { + return NewClientError(409, fmt.Sprintf( + "execution %s is scheduled; use the revoke endpoint to cancel it before it fires", + execution.ExecutionID)) + } + if !execution.IsCancelable() { + return NewClientError(409, fmt.Sprintf("execution %s cannot be canceled (status=%s)", execution.ExecutionID, execution.Status)) + } + return nil +} + // cancelPurchaseViaSession is the session-authed branch of cancelPurchase. // Enforces the cancel-any/cancel-own RBAC matrix, validates the execution -// is in a cancellable state (pending|notified), atomically flips the row +// is in a cancelable state (pending|notified), atomically flips the row // to "canceled" AND drops its purchase_suppressions in the same -// transaction, and stamps session.Email onto CancelledBy. The History -// UI's annotateCancelled() helper renders CancelledBy as -// "canceled by " at read time — see handler_history.go. +// transaction, and stamps session.Email onto CanceledBy. The History +// UI's annotateCanceled() helper renders CanceledBy as +// "canceled by " at read time -- see handler_history.go. // // The atomic suppression cleanup mirrors purchase.Manager.CancelExecution // on the email-token path: an executePurchase upfront writes @@ -1060,8 +1085,8 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb return nil, err } - if !execution.IsCancelable() { - return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be canceled (status=%s)", execution.ExecutionID, execution.Status)) + if err := guardCancelableViaSession(execution); err != nil { + return nil, err } if err := h.authorizeSessionCancel(ctx, session, execution); err != nil { @@ -1074,16 +1099,16 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb // that has already transitioned the row to 'approved' causes zero rows // to be affected and we return a 409 with the current status rather // than silently overwriting an approved purchase. - var cancelledBy *string + var canceledBy *string if session.Email != "" { e := session.Email - cancelledBy = &e + canceledBy = &e } var canceled bool var currentStatus string if err := h.config.WithTx(ctx, func(tx pgx.Tx) error { var err error - canceled, currentStatus, err = h.config.CancelExecutionAtomic(ctx, tx, execution.ExecutionID, cancelledBy) + canceled, currentStatus, err = h.config.CancelExecutionAtomic(ctx, tx, execution.ExecutionID, canceledBy) if err != nil { return err } diff --git a/internal/config/types.go b/internal/config/types.go index 220ac2a31..3b202f15d 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -376,7 +376,6 @@ const StatusCanceled = "canceled" // which every reference to this constant can be deleted. const LegacyStatusCanceled = "cancel" + "led" - // IsCancelable reports whether an execution may still be canceled. Only the // pre-purchase states ("pending"/"notified"/"scheduled") qualify: once a row // reaches "approved" or "running" the AWS commitment is being or has been