From fa6ae3f55802cae8db2ac152577bbe2baeeb150b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 20:08:11 +0300 Subject: [PATCH] fix(db): write canonical 'canceled' on all cancel paths + cleanup filter All cancel write paths (CancelExecutionAtomic, CancelScheduledExecutionAtomic, CancelAllPendingExchanges, CancelPendingExchangesByOrigin) were still emitting status='cancelled' and the old cancelled_by column after PR #1277 added the expand-contract rename. TransitionRIExchangeStatus in handler_ri_exchange.go also used the legacy spelling. If #1278 narrows the CHECK before these writers switch, every cancel path fails with check_violation (SQLSTATE 23514). CleanupOldExecutions filtered only 'cancelled', so rows written by new code with status='canceled' were never terminal-cleaned. Fixes: - Switch all cancel SQL writes to StatusCanceled ("canceled") via $N params - Use canceled_by column in CancelExecutionAtomic + CancelScheduledExecutionAtomic - Add 'canceled' to CleanupOldExecutions terminal-status IN clause - Use config.StatusCanceled in rejectRIExchange handler (line ~1372 + response) - Add 8 pgxmock regression tests asserting canonical spelling per path - Update 4 test mocks that hard-coded "cancelled" as the reject-transition arg --- internal/api/coverage_extras_test.go | 5 +- internal/api/handler_ri_exchange.go | 6 +- internal/api/handler_ri_exchange_test.go | 11 +- internal/api/router_handlers_test.go | 8 +- internal/config/store_postgres.go | 57 ++++--- .../config/store_postgres_pgxmock_test.go | 142 +++++++++++++++++- 6 files changed, 188 insertions(+), 41 deletions(-) diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index d385f8e3e..003a906a5 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -331,8 +331,9 @@ func TestHandler_rejectRIExchange_AlreadyProcessed(t *testing.T) { mockStore := new(MockConfigStore) mockStore.On("GetRIExchangeRecord", ctx, "11111111-1111-1111-1111-111111111111").Return( &config.RIExchangeRecord{ID: "11111111-1111-1111-1111-111111111111", ApprovalToken: "tok"}, nil) - // Transition returns nil indicating already processed - mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", "cancelled", mock.Anything). + // Transition returns nil indicating already processed. + // Canonical spelling used (#1277 follow-up). + mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", config.StatusCanceled, mock.Anything). Return(nil, nil) h := &Handler{config: mockStore} diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index b598bdb99..e968c558f 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1369,15 +1369,15 @@ func (h *Handler) rejectRIExchange(ctx context.Context, id, token string) (any, } // Token-based rejection: no session user, so transitioned_by = NULL. - transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "cancelled", nil) + transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", config.StatusCanceled, nil) if err != nil { return nil, fmt.Errorf("failed to transition exchange status: %w", err) } if transitioned == nil { - return nil, NewClientError(409, "exchange already processed, expired, or was cancelled") + return nil, NewClientError(409, "exchange already processed, expired, or was canceled") } - return map[string]string{"status": "cancelled"}, nil + return map[string]string{"status": config.StatusCanceled}, nil } // RIExchangeConfigResponse is the response for GET /api/ri-exchange/config. diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index a18b4d53d..d86cd08f4 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -262,8 +262,9 @@ func TestRejectRIExchange_AlreadyCompleted(t *testing.T) { ExchangeID: "exch-already-done", }, nil) - // Transition from pending→canceled fails (record is not pending) - mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "cancelled", mock.Anything). + // Transition from pending->canceled fails (record is not pending). + // Uses canonical spelling; see config.StatusCanceled. + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", config.StatusCanceled, mock.Anything). Return((*config.RIExchangeRecord)(nil), nil) _, err := h.rejectRIExchange(ctx, id, token) @@ -1577,10 +1578,10 @@ func TestRejectRIExchange_TokenPathActorIsNil(t *testing.T) { mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ ID: id, Status: "pending", ApprovalToken: "tok", }, nil) - // Token path: actor must be nil. - mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "cancelled", + // Token path: actor must be nil. Canonical spelling used (#1277 follow-up). + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", config.StatusCanceled, (*string)(nil), - ).Return(&config.RIExchangeRecord{ID: id, Status: "cancelled"}, nil) + ).Return(&config.RIExchangeRecord{ID: id, Status: config.StatusCanceled}, nil) _, err := (&Handler{config: mockStore}).rejectRIExchange(ctx, id, "tok") require.NoError(t, err) diff --git a/internal/api/router_handlers_test.go b/internal/api/router_handlers_test.go index 072b15231..f6373ce0b 100644 --- a/internal/api/router_handlers_test.go +++ b/internal/api/router_handlers_test.go @@ -784,18 +784,18 @@ func TestHandler_rejectRIExchange_ValidTokenAndRecord(t *testing.T) { ApprovalToken: "tok", Status: "pending", }, nil) - // rejectRIExchange transitions to "cancelled" (UK spelling, matches DB canonical) - mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", "cancelled", mock.Anything). + // rejectRIExchange transitions to "canceled" (US canonical spelling, #1277 follow-up). + mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", config.StatusCanceled, mock.Anything). Return(&config.RIExchangeRecord{ ID: "11111111-1111-1111-1111-111111111111", - Status: "cancelled", + Status: config.StatusCanceled, }, nil) h := &Handler{config: mockStore} result, err := h.rejectRIExchange(ctx, "11111111-1111-1111-1111-111111111111", "tok") require.NoError(t, err) m := result.(map[string]string) - assert.Equal(t, "cancelled", m["status"]) + assert.Equal(t, config.StatusCanceled, m["status"]) } // --------------------------------------------------------------------------- diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index b53628283..4ee0fafd8 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1044,12 +1044,13 @@ func (s *PostgresStore) SetCancelledBy(ctx context.Context, executionID, cancell } // 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 +// pending or notified to 'canceled' (canonical US spelling), setting +// canceled_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", +// live status fetched via a follow-up SELECT. Returns (true, "canceled", // nil) on success and (false, "", err) on a real DB error. // // The 'scheduled' status is intentionally NOT accepted here -- the @@ -1064,16 +1065,17 @@ func (s *PostgresStore) SetCancelledBy(ctx context.Context, executionID, cancell // 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) (canceled bool, currentStatus string, err error) { + // StatusCanceled is the canonical US-spelling value; see types.go. q := ` UPDATE purchase_executions - SET status = 'cancelled', - cancelled_by = $2, + SET status = $3, + canceled_by = $2, updated_at = NOW() WHERE execution_id = $1 AND status IN ('pending', 'notified') RETURNING status ` - rows, err := tx.Query(ctx, q, executionID, cancelledBy) + rows, err := tx.Query(ctx, q, executionID, cancelledBy, StatusCanceled) if err != nil { return false, "", fmt.Errorf("failed to cancel execution: %w", err) } @@ -1104,11 +1106,11 @@ func (s *PostgresStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, ex } // CancelScheduledExecutionAtomic atomically transitions an execution from -// 'scheduled' to 'cancelled', setting cancelled_by to the supplied actor -// (NULL when actor is nil). Used by the Gmail-style pre-fire delay revoke -// path (issue #290 / #291 wave-2): an approved-but-not-yet-fired execution -// can be revoked at $0 by flipping it to cancelled before the scheduler -// fires the cloud SDK call. +// 'scheduled' to 'canceled' (canonical US spelling), setting canceled_by to +// the supplied actor (NULL when actor is nil). Used by the Gmail-style +// pre-fire delay revoke path (issue #290 / #291 wave-2): an +// approved-but-not-yet-fired execution can be revoked at $0 by flipping it +// to canceled before the scheduler fires the cloud SDK call. // // The 'scheduled' status is the only accepted source. A concurrent // scheduler tick that already transitioned the row to 'approved' or @@ -1117,20 +1119,21 @@ func (s *PostgresStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, ex // ("revocation window has closed") so the frontend can fall through to // the post-execution Azure direct-cancel API path. // -// Returns (true, "cancelled", nil) on success and (false, "", err) on a +// Returns (true, StatusCanceled, nil) on success and (false, "", err) on a // real DB error. Must be called inside a WithTx block so the suppression // cleanup commits atomically with the status flip. func (s *PostgresStore) CancelScheduledExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (canceled bool, currentStatus string, err error) { + // StatusCanceled is the canonical US-spelling value; see types.go. q := ` UPDATE purchase_executions - SET status = 'cancelled', - cancelled_by = $2, + SET status = $3, + canceled_by = $2, updated_at = NOW() WHERE execution_id = $1 AND status = 'scheduled' RETURNING status ` - rows, err := tx.Query(ctx, q, executionID, cancelledBy) + rows, err := tx.Query(ctx, q, executionID, cancelledBy, StatusCanceled) if err != nil { return false, "", fmt.Errorf("failed to cancel scheduled execution: %w", err) } @@ -1598,9 +1601,11 @@ func (s *PostgresStore) GetScheduledExecutionsDue(ctx context.Context) ([]Purcha // Two independent cleanup branches, each with its own retention window so // that a row far in one dimension doesn't block cleanup in the other: // -// 1. Terminal-state cleanup: `status IN ('completed', 'cancelled') AND -// scheduled_date < NOW() - retention`. Keeps recent completions -// visible in the UI for at least `retention` days before purging. +// 1. Terminal-state cleanup: `status IN ('completed', 'cancelled', 'canceled') AND +// scheduled_date < NOW() - retention`. Both spellings are included to +// cover legacy rows ('cancelled') and new code rows ('canceled', canonical +// US spelling per StatusCanceled / migration 000089). Keeps recent +// completions visible in the UI for at least `retention` days before purging. // // 2. Expired-execution cleanup: `expires_at IS NOT NULL AND expires_at < // NOW() - retention`. A row whose approval token has been expired @@ -1620,10 +1625,14 @@ func (s *PostgresStore) GetScheduledExecutionsDue(ctx context.Context) ([]Purcha // NULL `expires_at` is excluded from branch 2 so rows that never had an // expiration deadline are safe from expiry-based cleanup. func (s *PostgresStore) CleanupOldExecutions(ctx context.Context, retentionDays int) (int64, error) { + // Both spellings are included: 'cancelled' (legacy, pre-migration 000089 + // rows) and 'canceled' (canonical, written by new code after the + // follow-up to #1277). #1278 will drop 'cancelled' once all rows are + // normalized. query := ` DELETE FROM purchase_executions WHERE ( - status IN ('completed', 'cancelled') + status IN ('completed', 'cancelled', 'canceled') AND scheduled_date < NOW() - INTERVAL '1 day' * $1 ) OR ( @@ -2585,11 +2594,11 @@ func (s *PostgresStore) GetRIExchangeDailySpend(ctx context.Context, date time.T func (s *PostgresStore) CancelAllPendingExchanges(ctx context.Context) (int64, error) { query := ` UPDATE ri_exchange_history - SET status = 'cancelled' + SET status = $1, updated_at = NOW() WHERE status = 'pending' ` - result, err := s.db.Exec(ctx, query) + result, err := s.db.Exec(ctx, query, StatusCanceled) if err != nil { return 0, fmt.Errorf("failed to cancel pending exchanges: %w", err) } @@ -2622,20 +2631,20 @@ func (s *PostgresStore) CancelPendingExchangesByOrigin(ctx context.Context, orig case common.ExchangeOriginLadder: query = ` UPDATE ri_exchange_history - SET status = 'cancelled', updated_at = NOW() + SET status = $1, updated_at = NOW() WHERE status = 'pending' AND ladder_run_id IS NOT NULL ` default: // common.ExchangeOriginStandalone (validated non-unknown above) query = ` UPDATE ri_exchange_history - SET status = 'cancelled', updated_at = NOW() + SET status = $1, updated_at = NOW() WHERE status = 'pending' AND ladder_run_id IS NULL ` } - result, err := s.db.Exec(ctx, query) + result, err := s.db.Exec(ctx, query, StatusCanceled) if err != nil { return 0, fmt.Errorf("failed to cancel pending exchanges by origin %q: %w", origin, err) } diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 4dc3d942e..d6d922019 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -1968,7 +1968,8 @@ func TestPGXMock_CancelAllPendingExchanges_Success(t *testing.T) { store := storeWith(mock) ctx := context.Background() - mock.ExpectExec("UPDATE").WillReturnResult(pgxmock.NewResult("UPDATE", 3)) + // Now parameterized: $1 = status value. + mock.ExpectExec("UPDATE").WithArgs(pgxmock.AnyArg()).WillReturnResult(pgxmock.NewResult("UPDATE", 3)) n, err := store.CancelAllPendingExchanges(ctx) require.NoError(t, err) @@ -1987,7 +1988,8 @@ func TestPGXMock_CancelPendingExchangesByOrigin_Standalone(t *testing.T) { ctx := context.Background() // Standalone origin must target WHERE ladder_run_id IS NULL only. - mock.ExpectExec("ladder_run_id IS NULL").WillReturnResult(pgxmock.NewResult("UPDATE", 2)) + // Now parameterized: $1 = status value. + mock.ExpectExec("ladder_run_id IS NULL").WithArgs(pgxmock.AnyArg()).WillReturnResult(pgxmock.NewResult("UPDATE", 2)) n, err := store.CancelPendingExchangesByOrigin(ctx, common.ExchangeOriginStandalone) require.NoError(t, err) @@ -2001,7 +2003,8 @@ func TestPGXMock_CancelPendingExchangesByOrigin_Ladder(t *testing.T) { ctx := context.Background() // Ladder origin must target WHERE ladder_run_id IS NOT NULL only. - mock.ExpectExec("ladder_run_id IS NOT NULL").WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + // Now parameterized: $1 = status value. + mock.ExpectExec("ladder_run_id IS NOT NULL").WithArgs(pgxmock.AnyArg()).WillReturnResult(pgxmock.NewResult("UPDATE", 1)) n, err := store.CancelPendingExchangesByOrigin(ctx, common.ExchangeOriginLadder) require.NoError(t, err) @@ -2521,3 +2524,136 @@ func TestPGXMock_GetExecutionByPlanAndDate_NotFoundWrapsErrNotFound(t *testing.T "zero-row GetExecutionByPlanAndDate must wrap ErrNotFound so getOrCreateExecution can create a new execution; got: %v", err) assert.NoError(t, mock.ExpectationsWereMet()) } + +// ─── Canonical-cancel write regression tests (#1277 follow-up) ─────────────── +// +// These tests assert that every cancel write path emits the canonical US +// spelling 'canceled' (StatusCanceled) rather than the legacy 'cancelled'. +// They fail on the pre-fix code (which hardcoded 'cancelled') and pass only +// when the parameterized $N arg receives StatusCanceled = "canceled". + +// TestPGXMock_CancelExecutionAtomic_WritesCanonicalStatus verifies that +// CancelExecutionAtomic sends StatusCanceled ("canceled") as the status +// parameter and sets canceled_by (not cancelled_by). +// Regression guard for the #1277 follow-up fix. +func TestPGXMock_CancelExecutionAtomic_WritesCanonicalStatus(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + actor := "user@example.com" + + // The mock uses regexp matching. We pin on: + // - "canceled_by" to confirm the new column is targeted (not cancelled_by) + // - WithArgs: $1=execID, $2=&actor, $3=StatusCanceled + // The RETURNING row also carries "canceled" so the caller's returned + // status is the canonical spelling end-to-end. + rows := pgxmock.NewRows([]string{"status"}).AddRow(StatusCanceled) + mock.ExpectQuery("canceled_by"). + WithArgs("exec-1", &actor, StatusCanceled). + WillReturnRows(rows) + + // Pass mock directly as pgx.Tx (PgxPoolIface embeds pgx.Tx). + ok, status, err := store.CancelExecutionAtomic(ctx, mock, "exec-1", &actor) + require.NoError(t, err) + assert.True(t, ok, "row should have been updated") + assert.Equal(t, StatusCanceled, status, "returned status must be canonical 'canceled'") + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_CancelScheduledExecutionAtomic_WritesCanonicalStatus mirrors the +// above for the scheduled-execution cancel path. +func TestPGXMock_CancelScheduledExecutionAtomic_WritesCanonicalStatus(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + actor := "scheduler@system" + + rows := pgxmock.NewRows([]string{"status"}).AddRow(StatusCanceled) + mock.ExpectQuery("canceled_by"). + WithArgs("exec-sched", &actor, StatusCanceled). + WillReturnRows(rows) + + ok, status, err := store.CancelScheduledExecutionAtomic(ctx, mock, "exec-sched", &actor) + require.NoError(t, err) + assert.True(t, ok, "row should have been updated") + assert.Equal(t, StatusCanceled, status, "returned status must be canonical 'canceled'") + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_CancelAllPendingExchanges_WritesCanonicalStatus pins +// CancelAllPendingExchanges to emit StatusCanceled as the $1 argument. +// A pre-fix version of the code sent no parameter (the value was inlined as +// the literal 'cancelled'), so this test double-checks that the parameterized +// form is used and the value is the canonical spelling. +func TestPGXMock_CancelAllPendingExchanges_WritesCanonicalStatus(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectExec("UPDATE ri_exchange_history"). + WithArgs(StatusCanceled). + WillReturnResult(pgxmock.NewResult("UPDATE", 2)) + + n, err := store.CancelAllPendingExchanges(ctx) + require.NoError(t, err) + assert.Equal(t, int64(2), n) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_CancelPendingExchangesByOrigin_Standalone_WritesCanonicalStatus +// asserts the standalone branch passes StatusCanceled. +func TestPGXMock_CancelPendingExchangesByOrigin_Standalone_WritesCanonicalStatus(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectExec("ladder_run_id IS NULL"). + WithArgs(StatusCanceled). + WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + + n, err := store.CancelPendingExchangesByOrigin(ctx, common.ExchangeOriginStandalone) + require.NoError(t, err) + assert.Equal(t, int64(1), n) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_CancelPendingExchangesByOrigin_Ladder_WritesCanonicalStatus +// asserts the ladder branch passes StatusCanceled. +func TestPGXMock_CancelPendingExchangesByOrigin_Ladder_WritesCanonicalStatus(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectExec("ladder_run_id IS NOT NULL"). + WithArgs(StatusCanceled). + WillReturnResult(pgxmock.NewResult("UPDATE", 3)) + + n, err := store.CancelPendingExchangesByOrigin(ctx, common.ExchangeOriginLadder) + require.NoError(t, err) + assert.Equal(t, int64(3), n) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_CleanupOldExecutions_IncludesCanonicalStatus verifies that the +// cleanup DELETE includes 'canceled' (canonical) in the terminal-status filter, +// so Plans-canceled rows written by new code are not missed. +// Regression guard for the #1277 follow-up Defect 2 fix. +func TestPGXMock_CleanupOldExecutions_IncludesCanonicalStatus(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + // The regex pins on both 'cancelled' (legacy) and 'canceled' (canonical) + // appearing together in the SQL. If either is missing the mock won't + // match, causing an "unexpected query" error from ExpectationsWereMet. + mock.ExpectExec(`'cancelled'.*'canceled'`). + WithArgs(pgxmock.AnyArg()). + WillReturnResult(pgxmock.NewResult("DELETE", 4)) + + n, err := store.CleanupOldExecutions(ctx, 90) + require.NoError(t, err) + assert.Equal(t, int64(4), n) + assert.NoError(t, mock.ExpectationsWereMet()) +}