From 662472a24cea8d77637bd2a33bb3febd9cf47413 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 22 Jul 2026 23:42:18 +0200 Subject: [PATCH] fix(api): COALESCE cancel actor across canceled_by/cancelled_by on read paths Migration 000089 (PR #1277) and its follow-up (PR #1453) moved cancel attribution writes to the new canonical canceled_by column, but every SELECT path in store_postgres.go still projected the bare legacy cancelled_by column. A cancel actor written only to canceled_by (the CancelExecutionAtomic / CancelScheduledExecutionAtomic paths) scanned back as NULL, silently dropping the actor from the History UI. Fix every read path to project COALESCE(canceled_by, cancelled_by) AS cancelled_by so a row written by either column is attributed correctly, and make SetCancelledBy write both columns so it stays symmetric with the atomic-cancel paths ahead of the planned contract migration (#1278) that drops the legacy column. Adds a pgxmock regression test asserting the projection; confirmed it fails against the pre-fix query (mock returns no rows because the issued SQL lacks the COALESCE expression) and passes post-fix. Relates to #1277, #1453. --- internal/api/handler_purchases_revoke.go | 2 +- internal/config/interfaces.go | 34 ++++++----- internal/config/store_postgres.go | 32 +++++----- .../config/store_postgres_pgxmock_test.go | 61 +++++++++++++++++++ 4 files changed, 98 insertions(+), 31 deletions(-) diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index a43de2b76..ecbde82cd 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -261,7 +261,7 @@ func (h *Handler) revokeScheduledExecution(ctx context.Context, session *Session if err := h.config.WithTx(ctx, func(tx pgx.Tx) error { var err error // The scheduled-revoke path uses its own CAS variant that flips ONLY - // status='scheduled' -> 'cancelled'. CancelExecutionAtomic accepts + // status='scheduled' -> 'canceled'. CancelExecutionAtomic accepts // only ('pending','notified') and would always return zero rows on // a scheduled row, miscoded as "race lost" -> a misleading 410 even // during the happy path. Issue #290 wave-2: keep the two CAS contracts diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index f3e3f3658..bee48e050 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -93,32 +93,34 @@ type StoreInterface interface { // When non-nil the actor is stamped onto transitioned_by + transitioned_at; when nil, // transitioned_by is set to NULL and transitioned_at is still set to NOW() for ordering. TransitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string) (*PurchaseExecution, error) - // SetCancelledBy stamps a cancelled_by / revoked_by email on an execution - // without overwriting any other columns. Used after TransitionExecutionStatus - // to fold the actor attribution into the same logical write without the - // full-row SavePurchaseExecution clobber risk (Finding #5 / PR #889). + // SetCancelledBy stamps canceled_by and the legacy cancelled_by column on + // an execution without overwriting any other columns. Used after + // TransitionExecutionStatus to fold the actor attribution into the same + // logical write without the full-row SavePurchaseExecution clobber risk + // (Finding #5 / PR #889). SetCancelledBy(ctx context.Context, executionID string, cancelledBy string) error // CancelExecutionAtomic atomically flips status from pending / notified - // to 'cancelled', setting cancelled_by. The 'scheduled' status is NOT - // accepted here; scheduled rows are revoked via + // to 'canceled' (canonical US spelling), setting canceled_by. The + // 'scheduled' status is NOT accepted here; scheduled rows are revoked via // CancelScheduledExecutionAtomic (Gmail-style pre-fire delay revoke // path, issue #291 wave-2) so the two flows surface distinct CAS race - // outcomes. Returns (true, "cancelled", nil) on success and (false, + // outcomes. Returns (true, "canceled", 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) (canceled bool, currentStatus string, err error) // CancelScheduledExecutionAtomic atomically flips status from 'scheduled' to - // 'cancelled', setting cancelled_by. Used by the Gmail-style pre-fire delay - // revoke path (issue #291 wave-2) to cancel a scheduled execution at $0 before - // the scheduler fires the SDK call. The 'pending'/'notified' set accepted by - // CancelExecutionAtomic is intentionally not extended here so the two revoke - // flows surface distinct CAS race outcomes -- a scheduled row that the - // scheduler has already transitioned to 'approved' / 'running' must surface as - // a 410 ("window closed") rather than a 409 ("not pending"). Returns - // (true, "cancelled", nil) on success and (false, currentStatus, nil) when - // zero rows were affected. Must be called inside a WithTx block. + // 'canceled' (canonical US spelling), setting canceled_by. Used by the + // Gmail-style pre-fire delay revoke path (issue #291 wave-2) to cancel a + // scheduled execution at $0 before the scheduler fires the SDK call. The + // 'pending'/'notified' set accepted by CancelExecutionAtomic is + // intentionally not extended here so the two revoke flows surface distinct + // CAS race outcomes -- a scheduled row that the scheduler has already + // transitioned to 'approved' / 'running' must surface as a 410 ("window + // closed") rather than a 409 ("not pending"). Returns (true, "canceled", + // nil) on success and (false, currentStatus, nil) when zero rows were + // affected. Must be called inside a WithTx block. CancelScheduledExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (canceled bool, currentStatus string, err error) // ListStuckExecutions returns executions in any of the given statuses // whose updated_at is older than the given duration. Used by the diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 9c6b7915f..ea04ad6a9 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1032,12 +1032,16 @@ func (s *PostgresStore) TransitionExecutionStatus(ctx context.Context, execution return &records[0], nil } -// SetCancelledBy stamps the cancelled_by column for a single execution without -// touching any other column. This avoids the full-row overwrite that a -// SavePurchaseExecution follow-up would perform, eliminating the lost-update -// window between TransitionExecutionStatus and the attribution write (Finding #5). +// SetCancelledBy stamps both the canceled_by and legacy cancelled_by columns +// for a single execution without touching any other column. This avoids the +// full-row overwrite that a SavePurchaseExecution follow-up would perform, +// eliminating the lost-update window between TransitionExecutionStatus and +// the attribution write (Finding #5). Writing both columns keeps this path +// symmetric with CancelExecutionAtomic/CancelScheduledExecutionAtomic (which +// write canceled_by only) so revoke-attribution keeps working unchanged if a +// future contract migration (#1278) drops the legacy column. func (s *PostgresStore) SetCancelledBy(ctx context.Context, executionID, cancelledBy string) error { - q := `UPDATE purchase_executions SET cancelled_by = $2, updated_at = NOW() + q := `UPDATE purchase_executions SET canceled_by = $2, cancelled_by = $2, updated_at = NOW() WHERE execution_id = $1` _, err := s.db.Exec(ctx, q, executionID, cancelledBy) return err @@ -1182,7 +1186,7 @@ func (s *PostgresStore) GetExecutionsByStatuses(ctx context.Context, statuses [] SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + cloud_account_id, source, approved_by, COALESCE(canceled_by, cancelled_by) AS cancelled_by, capacity_percent, created_by_user_id, retry_execution_id, retry_attempt_n, approval_token_expires_at, executed_by_user_id, executed_at, pre_approval_skip_reason, @@ -1223,7 +1227,7 @@ func (s *PostgresStore) GetPlannedExecutions(ctx context.Context, statuses []str SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + cloud_account_id, source, approved_by, COALESCE(canceled_by, cancelled_by) AS cancelled_by, capacity_percent, created_by_user_id, retry_execution_id, retry_attempt_n, approval_token_expires_at, executed_by_user_id, executed_at, pre_approval_skip_reason, @@ -1248,7 +1252,7 @@ func (s *PostgresStore) GetStaleApprovedExecutions(ctx context.Context, olderTha SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + cloud_account_id, source, approved_by, COALESCE(canceled_by, cancelled_by) AS cancelled_by, capacity_percent, created_by_user_id, retry_execution_id, retry_attempt_n, approval_token_expires_at, executed_by_user_id, executed_at, pre_approval_skip_reason, @@ -1290,7 +1294,7 @@ func (s *PostgresStore) ListStuckExecutions(ctx context.Context, statuses []stri SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + cloud_account_id, source, approved_by, COALESCE(canceled_by, cancelled_by) AS cancelled_by, capacity_percent, created_by_user_id, retry_execution_id, retry_attempt_n, approval_token_expires_at, executed_by_user_id, executed_at, pre_approval_skip_reason, @@ -1311,7 +1315,7 @@ func (s *PostgresStore) GetPendingExecutions(ctx context.Context) ([]PurchaseExe SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + cloud_account_id, source, approved_by, COALESCE(canceled_by, cancelled_by) AS cancelled_by, capacity_percent, created_by_user_id, retry_execution_id, retry_attempt_n, approval_token_expires_at, executed_by_user_id, executed_at, pre_approval_skip_reason, @@ -1335,7 +1339,7 @@ func (s *PostgresStore) GetPendingExecutionsTx(ctx context.Context, tx pgx.Tx) ( SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + cloud_account_id, source, approved_by, COALESCE(canceled_by, cancelled_by) AS cancelled_by, capacity_percent, created_by_user_id, retry_execution_id, retry_attempt_n, approval_token_expires_at, executed_by_user_id, executed_at, pre_approval_skip_reason, @@ -1365,7 +1369,7 @@ func (s *PostgresStore) GetExecutionByID(ctx context.Context, executionID string SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + cloud_account_id, source, approved_by, COALESCE(canceled_by, cancelled_by) AS cancelled_by, capacity_percent, created_by_user_id, retry_execution_id, retry_attempt_n, approval_token_expires_at, executed_by_user_id, executed_at, pre_approval_skip_reason, @@ -1392,7 +1396,7 @@ func (s *PostgresStore) GetExecutionByPlanAndDate(ctx context.Context, planID st SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + cloud_account_id, source, approved_by, COALESCE(canceled_by, cancelled_by) AS cancelled_by, capacity_percent, created_by_user_id, retry_execution_id, retry_attempt_n, approval_token_expires_at, executed_by_user_id, executed_at, pre_approval_skip_reason, @@ -1576,7 +1580,7 @@ func (s *PostgresStore) GetScheduledExecutionsDue(ctx context.Context) ([]Purcha SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + cloud_account_id, source, approved_by, COALESCE(canceled_by, cancelled_by) AS cancelled_by, capacity_percent, created_by_user_id, retry_execution_id, retry_attempt_n, approval_token_expires_at, executed_by_user_id, executed_at, pre_approval_skip_reason, diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 9f66dc7e2..5953900da 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -739,6 +739,67 @@ func TestPGXMock_GetPlannedExecutions_ProjectsAllScanColumns(t *testing.T) { assert.NoError(t, mock.ExpectationsWereMet()) } +// TestPGXMock_GetExecutionByID_ProjectsCoalescedCancelledBy guards the #1277 / +// #1453 read-side follow-up: CancelExecutionAtomic and +// CancelScheduledExecutionAtomic write the canceling actor to the NEW +// canceled_by column, but migration 000089's expand-contract contract states +// every SELECT must read back COALESCE(canceled_by, cancelled_by) so a row +// written by either the old or the new column is attributed correctly. Before +// this fix GetExecutionByID selected the bare legacy cancelled_by, so an +// actor written only to canceled_by (the atomic-cancel paths) scanned back as +// NULL and the History page lost the attribution. +// +// The mock uses regexp query matching, so ExpectQuery requires the issued +// SELECT to contain the COALESCE expression; a bare `cancelled_by` projection +// (the pre-fix query) does not match, the mock returns no rows, and +// GetExecutionByID surfaces ErrNotFound instead of the expected execution -- +// that is how this test fails against the pre-fix code and passes once the +// projection is fixed. +func TestPGXMock_GetExecutionByID_ProjectsCoalescedCancelledBy(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + recsJSON, _ := json.Marshal([]RecommendationRecord{}) + now := time.Now().Truncate(time.Second) + cols := []string{ + "plan_id", "execution_id", "status", "step_number", "scheduled_date", + "notification_sent", "approval_token", "recommendations", + "total_upfront_cost", "estimated_savings", "completed_at", "error", "expires_at", + "cloud_account_id", "source", "approved_by", "cancelled_by", "capacity_percent", + "created_by_user_id", "retry_execution_id", "retry_attempt_n", + "approval_token_expires_at", + "executed_by_user_id", "executed_at", "pre_approval_skip_reason", + "idempotency_key", + "scheduled_execution_at", + } + // Simulates the exact row shape the atomic-cancel paths produce: the + // actor lives only in canceled_by (legacy cancelled_by is NULL). What + // Postgres would actually return for COALESCE(canceled_by, cancelled_by) + // is the canceled_by value, so the mocked "cancelled_by" scan slot below + // carries that same value to stand in for the server-side COALESCE result. + rows := pgxmock.NewRows(cols).AddRow( + "plan-1", "exec-1", "canceled", 1, now, + sql.NullTime{}, "tok-123", recsJSON, + 100.0, 200.0, sql.NullTime{}, "", sql.NullTime{}, + nil, "", nil, strPtr("cancelling-actor@example.com"), 100, + nil, nil, 0, + sql.NullTime{}, + nil, sql.NullTime{}, nil, + nil, + sql.NullTime{}, + ) + mock.ExpectQuery(`COALESCE\(canceled_by,\s*cancelled_by\)`). + WithArgs(pgxmock.AnyArg()). + WillReturnRows(rows) + + exec, err := store.GetExecutionByID(ctx, "exec-1") + require.NoError(t, err) + require.NotNil(t, exec.CancelledBy, "actor written to canceled_by must round-trip via the COALESCE read projection") + assert.Equal(t, "cancelling-actor@example.com", *exec.CancelledBy) + assert.NoError(t, mock.ExpectationsWereMet()) +} + // ─── queryPurchaseHistory (via GetPurchaseHistory) ──────────────────────────── func TestPGXMock_GetPurchaseHistory_Success(t *testing.T) {