From abb16cb0f643b1ce462fbba9fdbedc2eb01355b0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 15:24:40 +0200 Subject: [PATCH 1/2] fix(purchases): atomic CAS cancel to close TOCTOU with concurrent approve (closes #671) Both cancel paths (session-authed dashboard cancel and email-token CancelExecution) used an out-of-transaction IsCancelable check followed by an unconditional SavePurchaseExecutionTx upsert. A concurrent approve that transitioned the row to 'approved' between the check and the upsert would be silently overwritten, leaving the DB at 'cancelled' while the AWS/Azure/GCP commitment is real and billable. Add CancelExecutionAtomic(ctx, tx, executionID, cancelledBy) to the store layer (interface + PostgresStore + all three mocks). The method issues a conditional UPDATE WHERE status IN ('pending','notified') inside the caller-provided tx, returning (false, currentStatus, nil) on zero rows affected so callers can 409 with the racing status visible. On success it returns (true, "cancelled", nil). Update both cancel paths to call CancelExecutionAtomic inside WithTx (paired with DeleteSuppressionsByExecutionTx on the success branch) and return 409 / a descriptive error when the CAS loses the race. Fix the misleading "optimistic-locking guard inside the tx" comment on handler_purchases.go that claimed a guard existed when it did not. Regression tests added for both paths: - TestManager_CancelExecution_RaceWithApprove (token path, purchase pkg) - TestHandler_cancelPurchase_Session_RaceWithApprove (session path, api pkg) Update all existing cancel tests to assert CancelExecutionAtomic rather than SavePurchaseExecution to reflect the new call site. --- internal/analytics/collector_test.go | 4 + internal/api/handler_purchases.go | 30 ++++--- internal/api/handler_purchases_test.go | 109 +++++++++++++++-------- internal/api/mocks_test.go | 11 +++ internal/config/interfaces.go | 7 ++ internal/config/store_postgres.go | 57 ++++++++++++ internal/mocks/stores.go | 6 ++ internal/purchase/approvals.go | 44 ++++++--- internal/purchase/approvals_test.go | 59 ++++++++---- internal/purchase/coverage_extra_test.go | 6 +- internal/purchase/mocks_test.go | 5 ++ internal/scheduler/scheduler_test.go | 5 ++ internal/server/test_helpers_test.go | 4 + 13 files changed, 272 insertions(+), 75 deletions(-) diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 2703b05b3..1741b5e5d 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -212,6 +212,10 @@ func (m *mockConfigStore) TransitionExecutionStatus(ctx context.Context, executi return nil, nil } +func (m *mockConfigStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (bool, string, error) { + return false, "", nil +} + func (m *mockConfigStore) SaveRIExchangeRecord(ctx context.Context, record *config.RIExchangeRecord) error { return nil } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index eaef69e9d..ee92c3dac 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -520,25 +520,35 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb return nil, err } - // Flip status + clear suppressions + stamp CancelledBy in one tx. - // An optimistic-locking guard inside the tx (status IN - // ('pending','notified')) prevents a concurrent approval from - // landing on top of us — if the status drifted, the UPDATE 0-rows - // the row count and we 409 cleanly without rolling back the entire - // flow into an inconsistent state. - execution.Status = "cancelled" + // Atomically flip status from pending/notified to cancelled + clear + // suppressions in one tx. CancelExecutionAtomic issues a conditional + // UPDATE WHERE status IN ('pending','notified'), so a concurrent approve + // 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 if session.Email != "" { - actor := session.Email - execution.CancelledBy = &actor + e := session.Email + cancelledBy = &e } + var cancelled bool + var currentStatus string if err := h.config.WithTx(ctx, func(tx pgx.Tx) error { - if err := h.config.SavePurchaseExecutionTx(ctx, tx, execution); err != nil { + var err error + cancelled, currentStatus, err = h.config.CancelExecutionAtomic(ctx, tx, execution.ExecutionID, cancelledBy) + if err != nil { return err } + if !cancelled { + return nil + } return h.config.DeleteSuppressionsByExecutionTx(ctx, tx, execution.ExecutionID) }); err != nil { return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", execution.ExecutionID, err)) } + if !cancelled { + return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: a concurrent operation already transitioned it to %q", execution.ExecutionID, currentStatus)) + } return map[string]string{"status": "cancelled"}, nil } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 0348dfef1..0fad9b0fa 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1588,47 +1588,43 @@ func sessionCancelReq() *events.LambdaFunctionURLRequest { // runSessionCancelAllowed asserts the success path of the session-authed // branch given a permission-matrix cell that should be allowed. The -// cancel commits in a single tx (SavePurchaseExecutionTx + -// DeleteSuppressionsByExecutionTx via WithTx); the mock store's WithTx -// default forwards fn(nil) and SavePurchaseExecutionTx default routes -// through SavePurchaseExecution, which we wire here. The suppression -// delete returns nil by default so we don't need to register it. +// cancel commits in a single tx via CancelExecutionAtomic + +// DeleteSuppressionsByExecutionTx; the mock store's WithTx default +// forwards fn(nil) and CancelExecutionAtomic default returns +// (true, "cancelled", nil) when no explicit expectation is registered. // -// Captures the saved execution so the caller can assert the audit-stamp -// invariants — primarily that CancelledBy is set to session.Email when -// the session has a non-empty email. cancelPurchase relies on this stamp -// for History UI attribution; if SavePurchaseExecution stops being -// called with the email-bearing copy the matrix tests would otherwise -// silently regress. +// Asserts the audit-stamp invariant: when session.Email is non-empty +// the cancelledBy pointer passed to CancelExecutionAtomic must carry +// that email so the DB column is stamped correctly for History UI +// attribution. func runSessionCancelAllowed(t *testing.T, exec *config.PurchaseExecution, session *Session, hasAny, hasOwn bool) { t.Helper() handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, hasAny, hasOwn) - var saved *config.PurchaseExecution - mockConfig.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")). + + // Capture the cancelledBy pointer passed to CancelExecutionAtomic + // so we can assert attribution was stamped correctly. + var capturedCancelledBy *string + mockConfig.On("CancelExecutionAtomic", mock.Anything, mock.Anything, cancelExecID, mock.Anything). Run(func(args mock.Arguments) { - saved = args.Get(1).(*config.PurchaseExecution) + if v, ok := args.Get(3).(*string); ok { + capturedCancelledBy = v + } }). - Return(nil) + Return(true, "cancelled", nil) result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.NoError(t, err) assert.Equal(t, "cancelled", result.(map[string]string)["status"]) - // Status flip + suppression cleanup are paired in one tx — the mock - // only sees the un-tx variants because of how MockConfigStore wires - // SavePurchaseExecutionTx → SavePurchaseExecution. Asserting the - // un-tx call ran is enough for the matrix tests; the atomicity - // itself is exercised by the live integration tests. - mockConfig.AssertCalled(t, "SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")) - require.NotNil(t, saved, "SavePurchaseExecution should have captured the execution") - assert.Equal(t, "cancelled", saved.Status) + // Verify the atomic cancel was called — this is the primary guard against + // regressions that skip the conditional UPDATE. + mockConfig.AssertCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, cancelExecID, mock.Anything) if session != nil && session.Email != "" { - require.NotNil(t, saved.CancelledBy, "CancelledBy must be stamped when session has an email") - assert.Equal(t, session.Email, *saved.CancelledBy, "CancelledBy must equal session.Email for audit attribution") + require.NotNil(t, capturedCancelledBy, "cancelledBy must be stamped when session has an email") + assert.Equal(t, session.Email, *capturedCancelledBy, "cancelledBy must equal session.Email for audit attribution") } // Verify the session-auth boundary actually fired — without this a // regression that bypassed ValidateSession (or stopped consulting - // HasPermissionAPI for non-admins) would silently still pass the - // status/audit assertions above. + // HasPermissionAPI for non-admins) would silently still pass. mockAuth.AssertExpectations(t) } @@ -1776,6 +1772,40 @@ func TestHandler_cancelPurchase_Session_AllowsEachCancelableStatus(t *testing.T) } } +// TestHandler_cancelPurchase_Session_RaceWithApprove is the regression +// guard for issue #671 on the session-authed cancel path. When a concurrent +// approve transitions the execution out of pending/notified before the +// conditional UPDATE runs, CancelExecutionAtomic returns +// (false, "approved", nil) and the handler must 409 with the racing status +// rather than silently overwriting the approved row. +func TestHandler_cancelPurchase_Session_RaceWithApprove(t *testing.T) { + creator := cancelCallerID + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "pending", // status at fetch time + CreatedByUserID: &creator, + } + session := &Session{UserID: cancelCallerID, Role: "admin", Email: "admin@example.com"} + + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) + // Simulate concurrent approve winning between IsCancelable check and + // the conditional UPDATE inside the tx. + mockConfig.On("CancelExecutionAtomic", mock.Anything, mock.Anything, cancelExecID, mock.Anything). + Return(false, "approved", nil) + + _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") + require.Error(t, err) + var ce *clientError + require.ErrorAs(t, err, &ce) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.message, "approved", "409 body must surface the racing status") + assert.Contains(t, ce.message, "concurrent", "409 body must mention the concurrent operation") + // Suppression cleanup must NOT have been called because the atomic + // UPDATE returned zero rows — the approve path owns the execution now. + mockConfig.AssertNotCalled(t, "DeleteSuppressionsByExecutionTx", mock.Anything, mock.Anything, mock.Anything) + mockAuth.AssertExpectations(t) +} + func TestHandler_cancelPurchase_Session_LegacyNullCreator_NonAdminRejected(t *testing.T) { // Pre-migration row: created_by_user_id is NULL. cancel-own can't // match a NULL creator, so a non-admin must be rejected. The email @@ -1848,12 +1878,17 @@ func TestHandler_cancelPurchase_DeepLink_AdminBypassesContactEmailGate(t *testin session := &Session{UserID: cancelCallerID, Role: "admin", Email: "admin@example.com"} handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) - var saved *config.PurchaseExecution - mockConfig.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")). + + // Capture cancelledBy to verify the audit-stamp is passed to the + // atomic UPDATE. + var capturedCancelledBy *string + mockConfig.On("CancelExecutionAtomic", mock.Anything, mock.Anything, cancelExecID, mock.Anything). Run(func(args mock.Arguments) { - saved = args.Get(1).(*config.PurchaseExecution) + if v, ok := args.Get(3).(*string); ok { + capturedCancelledBy = v + } }). - Return(nil) + Return(true, "cancelled", nil) // Token IS present in the URL — the deep-link flow always sends one. // The fix's whole point is that the admin session takes the @@ -1862,13 +1897,11 @@ func TestHandler_cancelPurchase_DeepLink_AdminBypassesContactEmailGate(t *testin require.NoError(t, err, "admin clicking Cancel from notification email must succeed even when no contact_email is configured") assert.Equal(t, "cancelled", result.(map[string]string)["status"]) - require.NotNil(t, saved, "session-authed branch must commit the status flip") - assert.Equal(t, "cancelled", saved.Status) - require.NotNil(t, saved.CancelledBy, "session-authed branch must stamp CancelledBy") - assert.Equal(t, session.Email, *saved.CancelledBy) + require.NotNil(t, capturedCancelledBy, "session-authed branch must stamp cancelledBy") + assert.Equal(t, session.Email, *capturedCancelledBy) // Critical security assertion: the token branch's contact_email gate - // (authorizeApprovalAction → GetGlobalConfig → resolveApprovalRecipients) + // (authorizeApprovalAction -> GetGlobalConfig -> resolveApprovalRecipients) // was NOT consulted. If a regression re-routed admins through the // token path, GetGlobalConfig would fire because the gate fetches // the global notification email; asserting it didn't is the cleanest @@ -1895,7 +1928,9 @@ func TestHandler_cancelPurchase_DeepLink_CancelOwnBypassesContactEmailGate(t *te session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false /*hasAny*/, true /*hasOwn*/) - mockConfig.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + // CancelExecutionAtomic is called by the session-authed branch. + mockConfig.On("CancelExecutionAtomic", mock.Anything, mock.Anything, cancelExecID, mock.Anything). + Return(true, "cancelled", nil) result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "deep-link-token") require.NoError(t, err) diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index e0c0956cb..a3762ba86 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -548,6 +548,17 @@ func (m *MockConfigStore) ListActiveSuppressions(ctx context.Context) ([]config. return args.Get(0).([]config.PurchaseSuppression), args.Error(1) } +func (m *MockConfigStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (bool, string, error) { + if !m.isExpected("CancelExecutionAtomic") { + // Default: succeed, returning "cancelled". Tests that exercise the + // race (zero-rows) path register an explicit expectation that + // returns (false, , nil). + return true, "cancelled", nil + } + args := m.Called(ctx, tx, executionID, cancelledBy) + return args.Bool(0), args.String(1), args.Error(2) +} + func (m *MockConfigStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, execution *config.PurchaseExecution) error { if !m.isExpected("SavePurchaseExecutionTx") { // Default to calling SavePurchaseExecution so tests that only diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 0d214cec0..e81f98b4d 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -65,6 +65,13 @@ type StoreInterface interface { ListPendingExecutionIDsForAccount(ctx context.Context, accountID string) ([]string, error) CleanupOldExecutions(ctx context.Context, retentionDays int) (int64, error) TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string) (*PurchaseExecution, error) + // CancelExecutionAtomic atomically flips status from pending/notified to + // cancelled, setting cancelled_by. Returns (true, "cancelled", nil) on + // success and (false, currentStatus, nil) when zero rows were affected + // (the execution had already been approved or otherwise transitioned). + // Must be called inside a WithTx block so the suppression cleanup and + // the status flip commit atomically. + CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (cancelled bool, currentStatus string, err error) // Purchase history SavePurchaseHistory(ctx context.Context, record *PurchaseHistoryRecord) error diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index eddfccd6c..eb348ada6 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -802,6 +802,63 @@ func (s *PostgresStore) TransitionExecutionStatus(ctx context.Context, execution return &records[0], nil } +// CancelExecutionAtomic atomically transitions an execution from +// pending or notified to cancelled, setting cancelled_by to the supplied +// actor (NULL when actor is nil). The UPDATE is conditional on +// status IN ('pending','notified') so a concurrent approve that has +// already transitioned the row to 'approved' causes zero rows to be +// affected and the method returns (false, currentStatus, nil) with the +// live status fetched via a follow-up SELECT. Returns (true, "cancelled", +// nil) on success and (false, "", err) on a real DB error. +// +// Callers must run the suppression cleanup in the same transaction; use +// the WithTx + DeleteSuppressionsByExecutionTx pairing at the call site +// exactly as the old SavePurchaseExecutionTx path did, except now the +// status guard is inside the UPDATE rather than checked optimistically +// before entering the tx. +func (s *PostgresStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (cancelled bool, currentStatus string, err error) { + q := ` + UPDATE purchase_executions + SET status = 'cancelled', + cancelled_by = $2, + updated_at = NOW() + WHERE execution_id = $1 + AND status IN ('pending', 'notified') + RETURNING status + ` + rows, err := tx.Query(ctx, q, executionID, cancelledBy) + if err != nil { + return false, "", fmt.Errorf("failed to cancel execution: %w", err) + } + defer rows.Close() + + if rows.Next() { + var st string + if scanErr := rows.Scan(&st); scanErr != nil { + return false, "", fmt.Errorf("failed to scan cancel result: %w", scanErr) + } + if rowsErr := rows.Err(); rowsErr != nil { + return false, "", fmt.Errorf("failed to iterate cancel result: %w", rowsErr) + } + return true, st, nil + } + if rowsErr := rows.Err(); rowsErr != nil { + return false, "", fmt.Errorf("failed to iterate cancel result: %w", rowsErr) + } + + // Zero rows affected: execution either does not exist or has already + // transitioned out of pending/notified. Surface the current status so + // callers can return a meaningful 409 body. + existing, existErr := s.GetExecutionByID(ctx, executionID) + if existErr != nil { + return false, "", fmt.Errorf("execution not found or db error: %w", existErr) + } + if existing == nil { + return false, "", fmt.Errorf("execution not found: %s", executionID) + } + return false, existing.Status, nil +} + // GetExecutionsByStatuses returns executions whose Status is any of the // supplied values, newest-first, capped at `limit`. Used by the History // handler to merge pending/failed/expired rows alongside completed purchases diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 1e8cc25de..89c531ea9 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -117,6 +117,12 @@ func (m *MockConfigStore) TransitionExecutionStatus(ctx context.Context, executi return args.Get(0).(*config.PurchaseExecution), args.Error(1) } +// CancelExecutionAtomic mocks the CancelExecutionAtomic operation. +func (m *MockConfigStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (bool, string, error) { + args := m.Called(ctx, tx, executionID, cancelledBy) + return args.Bool(0), args.String(1), args.Error(2) +} + // GetPendingExecutions mocks the GetPendingExecutions operation func (m *MockConfigStore) GetPendingExecutions(ctx context.Context) ([]config.PurchaseExecution, error) { args := m.Called(ctx) diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 2891993a5..916be1c10 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -133,31 +133,53 @@ func (m *Manager) ApproveAndExecute(ctx context.Context, executionID, actor stri // caller (HTTP path: authorizeApprovalAction; SQS path: // verifyAsyncApprovalActor) before reaching here. Same empty-actor // rationale as ApproveExecution. +// +// Concurrency: CancelExecutionAtomic uses a conditional UPDATE WHERE +// status IN ('pending','notified') so a concurrent approve that wins +// the race causes zero rows to be affected and the caller receives a +// clean error with the current status rather than silently overwriting +// the approved row. This is the token/email-link cancel analogue of +// the atomic guard TransitionExecutionStatus provides for ApproveAndExecute. func (m *Manager) CancelExecution(ctx context.Context, executionID, token, actor string) error { logging.Infof("Cancelling execution: %s", executionID) - execution, err := m.loadCancelableExecution(ctx, executionID, token) - if err != nil { + if _, err := m.loadCancelableExecution(ctx, executionID, token); err != nil { return err } - // Update status + attribution — see ApproveExecution for the empty-actor - // nil-vs-empty-string rationale. Paired with DeleteSuppressionsByExecution - // in the same transaction so the status flip and the un-suppression - // commit atomically — a crash between the two would otherwise leave the - // rec-list hiding capacity the user already cancelled. - execution.Status = "cancelled" + // Build the nullable cancelled_by pointer — see ApproveExecution for + // the nil-vs-empty-string rationale. + var cancelledBy *string if actor != "" { a := actor - execution.CancelledBy = &a + cancelledBy = &a } + + // Atomic conditional UPDATE + suppression cleanup in one transaction. + // CancelExecutionAtomic flips status only when status IN + // ('pending','notified') so a concurrent approve that has already + // transitioned the row causes zero rows affected and we surface a 409. + var cancelled bool + var currentStatus string if err := m.config.WithTx(ctx, func(tx pgx.Tx) error { - if err := m.config.SavePurchaseExecutionTx(ctx, tx, execution); err != nil { + var err error + cancelled, currentStatus, err = m.config.CancelExecutionAtomic(ctx, tx, executionID, cancelledBy) + if err != nil { return err } + if !cancelled { + // Row already transitioned (concurrent approve/cancel won the + // race). Return early without touching suppressions — the other + // operation owns the execution state now. + return nil + } return m.config.DeleteSuppressionsByExecutionTx(ctx, tx, executionID) }); err != nil { - return fmt.Errorf("failed to save execution: %w", err) + return fmt.Errorf("failed to cancel execution: %w", err) + } + + if !cancelled { + return fmt.Errorf("execution %s cannot be cancelled: concurrent operation already transitioned it to %q", executionID, currentStatus) } logging.Infof("Execution %s cancelled", executionID) diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index 44f20acc0..e4f3ee3ff 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -306,7 +306,9 @@ func TestManager_CancelExecution(t *testing.T) { } mockStore.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) - mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + // WithTx passes nil as the tx sentinel in tests; empty actor -> nil cancelledBy. + mockStore.On("CancelExecutionAtomic", ctx, mock.Anything, "exec-123", (*string)(nil)). + Return(true, "cancelled", nil) manager := &Manager{ config: mockStore, @@ -407,12 +409,9 @@ func TestManager_CancelExecution_RejectsNonCancelableStatus(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "execution cannot be cancelled") assert.Contains(t, err.Error(), status) - // Status guard must fire before any persistence — a rejected - // cancel must not flip the row or drop suppressions. - // SavePurchaseExecutionTx forwards to SavePurchaseExecution in - // the mock, so this single assertion covers both the tx and - // non-tx write paths. - mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + // Status guard fires before the atomic UPDATE — a rejected + // cancel must never reach CancelExecutionAtomic. + mockStore.AssertNotCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything) mockStore.AssertExpectations(t) }) } @@ -438,12 +437,9 @@ func TestManager_CancelExecution_AllowsCancelableStatus(t *testing.T) { ApprovalToken: "valid-token", } mockStore.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) - var saved *config.PurchaseExecution - mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). - Run(func(args mock.Arguments) { - saved = args.Get(1).(*config.PurchaseExecution) - }). - Return(nil) + // CancelExecutionAtomic is called inside WithTx (nil tx sentinel in tests). + mockStore.On("CancelExecutionAtomic", ctx, mock.Anything, "exec-123", (*string)(nil)). + Return(true, "cancelled", nil) manager := &Manager{ config: mockStore, @@ -453,8 +449,7 @@ func TestManager_CancelExecution_AllowsCancelableStatus(t *testing.T) { err := manager.CancelExecution(ctx, "exec-123", "valid-token", "") require.NoError(t, err) - require.NotNil(t, saved, "cancel should persist the execution") - assert.Equal(t, "cancelled", saved.Status) + mockStore.AssertCalled(t, "CancelExecutionAtomic", ctx, mock.Anything, "exec-123", (*string)(nil)) mockStore.AssertExpectations(t) }) } @@ -670,6 +665,38 @@ func TestOrphanExecutionError_RecLevelAccountIDPreventsOrphan(t *testing.T) { assert.NoError(t, err, "rec-level CloudAccountID must prevent orphan classification") } +// TestManager_CancelExecution_RaceWithApprove is the regression guard for +// issue #671: when a concurrent approve wins the race and transitions the +// execution to 'approved' before the cancel's conditional UPDATE runs, +// CancelExecutionAtomic returns (false, "approved", nil). CancelExecution +// must surface a clean error containing the racing status rather than +// silently overwriting the approved row. +func TestManager_CancelExecution_RaceWithApprove(t *testing.T) { + ctx := context.Background() + manager, store, _ := newApproveManager(t) + + execution := &config.PurchaseExecution{ + ExecutionID: "exec-raced", + Status: "pending", // status at load time + ApprovalToken: "valid-token", + } + store.On("GetExecutionByID", ctx, "exec-raced").Return(execution, nil) + // Simulate: concurrent approve won between our IsCancelable check and + // the atomic UPDATE. The DB row is now 'approved' so zero rows affected. + store.On("CancelExecutionAtomic", ctx, mock.Anything, "exec-raced", (*string)(nil)). + Return(false, "approved", nil) + + err := manager.CancelExecution(ctx, "exec-raced", "valid-token", "") + require.Error(t, err) + assert.Contains(t, err.Error(), "approved", "error must surface the racing status") + assert.Contains(t, err.Error(), "concurrent", "error must mention the concurrent operation") + // The approve is already in flight — the DB row must not have been + // overwritten by the cancel. AssertExpectations verifies that + // CancelExecutionAtomic was called (confirming the guard reached the DB) + // and that no further writes landed. + store.AssertExpectations(t) +} + // TestManager_CancelExecution_ExpiredToken is the cancel-path regression // guard for issue #397. func TestManager_CancelExecution_ExpiredToken(t *testing.T) { @@ -688,6 +715,6 @@ func TestManager_CancelExecution_ExpiredToken(t *testing.T) { err := manager.CancelExecution(ctx, "exec-cancel-expired", "valid-token", "") require.Error(t, err) assert.Contains(t, err.Error(), "expired") - store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + store.AssertNotCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything) store.AssertExpectations(t) } diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index 2ae5ff110..8d36e3343 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -388,7 +388,11 @@ func TestProcessMessage_CancelHappyPath(t *testing.T) { // execution; mock returns it twice. mockStore.On("GetExecutionByID", ctx, "exec-cancel").Return(exec, nil).Twice() mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) - mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + // CancelExecutionAtomic is called inside WithTx (nil tx sentinel in + // tests); actor_email is non-empty so cancelledBy is non-nil. + actor := "owner@example.com" + mockStore.On("CancelExecutionAtomic", ctx, mock.Anything, "exec-cancel", &actor). + Return(true, "cancelled", nil) manager := &Manager{ config: mockStore, diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index 96b0163d8..406beb9f3 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -505,6 +505,11 @@ func (m *MockConfigStore) CreateSuppression(_ context.Context, _ *config.Purchas func (m *MockConfigStore) CreateSuppressionTx(_ context.Context, _ pgx.Tx, _ *config.PurchaseSuppression) error { return nil } +func (m *MockConfigStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (bool, string, error) { + args := m.Called(ctx, tx, executionID, cancelledBy) + return args.Bool(0), args.String(1), args.Error(2) +} + func (m *MockConfigStore) DeleteSuppressionsByExecution(_ context.Context, _ string) error { return nil } diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 051bb83cd..3bd167896 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -211,6 +211,11 @@ func (m *MockConfigStore) TransitionExecutionStatus(ctx context.Context, executi return args.Get(0).(*config.PurchaseExecution), args.Error(1) } +func (m *MockConfigStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (bool, string, error) { + args := m.Called(ctx, tx, executionID, cancelledBy) + return args.Bool(0), args.String(1), args.Error(2) +} + func (m *MockConfigStore) SaveRIExchangeRecord(ctx context.Context, record *config.RIExchangeRecord) error { args := m.Called(ctx, record) return args.Error(0) diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 958d9dd3f..917e485ab 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -111,6 +111,10 @@ func (m *mockConfigStoreForHealth) TransitionExecutionStatus(ctx context.Context return nil, nil } +func (m *mockConfigStoreForHealth) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (bool, string, error) { + return false, "", nil +} + func (m *mockConfigStoreForHealth) SaveRIExchangeRecord(ctx context.Context, record *config.RIExchangeRecord) error { return nil } From 892b13d9577e40cfc59e9be1135ef9c2aed9d7a8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 16:05:06 +0200 Subject: [PATCH 2/2] fix(cancel): surface tx errors as 5xx, tighten suppression-cleanup assertions - handler_purchases.go: return fmt.Errorf (5xx) for WithTx failures instead of NewClientError(409); only the !cancelled path is a true conflict - handler_purchases_test.go: add DeleteSuppressionsByExecutionTx expectation + AssertCalled in runSessionCancelAllowed to cover the transactional contract - purchase/mocks_test.go: make DeleteSuppressionsByExecutionTx use m.Called so testify tracks calls for AssertNotCalled/AssertExpectations - approvals_test.go, coverage_extra_test.go: register the expectation on all success-path cancel tests; add AssertNotCalled on race test to guard against accidental cleanup writes when CAS misses Addresses CodeRabbit review on PR #674. --- internal/api/handler_purchases.go | 2 +- internal/api/handler_purchases_test.go | 5 +++++ internal/purchase/approvals_test.go | 8 ++++++++ internal/purchase/coverage_extra_test.go | 3 +++ internal/purchase/mocks_test.go | 4 ++-- 5 files changed, 19 insertions(+), 3 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index ee92c3dac..c00644a6f 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -544,7 +544,7 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb } return h.config.DeleteSuppressionsByExecutionTx(ctx, tx, execution.ExecutionID) }); err != nil { - return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", execution.ExecutionID, err)) + return nil, fmt.Errorf("cancel execution %s: %w", execution.ExecutionID, err) } if !cancelled { return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: a concurrent operation already transitioned it to %q", execution.ExecutionID, currentStatus)) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 0fad9b0fa..111c483b4 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1611,6 +1611,9 @@ func runSessionCancelAllowed(t *testing.T, exec *config.PurchaseExecution, sessi } }). Return(true, "cancelled", nil) + // When cancel succeeds the transaction must also clean up suppressions. + mockConfig.On("DeleteSuppressionsByExecutionTx", mock.Anything, mock.Anything, cancelExecID). + Return(nil) result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.NoError(t, err) @@ -1618,6 +1621,8 @@ func runSessionCancelAllowed(t *testing.T, exec *config.PurchaseExecution, sessi // Verify the atomic cancel was called — this is the primary guard against // regressions that skip the conditional UPDATE. mockConfig.AssertCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, cancelExecID, mock.Anything) + // Verify suppression cleanup ran within the same transaction. + mockConfig.AssertCalled(t, "DeleteSuppressionsByExecutionTx", mock.Anything, mock.Anything, cancelExecID) if session != nil && session.Email != "" { require.NotNil(t, capturedCancelledBy, "cancelledBy must be stamped when session has an email") assert.Equal(t, session.Email, *capturedCancelledBy, "cancelledBy must equal session.Email for audit attribution") diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index e4f3ee3ff..473a55eaf 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -309,6 +309,8 @@ func TestManager_CancelExecution(t *testing.T) { // WithTx passes nil as the tx sentinel in tests; empty actor -> nil cancelledBy. mockStore.On("CancelExecutionAtomic", ctx, mock.Anything, "exec-123", (*string)(nil)). Return(true, "cancelled", nil) + mockStore.On("DeleteSuppressionsByExecutionTx", ctx, mock.Anything, "exec-123"). + Return(nil) manager := &Manager{ config: mockStore, @@ -440,6 +442,9 @@ func TestManager_CancelExecution_AllowsCancelableStatus(t *testing.T) { // CancelExecutionAtomic is called inside WithTx (nil tx sentinel in tests). mockStore.On("CancelExecutionAtomic", ctx, mock.Anything, "exec-123", (*string)(nil)). Return(true, "cancelled", nil) + // Suppression cleanup must follow a successful atomic cancel. + mockStore.On("DeleteSuppressionsByExecutionTx", ctx, mock.Anything, "exec-123"). + Return(nil) manager := &Manager{ config: mockStore, @@ -450,6 +455,7 @@ func TestManager_CancelExecution_AllowsCancelableStatus(t *testing.T) { err := manager.CancelExecution(ctx, "exec-123", "valid-token", "") require.NoError(t, err) mockStore.AssertCalled(t, "CancelExecutionAtomic", ctx, mock.Anything, "exec-123", (*string)(nil)) + mockStore.AssertCalled(t, "DeleteSuppressionsByExecutionTx", ctx, mock.Anything, "exec-123") mockStore.AssertExpectations(t) }) } @@ -695,6 +701,8 @@ func TestManager_CancelExecution_RaceWithApprove(t *testing.T) { // CancelExecutionAtomic was called (confirming the guard reached the DB) // and that no further writes landed. store.AssertExpectations(t) + // When the CAS misses, suppression cleanup must never fire. + store.AssertNotCalled(t, "DeleteSuppressionsByExecutionTx", mock.Anything, mock.Anything, mock.Anything) } // TestManager_CancelExecution_ExpiredToken is the cancel-path regression diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index 8d36e3343..c86bdd0d9 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -393,6 +393,9 @@ func TestProcessMessage_CancelHappyPath(t *testing.T) { actor := "owner@example.com" mockStore.On("CancelExecutionAtomic", ctx, mock.Anything, "exec-cancel", &actor). Return(true, "cancelled", nil) + // Suppression cleanup must follow a successful atomic cancel. + mockStore.On("DeleteSuppressionsByExecutionTx", ctx, mock.Anything, "exec-cancel"). + Return(nil) manager := &Manager{ config: mockStore, diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index 406beb9f3..63aaa35bd 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -513,8 +513,8 @@ func (m *MockConfigStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, func (m *MockConfigStore) DeleteSuppressionsByExecution(_ context.Context, _ string) error { return nil } -func (m *MockConfigStore) DeleteSuppressionsByExecutionTx(_ context.Context, _ pgx.Tx, _ string) error { - return nil +func (m *MockConfigStore) DeleteSuppressionsByExecutionTx(ctx context.Context, tx pgx.Tx, executionID string) error { + return m.Called(ctx, tx, executionID).Error(0) } func (m *MockConfigStore) ListActiveSuppressions(_ context.Context) ([]config.PurchaseSuppression, error) { return nil, nil