Skip to content

fix(api): COALESCE cancel actor across canceled_by/cancelled_by #1490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 1 commit into from
Jul 22, 2026
Merged

fix(api): COALESCE cancel actor across canceled_by/cancelled_by #1490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/api/handler_purchases_revoke.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
34 changes: 18 additions & 16 deletions internal/config/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 18 additions & 14 deletions internal/config/store_postgres.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
61 changes: 61 additions & 0 deletions internal/config/store_postgres_pgxmock_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading