From 6fd7d9f758563f7219ece9c970d0e427d81b227a Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 20:50:54 +0100 Subject: [PATCH 1/3] fix(config): let an account with only terminal executions be deleted Migration 000053 made purchase_executions.cloud_account_id ON DELETE RESTRICT, so one completed/failed/canceled/expired execution blocked the account delete forever, and the handler answered with a 409 asking the operator to cancel pending purchases that did not exist. DeleteCloudAccount now detaches the account's terminal executions (cloud_account_id = NULL, status in the new TerminalExecutionStatuses allowlist) in its transaction before the DELETE, keeping them as history. Live rows stay attached, so the FK still refuses the delete. The deleteAccount preflight adds CountLiveExecutionsForAccount and answers a 409 with reason "active_executions" for approved, running, scheduled or paused rows, after the existing pending/notified 409. Migration 000127 gives purchase_executions its own updated_at trigger that keeps updated_at on a detach-only UPDATE, so deleting an account does not pull old failures back into the plan health window or restart the retention clock of canceled rows. Closes #265 Co-Authored-By: Claude Opus 5.5 --- internal/analytics/collector_test.go | 4 + internal/api/handler_accounts.go | 118 +++++++---- internal/api/handler_accounts_test.go | 125 +++++++++++ internal/config/interfaces.go | 7 + internal/config/store_postgres.go | 39 ++++ ...ostgres_delete_account_integration_test.go | 197 ++++++++++++++++++ .../config/store_postgres_pgxmock_test.go | 56 +++++ internal/config/types.go | 32 +++ ...053_executions_account_fk_restrict_test.go | 8 +- ...xecutions_detach_keeps_updated_at.down.sql | 8 + ..._executions_detach_keeps_updated_at.up.sql | 37 ++++ ...executions_detach_keeps_updated_at_test.go | 96 +++++++++ internal/mocks/stores.go | 15 ++ internal/server/test_helpers_test.go | 4 + 14 files changed, 703 insertions(+), 43 deletions(-) create mode 100644 internal/config/store_postgres_delete_account_integration_test.go create mode 100644 internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.down.sql create mode 100644 internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.up.sql create mode 100644 internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at_test.go diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index f4916c13..1d4cf6bd 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -299,6 +299,10 @@ func (m *mockConfigStore) CountPendingExecutionsForAccount(ctx context.Context, return 0, nil } +func (m *mockConfigStore) CountLiveExecutionsForAccount(ctx context.Context, accountID string) (int, error) { + return 0, nil +} + func (m *mockConfigStore) ListPendingExecutionIDsForAccount(ctx context.Context, accountID string) ([]string, error) { return nil, nil } diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 0c6a19dd..3f7dbd2f 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -581,12 +581,12 @@ func (h *Handler) updateAccount(ctx context.Context, httpReq *events.LambdaFunct // deleteAccount handles DELETE /api/accounts/:id. // -// Preflights the delete against pending/notified purchase_executions -// (issue #606). Migration 000053 also enforces ON DELETE RESTRICT at the -// DB level so a race that slips past the preflight still can't orphan a -// pending execution — but the preflight gives the frontend a structured -// 409 (with the count + execution_ids) so it can offer a Cancel-All-Then- -// Delete affordance instead of surfacing a raw FK-violation 500. +// Preflights the delete against the purchase_executions that still need the +// account (preflightAccountDelete). Executions in a terminal status do not +// block: DeleteCloudAccount detaches them (cloud_account_id = NULL) and keeps +// them as history (issue #265). Migration 000053 also enforces ON DELETE +// RESTRICT at the DB level, so a live execution that races past the +// preflight still can't be orphaned. func (h *Handler) deleteAccount(ctx context.Context, req *events.LambdaFunctionURLRequest, id string) (any, error) { if err := validateUUID(id); err != nil { return nil, err @@ -604,44 +604,20 @@ func (h *Handler) deleteAccount(ctx context.Context, req *events.LambdaFunctionU return nil, err } - // Preflight: refuse the delete if pending/notified executions still - // reference this account. The frontend uses the pending_count + the - // pending_execution_ids list to drive the Cancel-All-Then-Delete UX. - pendingCount, err := h.config.CountPendingExecutionsForAccount(ctx, id) - if err != nil { - return nil, fmt.Errorf("accounts: %w", err) - } - if pendingCount > 0 { - execIDs, listErr := h.config.ListPendingExecutionIDsForAccount(ctx, id) - if listErr != nil { - // Count succeeded but list failed — still surface the 409 so the - // operator gets a clear "cancel them first" message instead of - // the raw FK error from the eventual DB delete. The list payload - // is omitted; the frontend falls back to a generic message. - return nil, NewClientErrorWithDetails(409, - fmt.Sprintf("cannot delete account: %d pending purchase(s) must be canceled first", pendingCount), - map[string]any{ - "pending_count": pendingCount, - "reason": "pending_executions", - }) - } - return nil, NewClientErrorWithDetails(409, - fmt.Sprintf("cannot delete account: %d pending purchase(s) must be canceled first", pendingCount), - map[string]any{ - "pending_count": pendingCount, - "pending_execution_ids": execIDs, - "reason": "pending_executions", - }) + if err := h.preflightAccountDelete(ctx, id); err != nil { + return nil, err } if err := h.config.DeleteCloudAccount(ctx, id); err != nil { - // Race: the preflight count was 0 but a pending execution row was - // inserted concurrently before we issued the DELETE. Migration 000053 - // enforces ON DELETE RESTRICT, so Postgres raises SQLSTATE 23503 - // (foreign_key_violation). Map this to the same structured 409 the - // preflight branch returns so the frontend's Cancel-All-Then-Delete - // affordance still kicks in — minus the count/ids, which we can't - // supply without re-querying (and the operator can just retry). + // Race: the preflight found no live execution but one was inserted + // concurrently before we issued the DELETE. Migration 000053 enforces + // ON DELETE RESTRICT, so Postgres raises SQLSTATE 23503 + // (foreign_key_violation). Terminal executions never cause this: + // DeleteCloudAccount detaches them in the same transaction. Map it to + // the same structured 409 the preflight returns so the frontend's + // Cancel-All-Then-Delete affordance still kicks in — minus the + // count/ids, which we can't supply without re-querying (and the + // operator can just retry). var pgErr *pgconn.PgError if errors.As(err, &pgErr) && pgErr.Code == "23503" { return nil, NewClientErrorWithDetails(409, @@ -656,6 +632,66 @@ func (h *Handler) deleteAccount(ctx context.Context, req *events.LambdaFunctionU return nil, nil } +// preflightAccountDelete refuses the delete with a structured 409 while any +// purchase execution still needs the account: +// +// - pending/notified rows (reason "pending_executions", issue #606): the +// frontend uses pending_count + pending_execution_ids to drive the +// Cancel-All-Then-Delete UX. +// - any other non-terminal row, e.g. approved, running, scheduled or +// paused (reason "active_executions", issue #265). These can't be +// canceled from the account page, so the operator has to let them +// finish, or revoke or delete them, first. +// +// Terminal rows are not counted: DeleteCloudAccount detaches them and keeps +// them as history. +func (h *Handler) preflightAccountDelete(ctx context.Context, id string) error { + pendingCount, err := h.config.CountPendingExecutionsForAccount(ctx, id) + if err != nil { + return fmt.Errorf("accounts: %w", err) + } + if pendingCount > 0 { + return h.pendingExecutionsConflict(ctx, id, pendingCount) + } + + liveCount, err := h.config.CountLiveExecutionsForAccount(ctx, id) + if err != nil { + return fmt.Errorf("accounts: %w", err) + } + if liveCount > 0 { + return NewClientErrorWithDetails(409, + fmt.Sprintf("cannot delete account: %d in-progress purchase(s) (approved, running, scheduled or paused) still use it; "+ + "wait for them to finish, or revoke or delete them, first", liveCount), + map[string]any{ + "active_count": liveCount, + "reason": "active_executions", + }) + } + return nil +} + +// pendingExecutionsConflict builds the issue #606 409 for pendingCount +// pending/notified executions, with their IDs when they can be listed. +func (h *Handler) pendingExecutionsConflict(ctx context.Context, id string, pendingCount int) error { + msg := fmt.Sprintf("cannot delete account: %d pending purchase(s) must be canceled first", pendingCount) + execIDs, listErr := h.config.ListPendingExecutionIDsForAccount(ctx, id) + if listErr != nil { + // Count succeeded but list failed — still surface the 409 so the + // operator gets a clear "cancel them first" message instead of + // the raw FK error from the eventual DB delete. The list payload + // is omitted; the frontend falls back to a generic message. + return NewClientErrorWithDetails(409, msg, map[string]any{ + "pending_count": pendingCount, + "reason": "pending_executions", + }) + } + return NewClientErrorWithDetails(409, msg, map[string]any{ + "pending_count": pendingCount, + "pending_execution_ids": execIDs, + "reason": "pending_executions", + }) +} + // parseAndValidateCredentialsRequest performs all input validation that must // run before we touch the database: UUID format, permission, body parse, // credential_type allowlist, and per-type payload schema. Pulled out of diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index 5866d693..529525a3 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -530,6 +530,131 @@ func TestDeleteAccount_PendingZero_DeletesNormally(t *testing.T) { assert.True(t, deleteCalled, "delete should proceed when no pending executions") } +// TestDeleteAccount_ActiveExecutions_Returns409 asserts the issue #265 +// preflight: with no cancelable pending/notified rows but an approved, +// running, scheduled or paused row still on the account, the handler refuses +// with a 409 that says why (reason "active_executions" + count) instead of +// letting the FK raise the misleading "cancel pending purchases" 409. +func TestDeleteAccount_ActiveExecutions_Returns409(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + store := setupAdminMock(ctx) + store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { return 0, nil } + var liveAccount string + store.CountLiveExecutionsForAccountFn = func(_ context.Context, id string) (int, error) { + liveAccount = id + return 2, nil + } + deleteCalled := false + store.DeleteCloudAccountFn = func(_ context.Context, _ string) error { + deleteCalled = true + return nil + } + handler := &Handler{auth: mockAuth, config: store} + + const id = "11111111-1111-1111-1111-111111111111" + result, err := handler.deleteAccount(ctx, adminRequest(""), id) + assert.Nil(t, result) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %v", err) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.message, "2 in-progress purchase(s)") + assert.NotContains(t, ce.message, "canceled first", "these rows cannot be canceled; the message must not say to") + details := ce.Details() + require.NotNil(t, details) + assert.EqualValues(t, 2, details["active_count"]) + assert.Equal(t, "active_executions", details["reason"]) + assert.Equal(t, id, liveAccount, "the live count must be scoped to the account being deleted") + assert.False(t, deleteCalled, "DeleteCloudAccount must NOT be called while live executions exist") +} + +// TestDeleteAccount_PendingTakesPrecedenceOverActive asserts the #606 +// Cancel-All-Then-Delete 409 still wins when cancelable rows exist, so the +// frontend keeps getting the execution IDs it needs. +func TestDeleteAccount_PendingTakesPrecedenceOverActive(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + store := setupAdminMock(ctx) + store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { return 1, nil } + store.ListPendingExecutionIDsForAccountFn = func(_ context.Context, _ string) ([]string, error) { + return []string{"exec-1"}, nil + } + liveCalled := false + store.CountLiveExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { + liveCalled = true + return 4, nil + } + handler := &Handler{auth: mockAuth, config: store} + + _, err := handler.deleteAccount(ctx, adminRequest(""), "11111111-1111-1111-1111-111111111111") + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %v", err) + assert.Equal(t, 409, ce.code) + assert.Equal(t, "pending_executions", ce.Details()["reason"]) + assert.False(t, liveCalled, "the live count is not needed once pending rows already block") +} + +// TestDeleteAccount_OnlyTerminalExecutions_Deletes asserts the issue #265 +// happy path at the handler: no pending and no live rows means the delete +// goes through (DeleteCloudAccount detaches the terminal history itself). +func TestDeleteAccount_OnlyTerminalExecutions_Deletes(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + store := setupAdminMock(ctx) + store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { return 0, nil } + liveCalled := false + store.CountLiveExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { + liveCalled = true + return 0, nil + } + deleteCalled := false + store.DeleteCloudAccountFn = func(_ context.Context, _ string) error { + deleteCalled = true + return nil + } + handler := &Handler{auth: mockAuth, config: store} + + result, err := handler.deleteAccount(ctx, adminRequest(""), "11111111-1111-1111-1111-111111111111") + require.NoError(t, err) + assert.Nil(t, result) + assert.True(t, liveCalled, "the live preflight must run before the delete") + assert.True(t, deleteCalled) +} + +// TestDeleteAccount_LiveCountErr_Returns500 asserts a failing live count is +// surfaced as an error (500) and never falls through to the delete. +func TestDeleteAccount_LiveCountErr_Returns500(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + store := setupAdminMock(ctx) + store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { return 0, nil } + store.CountLiveExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { + return 0, errors.New("db down") + } + deleteCalled := false + store.DeleteCloudAccountFn = func(_ context.Context, _ string) error { + deleteCalled = true + return nil + } + handler := &Handler{auth: mockAuth, config: store} + + _, err := handler.deleteAccount(ctx, adminRequest(""), "11111111-1111-1111-1111-111111111111") + require.Error(t, err) + _, ok := IsClientError(err) + assert.False(t, ok, "a store failure must not be reported as a client error") + assert.Contains(t, err.Error(), "db down") + assert.False(t, deleteCalled) +} + // TestDeleteAccount_PendingListErr_StillReturns409 asserts that a failure // listing execution_ids doesn't downgrade the 409 to a 500 — the operator // still gets the structured count and the explicit "cancel first" ask, diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index b8ac4fe5..89b3c953 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -206,6 +206,13 @@ type StoreInterface interface { // 409 with a count so the frontend can offer Cancel-All-Then-Delete UX // instead of surfacing a raw constraint error. See issue #606. CountPendingExecutionsForAccount(ctx context.Context, accountID string) (int, error) + // CountLiveExecutionsForAccount returns the number of purchase_executions + // referencing the given cloud account whose status is not in + // TerminalExecutionStatuses (pending, notified, approved, running, + // scheduled, paused, or any status not yet classified). These are the + // rows that block DeleteCloudAccount; terminal rows are detached and kept + // as history instead (issue #265). + CountLiveExecutionsForAccount(ctx context.Context, accountID string) (int, error) // ListPendingExecutionIDsForAccount returns the execution IDs of all // pending / notified executions referencing this account, used by the // frontend's Cancel-All-Then-Delete flow when the operator opts to diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 37647e4f..df8c9d27 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1897,6 +1897,26 @@ func (s *PostgresStore) CountPendingExecutionsForAccount(ctx context.Context, ac return n, nil } +// CountLiveExecutionsForAccount returns the number of purchase executions +// referencing this cloud account whose status is not in +// TerminalExecutionStatuses: rows an executor, scheduler or reaper may still +// act on, which need the account to exist. DeleteCloudAccount detaches every +// other row, so these are exactly the rows that block the delete (issue #265). +// It includes the pending/notified rows CountPendingExecutionsForAccount +// counts. +func (s *PostgresStore) CountLiveExecutionsForAccount(ctx context.Context, accountID string) (int, error) { + var n int + err := s.db.QueryRow(ctx, ` + SELECT COUNT(*) FROM purchase_executions + WHERE cloud_account_id = $1 + AND status <> ALL($2) + `, accountID, TerminalExecutionStatuses).Scan(&n) + if err != nil { + return 0, fmt.Errorf("failed to count live executions for account: %w", err) + } + return n, nil +} + // ListPendingExecutionIDsForAccount returns the execution IDs that the // frontend's Cancel-All-Then-Delete flow needs to POST cancel for. Capped // at 1000 rows — a single account with more pending executions than that @@ -3829,6 +3849,8 @@ func (s *PostgresStore) UpdateCloudAccount(ctx context.Context, account *CloudAc } // DeleteCloudAccount deletes a cloud account. Cascades to credentials and overrides. +// Terminal purchase executions are kept as history with cloud_account_id set to +// NULL; a live execution still blocks the delete through the FK (issue #265). // If an approved account_registrations row points at this account, it is reset to // 'pending' in the same transaction so the admin can re-approve through the normal // flow instead of being left with a dead-end "Approved (account pending link)" row. @@ -3853,6 +3875,23 @@ func (s *PostgresStore) DeleteCloudAccount(ctx context.Context, id string) error return fmt.Errorf("failed to reset linked registration: %w", err) } + // Detach the account's terminal executions so they survive as history + // (issue #265). Migration 000053's ON DELETE RESTRICT FK otherwise blocks + // the delete for every referencing row whatever its status. Live rows are + // left attached, so the DELETE below still fails on the FK (SQLSTATE + // 23503) while any remain, and this UPDATE rolls back with it. Migration + // 000127 keeps updated_at unchanged on these rows: detaching is not a + // lifecycle event, and the plan health score and the retention sweep both + // read updated_at as one. + if _, err = tx.Exec(ctx, ` + UPDATE purchase_executions + SET cloud_account_id = NULL + WHERE cloud_account_id = $1 + AND status = ANY($2) + `, id, TerminalExecutionStatuses); err != nil { + return fmt.Errorf("failed to detach terminal executions: %w", err) + } + tag, err := tx.Exec(ctx, `DELETE FROM cloud_accounts WHERE id = $1`, id) if err != nil { return fmt.Errorf("failed to delete cloud account: %w", err) diff --git a/internal/config/store_postgres_delete_account_integration_test.go b/internal/config/store_postgres_delete_account_integration_test.go new file mode 100644 index 00000000..a0077908 --- /dev/null +++ b/internal/config/store_postgres_delete_account_integration_test.go @@ -0,0 +1,197 @@ +//go:build integration + +package config + +// Real PostgreSQL tests for DeleteCloudAccount against purchase_executions +// history (issue #265). Migration 000053 made the execution FK ON DELETE +// RESTRICT, which used to block the delete for every referencing row +// whatever its status, so one failed execution made an account undeletable. + +import ( + "context" + "errors" + "regexp" + "sort" + "testing" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func seedDeleteAccount(ctx context.Context, t *testing.T, store *PostgresStore) string { + t.Helper() + id := uuid.New().String() + _, err := store.db.Exec(ctx, ` + INSERT INTO cloud_accounts (id, name, enabled, provider, external_id, aws_is_org_root, created_at, updated_at) + VALUES ($1, 'Delete Test Account', true, 'azure', $2, false, now(), now())`, id, id) + require.NoError(t, err, "seed cloud_accounts") + return id +} + +// liveExecutionStatuses are the schema's statuses outside +// TerminalExecutionStatuses. TestTerminalExecutionStatuses_CoverSchema keeps +// the two lists equal to the status CHECK constraint between them. +var liveExecutionStatuses = []string{"pending", "notified", "approved", "running", "scheduled", "paused"} + +// seedDeleteAccountExec inserts an execution in status on accountID with an +// old updated_at, so a later bump is visible. It is a plain INSERT on purpose: +// the BEFORE UPDATE trigger would overwrite an updated_at set by an UPDATE. +func seedDeleteAccountExec(ctx context.Context, t *testing.T, store *PostgresStore, accountID, status string) string { + t.Helper() + var id string + require.NoError(t, store.db.QueryRow(ctx, ` + INSERT INTO purchase_executions (status, step_number, scheduled_date, cloud_account_id, updated_at) + VALUES ($1, 1, NOW(), $2, $3) + RETURNING execution_id::text`, status, accountID, agedUpdatedAt).Scan(&id), + "seed execution in status %s", status) + return id +} + +var agedUpdatedAt = time.Date(2021, 3, 4, 5, 6, 7, 0, time.UTC) + +func storedUpdatedAt(ctx context.Context, t *testing.T, store *PostgresStore, execID string) time.Time { + t.Helper() + var ts time.Time + require.NoError(t, store.db.QueryRow(ctx, + `SELECT updated_at FROM purchase_executions WHERE execution_id = $1`, execID).Scan(&ts)) + return ts.UTC() +} + +func storedExecAccount(ctx context.Context, t *testing.T, store *PostgresStore, execID string) (status string, accountID *string) { + t.Helper() + require.NoError(t, store.db.QueryRow(ctx, + `SELECT status, cloud_account_id::text FROM purchase_executions WHERE execution_id = $1`, execID, + ).Scan(&status, &accountID), "execution %s must survive the account delete", execID) + return status, accountID +} + +func accountExists(ctx context.Context, t *testing.T, store *PostgresStore, id string) bool { + t.Helper() + var n int + require.NoError(t, store.db.QueryRow(ctx, `SELECT COUNT(*) FROM cloud_accounts WHERE id = $1`, id).Scan(&n)) + return n == 1 +} + +// An account whose executions are all terminal is deletable, and the history +// rows survive with their status intact and the account reference detached. +func TestDeleteCloudAccount_TerminalExecutionsAreDetached(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + + for _, status := range TerminalExecutionStatuses { + t.Run(status, func(t *testing.T) { + acct := seedDeleteAccount(ctx, t, store) + execID := seedDeleteAccountExec(ctx, t, store, acct, status) + + require.NoError(t, store.DeleteCloudAccount(ctx, acct)) + + assert.False(t, accountExists(ctx, t, store, acct), "account row must be gone") + gotStatus, gotAcct := storedExecAccount(ctx, t, store, execID) + assert.Equal(t, status, gotStatus, "history row status must be unchanged") + assert.Nil(t, gotAcct, "history row must be detached from the deleted account") + assert.True(t, storedUpdatedAt(ctx, t, store, execID).Equal(agedUpdatedAt), + "detaching must not move updated_at (plan health and retention read it as a lifecycle clock)") + }) + } +} + +// A live execution still blocks the delete at the FK, and the transaction +// rolls back whole: the account survives and its terminal history stays +// attached to it. +func TestDeleteCloudAccount_LiveExecutionBlocks(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + + for _, status := range liveExecutionStatuses { + t.Run(status, func(t *testing.T) { + acct := seedDeleteAccount(ctx, t, store) + liveID := seedDeleteAccountExec(ctx, t, store, acct, status) + historyID := seedDeleteAccountExec(ctx, t, store, acct, StatusFailed) + + n, err := store.CountLiveExecutionsForAccount(ctx, acct) + require.NoError(t, err) + assert.Equal(t, 1, n, "only the %s row is live", status) + + err = store.DeleteCloudAccount(ctx, acct) + require.Error(t, err) + var pgErr *pgconn.PgError + require.True(t, errors.As(err, &pgErr), "want a *pgconn.PgError, got %v", err) + assert.Equal(t, "23503", pgErr.Code, "the RESTRICT FK must still refuse a live row") + + assert.True(t, accountExists(ctx, t, store, acct), "account must survive a refused delete") + for _, id := range []string{liveID, historyID} { + _, gotAcct := storedExecAccount(ctx, t, store, id) + require.NotNil(t, gotAcct, "a refused delete must leave %s attached", id) + assert.Equal(t, acct, *gotAcct) + } + }) + } +} + +// Detaching is scoped to the deleted account: another account's terminal +// execution keeps its reference. +func TestDeleteCloudAccount_DetachIsScopedToAccount(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + gone, kept := seedDeleteAccount(ctx, t, store), seedDeleteAccount(ctx, t, store) + seedDeleteAccountExec(ctx, t, store, gone, StatusCompleted) + keptID := seedDeleteAccountExec(ctx, t, store, kept, StatusCompleted) + + require.NoError(t, store.DeleteCloudAccount(ctx, gone)) + + _, gotAcct := storedExecAccount(ctx, t, store, keptID) + require.NotNil(t, gotAcct) + assert.Equal(t, kept, *gotAcct) +} + +// CountLiveExecutionsForAccount counts every non-terminal row of the account +// and nothing else; CountPendingExecutionsForAccount keeps counting only the +// cancelable pending/notified subset. +func TestCountLiveExecutionsForAccount(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + acct, other := seedDeleteAccount(ctx, t, store), seedDeleteAccount(ctx, t, store) + for _, status := range liveExecutionStatuses { + seedDeleteAccountExec(ctx, t, store, acct, status) + } + for _, status := range TerminalExecutionStatuses { + seedDeleteAccountExec(ctx, t, store, acct, status) + } + seedDeleteAccountExec(ctx, t, store, other, "running") + + live, err := store.CountLiveExecutionsForAccount(ctx, acct) + require.NoError(t, err) + assert.Equal(t, len(liveExecutionStatuses), live) + + pending, err := store.CountPendingExecutionsForAccount(ctx, acct) + require.NoError(t, err) + assert.Equal(t, 2, pending, "pending + notified") +} + +// TerminalExecutionStatuses plus liveExecutionStatuses must be exactly the +// statuses the schema accepts. A status added to the CHECK constraint fails +// this test until it is classified: as terminal (detached on account delete) +// or live (blocks the delete, the default the allowlist gives it). +func TestTerminalExecutionStatuses_CoverSchema(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + + var def string + require.NoError(t, store.db.QueryRow(ctx, ` + SELECT pg_get_constraintdef(oid) FROM pg_constraint + WHERE conname = 'purchase_executions_status_check'`).Scan(&def)) + matches := regexp.MustCompile(`'([a-z_]+)'`).FindAllStringSubmatch(def, -1) + schema := make([]string, 0, len(matches)) + for _, m := range matches { + schema = append(schema, m[1]) + } + require.NotEmpty(t, schema, "could not parse %q", def) + + classified := append(append([]string{}, TerminalExecutionStatuses...), liveExecutionStatuses...) + sort.Strings(schema) + sort.Strings(classified) + assert.Equal(t, schema, classified) +} diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 60366373..cb917383 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -1742,6 +1742,7 @@ func TestPGXMock_DeleteCloudAccount_Success(t *testing.T) { mock.ExpectBegin() mock.ExpectExec("UPDATE account_registrations").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("UPDATE", 0)) + expectDetachTerminalExecutions(mock) mock.ExpectExec("DELETE FROM cloud_accounts").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("DELETE", 1)) mock.ExpectCommit() @@ -1758,6 +1759,7 @@ func TestPGXMock_DeleteCloudAccount_WithLinkedRegistration(t *testing.T) { mock.ExpectBegin() mock.ExpectExec("UPDATE account_registrations").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + expectDetachTerminalExecutions(mock) mock.ExpectExec("DELETE FROM cloud_accounts").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("DELETE", 1)) mock.ExpectCommit() @@ -1774,6 +1776,7 @@ func TestPGXMock_DeleteCloudAccount_NotFound(t *testing.T) { mock.ExpectBegin() mock.ExpectExec("UPDATE account_registrations").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("UPDATE", 0)) + expectDetachTerminalExecutions(mock) mock.ExpectExec("DELETE FROM cloud_accounts").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("DELETE", 0)) mock.ExpectRollback() @@ -1783,6 +1786,59 @@ func TestPGXMock_DeleteCloudAccount_NotFound(t *testing.T) { assert.Contains(t, err.Error(), "not found") } +// expectDetachTerminalExecutions expects DeleteCloudAccount's issue #265 +// detach of the account's terminal executions, parameterized by exactly +// TerminalExecutionStatuses. +func expectDetachTerminalExecutions(mock pgxmock.PgxPoolIface) { + mock.ExpectExec(`UPDATE purchase_executions\s+SET cloud_account_id = NULL\s+WHERE cloud_account_id = \$1\s+AND status = ANY\(\$2\)`). + WithArgs(pgxmock.AnyArg(), TerminalExecutionStatuses). + WillReturnResult(pgxmock.NewResult("UPDATE", 2)) +} + +func TestPGXMock_DeleteCloudAccount_DetachFailsRollsBack(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectBegin() + mock.ExpectExec("UPDATE account_registrations").WithArgs(pgxmock.AnyArg()). + WillReturnResult(pgxmock.NewResult("UPDATE", 0)) + mock.ExpectExec("UPDATE purchase_executions").WithArgs(pgxmock.AnyArg(), TerminalExecutionStatuses). + WillReturnError(errors.New("boom")) + mock.ExpectRollback() + + err := store.DeleteCloudAccount(ctx, "acct-id") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to detach terminal executions") +} + +func TestPGXMock_CountLiveExecutionsForAccount(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectQuery(`SELECT COUNT\(\*\) FROM purchase_executions\s+WHERE cloud_account_id = \$1\s+AND status <> ALL\(\$2\)`). + WithArgs("acct-id", TerminalExecutionStatuses). + WillReturnRows(pgxmock.NewRows([]string{"count"}).AddRow(3)) + + n, err := store.CountLiveExecutionsForAccount(ctx, "acct-id") + require.NoError(t, err) + assert.Equal(t, 3, n) +} + +func TestPGXMock_CountLiveExecutionsForAccount_Error(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectQuery("SELECT COUNT").WithArgs("acct-id", TerminalExecutionStatuses). + WillReturnError(errors.New("boom")) + + _, err := store.CountLiveExecutionsForAccount(ctx, "acct-id") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to count live executions for account") +} + // ─── ListCloudAccounts ──────────────────────────────────────────────────────── func TestPGXMock_ListCloudAccounts_Success(t *testing.T) { diff --git a/internal/config/types.go b/internal/config/types.go index 66fd4591..4f976315 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -536,6 +536,38 @@ var HealthScoredExecutionStatuses = []string{ LegacyStatusCanceled, } +// StatusRevocationRequested records that an operator asked for a completed +// purchase to be revoked. The cloud-side revocation happens outside this +// system (an AWS support case), and no code moves a row out of this status. +const StatusRevocationRequested = "revocation_requested" + +// TerminalExecutionStatuses are the execution statuses that no executor, +// scheduler or reaper will act on again. DeleteCloudAccount detaches rows in +// these statuses from the account being deleted (cloud_account_id = NULL, the +// same treatment purchase_history gets from its ON DELETE SET NULL FK) so the +// history survives the delete, and CountLiveExecutionsForAccount counts every +// other row as one that still needs the account (issue #265). +// +// It is an ALLOWLIST on purpose: a status added to the schema later counts as +// live, and so blocks the delete, until someone classifies it here. Blocking +// a delete is recoverable; detaching an execution that will still run is not, +// because a row with no account falls back to plan fan-out or ambient +// credentials. +// +// A failed row is listed even though it can be retried. A retry of a row +// detached here still carries the deleted account on its recommendations, so +// the executor refuses it before submission ("account not found") instead of +// buying elsewhere. +var TerminalExecutionStatuses = []string{ + StatusCompleted, + StatusPartiallyCompleted, + StatusFailed, + StatusCanceled, + LegacyStatusCanceled, + StatusExpired, + StatusRevocationRequested, +} + // IsCancelable reports whether an execution may still be canceled. Only the // pre-approval states ("pending"/"notified") qualify. A "scheduled" row is not // canceled: it is revoked through the revoke endpoint diff --git a/internal/database/postgres/migrations/000053_executions_account_fk_restrict_test.go b/internal/database/postgres/migrations/000053_executions_account_fk_restrict_test.go index 9b3c7e52..919cad82 100644 --- a/internal/database/postgres/migrations/000053_executions_account_fk_restrict_test.go +++ b/internal/database/postgres/migrations/000053_executions_account_fk_restrict_test.go @@ -80,8 +80,12 @@ func TestMigration_ExecutionsAccountFKRestrict(t *testing.T) { "expected FK violation, got %v", err) // RESTRICT applies to every referencing row regardless of status, so the - // account delete only succeeds once the execution row itself is removed - // (the API layer's Cancel-All-And-Delete flow does exactly that). + // account delete only succeeds once no execution row references it. The + // API never deletes execution rows for this: Cancel-All-And-Delete only + // cancels them, and config.PostgresStore.DeleteCloudAccount detaches + // terminal rows (cloud_account_id = NULL) in its own transaction before + // the DELETE (issue #265). Removing the row is just the shortest way to + // show the constraint stops binding once nothing references the account. _, err = pool.Exec(ctx, ` DELETE FROM purchase_executions WHERE execution_id = 'eeeeeeee-eeee-eeee-eeee-eeeeeeee0001' diff --git a/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.down.sql b/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.down.sql new file mode 100644 index 00000000..c32d9699 --- /dev/null +++ b/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.down.sql @@ -0,0 +1,8 @@ +-- Restore the shared updated_at trigger from migration 000001, which stamps +-- NOW() on every UPDATE, including a detach from a deleted account. +DROP TRIGGER IF EXISTS update_purchase_executions_updated_at ON purchase_executions; +CREATE TRIGGER update_purchase_executions_updated_at + BEFORE UPDATE ON purchase_executions + FOR EACH ROW EXECUTE FUNCTION update_updated_at_column(); + +DROP FUNCTION IF EXISTS purchase_executions_set_updated_at(); diff --git a/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.up.sql b/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.up.sql new file mode 100644 index 00000000..779b8e77 --- /dev/null +++ b/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.up.sql @@ -0,0 +1,37 @@ +-- 000127: keep purchase_executions.updated_at when a row is only detached from +-- a deleted cloud account (issue #265). +-- +-- DeleteCloudAccount now detaches the account's terminal executions +-- (cloud_account_id = NULL) so they survive the delete as history instead of +-- tripping migration 000053's ON DELETE RESTRICT FK. The shared +-- update_updated_at_column() trigger would stamp NOW() on every detached row, +-- and updated_at is read as a lifecycle clock: CountExecutionsByPlanAndStatus +-- windows the plan health score's failed/canceled counts on it, and +-- CleanupOldExecutions ages canceled rows by it. Deleting an account would +-- then pull its old failures back into every plan's health window and restart +-- the retention clock on its canceled rows. +-- +-- This trigger function keeps OLD.updated_at for an UPDATE whose only change +-- is cloud_account_id going from a value to NULL, and stamps NOW() on every +-- other UPDATE exactly as before. The whole-row comparison runs only inside +-- the detach branch, so ordinary updates pay for one extra NULL check. + +CREATE OR REPLACE FUNCTION purchase_executions_set_updated_at() +RETURNS TRIGGER AS $$ +BEGIN + IF OLD.cloud_account_id IS NOT NULL AND NEW.cloud_account_id IS NULL THEN + IF (to_jsonb(NEW) - 'cloud_account_id' - 'updated_at') + = (to_jsonb(OLD) - 'cloud_account_id' - 'updated_at') THEN + NEW.updated_at = OLD.updated_at; + RETURN NEW; + END IF; + END IF; + NEW.updated_at = NOW(); + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +DROP TRIGGER IF EXISTS update_purchase_executions_updated_at ON purchase_executions; +CREATE TRIGGER update_purchase_executions_updated_at + BEFORE UPDATE ON purchase_executions + FOR EACH ROW EXECUTE FUNCTION purchase_executions_set_updated_at(); diff --git a/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at_test.go b/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at_test.go new file mode 100644 index 00000000..e8a1abd9 --- /dev/null +++ b/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at_test.go @@ -0,0 +1,96 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMigration_ExecutionsDetachKeepsUpdatedAt locks down migration 000127: +// an UPDATE that only detaches a purchase_executions row from its cloud +// account keeps updated_at, every other UPDATE still stamps NOW(), and the +// down migration restores the stamp-everything trigger (issue #265). +func TestMigration_ExecutionsDetachKeepsUpdatedAt(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + + const acct = "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + old := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC) + + // seed inserts a failed execution on acct with an old updated_at (an + // INSERT does not fire the BEFORE UPDATE trigger) and returns its id. + seed := func(t *testing.T) string { + t.Helper() + var id string + require.NoError(t, pool.QueryRow(ctx, ` + INSERT INTO purchase_executions (status, step_number, scheduled_date, cloud_account_id, updated_at) + VALUES ('failed', 1, NOW(), $1, $2) + RETURNING execution_id::text`, acct, old).Scan(&id)) + return id + } + updatedAt := func(t *testing.T, id string) time.Time { + t.Helper() + var ts time.Time + require.NoError(t, pool.QueryRow(ctx, + `SELECT updated_at FROM purchase_executions WHERE execution_id = $1`, id).Scan(&ts)) + return ts.UTC() + } + exec := func(t *testing.T, sql, id string) { + t.Helper() + _, err := pool.Exec(ctx, sql, id) + require.NoError(t, err) + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 127)) + _, err := pool.Exec(ctx, ` + INSERT INTO cloud_accounts (id, name, provider, external_id) + VALUES ($1, 'detach-test', 'azure', $2)`, acct, acct) + require.NoError(t, err) + + t.Run("detach only keeps updated_at", func(t *testing.T) { + id := seed(t) + exec(t, `UPDATE purchase_executions SET cloud_account_id = NULL WHERE execution_id = $1`, id) + assert.True(t, updatedAt(t, id).Equal(old), "a detach-only UPDATE must keep updated_at") + }) + + t.Run("detach plus another change stamps NOW", func(t *testing.T) { + id := seed(t) + exec(t, `UPDATE purchase_executions SET cloud_account_id = NULL, error = 'x' WHERE execution_id = $1`, id) + assert.True(t, updatedAt(t, id).After(old), "a detach that also changes another column must stamp NOW()") + }) + + t.Run("ordinary update stamps NOW", func(t *testing.T) { + id := seed(t) + exec(t, `UPDATE purchase_executions SET status = 'canceled' WHERE execution_id = $1`, id) + assert.True(t, updatedAt(t, id).After(old), "a status change must stamp NOW()") + }) + + t.Run("attach stamps NOW", func(t *testing.T) { + id := seed(t) + exec(t, `UPDATE purchase_executions SET cloud_account_id = NULL WHERE execution_id = $1`, id) + _, err := pool.Exec(ctx, `UPDATE purchase_executions SET cloud_account_id = $2 WHERE execution_id = $1`, id, acct) + require.NoError(t, err) + assert.True(t, updatedAt(t, id).After(old), "setting an account must stamp NOW()") + }) + + t.Run("down restores the shared trigger", func(t *testing.T) { + id := seed(t) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 126)) + exec(t, `UPDATE purchase_executions SET cloud_account_id = NULL WHERE execution_id = $1`, id) + assert.True(t, updatedAt(t, id).After(old), "after the down migration a detach stamps NOW() again") + + var n int + require.NoError(t, pool.QueryRow(ctx, + `SELECT COUNT(*) FROM pg_proc WHERE proname = 'purchase_executions_set_updated_at'`).Scan(&n)) + assert.Zero(t, n, "down must drop the 000127 trigger function") + }) +} diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index e2fbd3e5..af3464c9 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -50,6 +50,7 @@ type MockConfigStore struct { GetPlanAccountsFn func(ctx context.Context, planID string) ([]config.CloudAccount, error) SaveAccountServiceOverrideFn func(ctx context.Context, override *config.AccountServiceOverride) error CountPendingExecutionsForAccountFn func(ctx context.Context, accountID string) (int, error) + CountLiveExecutionsForAccountFn func(ctx context.Context, accountID string) (int, error) ListPendingExecutionIDsForAccountFn func(ctx context.Context, accountID string) ([]string, error) SavePurchaseExecutionFn func(ctx context.Context, exec *config.PurchaseExecution) error PinExecutionFanOutShapeFn func(ctx context.Context, lineageKey, shape string) (string, error) @@ -673,6 +674,20 @@ func (m *MockConfigStore) CountPendingExecutionsForAccount(ctx context.Context, return args.Int(0), args.Error(1) } +// CountLiveExecutionsForAccount mocks the CountLiveExecutionsForAccount operation. +// Defaults to (0, nil) when no Fn is set and no expectation is registered. +func (m *MockConfigStore) CountLiveExecutionsForAccount(ctx context.Context, accountID string) (int, error) { + m.record("CountLiveExecutionsForAccount", ctx, accountID) + if m.CountLiveExecutionsForAccountFn != nil { + return m.CountLiveExecutionsForAccountFn(ctx, accountID) + } + if !isExpected(&m.Mock, "CountLiveExecutionsForAccount") { + return 0, nil + } + args := m.Called(ctx, accountID) + return args.Int(0), args.Error(1) +} + // ListPendingExecutionIDsForAccount mocks the ListPendingExecutionIDsForAccount operation. // Defaults to (nil, nil) when no Fn is set and no expectation is registered. func (m *MockConfigStore) ListPendingExecutionIDsForAccount(ctx context.Context, accountID string) ([]string, error) { diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 2938c89f..6a9398a8 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -150,6 +150,10 @@ func (m *mockConfigStoreForHealth) CountPendingExecutionsForAccount(ctx context. return 0, nil } +func (m *mockConfigStoreForHealth) CountLiveExecutionsForAccount(ctx context.Context, accountID string) (int, error) { + return 0, nil +} + func (m *mockConfigStoreForHealth) ListPendingExecutionIDsForAccount(ctx context.Context, accountID string) ([]string, error) { return nil, nil } From 510ea410762126d2dd37680ed15ee59d76035565 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 17 Oct 2026 16:27:50 +0100 Subject: [PATCH 2/3] fix(db): renumber executions-detach migration to 000128 after #905 #905 merged its own 000127 (never_started_legacy_ramps_to_new), so this PR's migration moves to 000128. The down step in its test now targets the real predecessor instead of a hard-coded 126. Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres.go | 2 +- ...=> 000128_executions_detach_keeps_updated_at.down.sql} | 0 ...l => 000128_executions_detach_keeps_updated_at.up.sql} | 2 +- ... => 000128_executions_detach_keeps_updated_at_test.go} | 8 ++++---- 4 files changed, 6 insertions(+), 6 deletions(-) rename internal/database/postgres/migrations/{000127_executions_detach_keeps_updated_at.down.sql => 000128_executions_detach_keeps_updated_at.down.sql} (100%) rename internal/database/postgres/migrations/{000127_executions_detach_keeps_updated_at.up.sql => 000128_executions_detach_keeps_updated_at.up.sql} (96%) rename internal/database/postgres/migrations/{000127_executions_detach_keeps_updated_at_test.go => 000128_executions_detach_keeps_updated_at_test.go} (96%) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 312eec38..3b4d7d46 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3947,7 +3947,7 @@ func (s *PostgresStore) DeleteCloudAccount(ctx context.Context, id string) error // the delete for every referencing row whatever its status. Live rows are // left attached, so the DELETE below still fails on the FK (SQLSTATE // 23503) while any remain, and this UPDATE rolls back with it. Migration - // 000127 keeps updated_at unchanged on these rows: detaching is not a + // 000128 keeps updated_at unchanged on these rows: detaching is not a // lifecycle event, and the plan health score and the retention sweep both // read updated_at as one. if _, err = tx.Exec(ctx, ` diff --git a/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.down.sql b/internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at.down.sql similarity index 100% rename from internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.down.sql rename to internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at.down.sql diff --git a/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.up.sql b/internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at.up.sql similarity index 96% rename from internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.up.sql rename to internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at.up.sql index 779b8e77..488773d6 100644 --- a/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.up.sql +++ b/internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at.up.sql @@ -1,4 +1,4 @@ --- 000127: keep purchase_executions.updated_at when a row is only detached from +-- 000128: keep purchase_executions.updated_at when a row is only detached from -- a deleted cloud account (issue #265). -- -- DeleteCloudAccount now detaches the account's terminal executions diff --git a/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at_test.go b/internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at_test.go similarity index 96% rename from internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at_test.go rename to internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at_test.go index e8a1abd9..206b15ea 100644 --- a/internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at_test.go +++ b/internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at_test.go @@ -13,7 +13,7 @@ import ( "github.com/stretchr/testify/require" ) -// TestMigration_ExecutionsDetachKeepsUpdatedAt locks down migration 000127: +// TestMigration_ExecutionsDetachKeepsUpdatedAt locks down migration 000128: // an UPDATE that only detaches a purchase_executions row from its cloud // account keeps updated_at, every other UPDATE still stamps NOW(), and the // down migration restores the stamp-everything trigger (issue #265). @@ -50,7 +50,7 @@ func TestMigration_ExecutionsDetachKeepsUpdatedAt(t *testing.T) { require.NoError(t, err) } - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 127)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 128)) _, err := pool.Exec(ctx, ` INSERT INTO cloud_accounts (id, name, provider, external_id) VALUES ($1, 'detach-test', 'azure', $2)`, acct, acct) @@ -84,13 +84,13 @@ func TestMigration_ExecutionsDetachKeepsUpdatedAt(t *testing.T) { t.Run("down restores the shared trigger", func(t *testing.T) { id := seed(t) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 126)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousMigrationVersion(t, 128))) exec(t, `UPDATE purchase_executions SET cloud_account_id = NULL WHERE execution_id = $1`, id) assert.True(t, updatedAt(t, id).After(old), "after the down migration a detach stamps NOW() again") var n int require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM pg_proc WHERE proname = 'purchase_executions_set_updated_at'`).Scan(&n)) - assert.Zero(t, n, "down must drop the 000127 trigger function") + assert.Zero(t, n, "down must drop the 000128 trigger function") }) } From 29fc68aae461ca9cf4059a040c7841e63d224dd1 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 17 Oct 2026 16:40:58 +0100 Subject: [PATCH 3/3] fix(api): re-run the delete preflight after an FK-violation race When DeleteCloudAccount hits SQLSTATE 23503 because a row raced past the preflight, re-query so the client gets the structured pending (with counts) or active 409 for that row. Fall back to the bare pending 409 if the re-query fails or no longer sees a blocker (CodeRabbit on #1020). Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_accounts.go | 14 +++-- internal/api/handler_accounts_test.go | 87 +++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 0ae3368a..a0e51ee1 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -620,13 +620,17 @@ func (h *Handler) deleteAccount(ctx context.Context, req *events.LambdaFunctionU // concurrently before we issued the DELETE. Migration 000053 enforces // ON DELETE RESTRICT, so Postgres raises SQLSTATE 23503 // (foreign_key_violation). Terminal executions never cause this: - // DeleteCloudAccount detaches them in the same transaction. Map it to - // the same structured 409 the preflight returns so the frontend's - // Cancel-All-Then-Delete affordance still kicks in — minus the - // count/ids, which we can't supply without re-querying (and the - // operator can just retry). + // DeleteCloudAccount detaches them in the same transaction. Re-run the + // preflight so the client gets the same structured 409 (pending with + // counts and ids, or active), and fall back to a bare pending 409 if + // the re-query fails or no longer sees the blocker. var pgErr *pgconn.PgError if errors.As(err, &pgErr) && pgErr.Code == "23503" { + if perr := h.preflightAccountDelete(ctx, id); perr != nil { + if ce, ok := IsClientError(perr); ok { + return nil, ce + } + } return nil, NewClientErrorWithDetails(409, "cannot delete account: pending purchase(s) must be canceled first", map[string]any{ diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index 78c62a3b..bf6dd62d 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -723,6 +723,93 @@ func TestDeleteAccount_FKViolationRace_Returns409(t *testing.T) { assert.Equal(t, "pending_executions", details["reason"]) } +// TestDeleteAccount_FKViolationRace_RequeriesPreflight asserts that after a +// 23503 race the handler re-runs the preflight, so the client gets the +// specific structured 409 for whatever row raced in: pending (with its count) +// or active (approved/running/scheduled/paused), not a generic pending 409. +func TestDeleteAccount_FKViolationRace_RequeriesPreflight(t *testing.T) { + cases := []struct { + name string + pending int + live int + wantReason string + wantKey string + }{ + {"pending row raced in", 2, 0, "pending_executions", "pending_count"}, + {"active row raced in", 0, 1, "active_executions", "active_count"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + store := setupAdminMock(ctx) + pendingCalls, liveCalls := 0, 0 + // First call is the preflight (nothing yet); the second is the + // re-query after the FK violation, which sees the raced-in row. + store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { + pendingCalls++ + if pendingCalls == 1 { + return 0, nil + } + return tc.pending, nil + } + store.CountLiveExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { + liveCalls++ + if liveCalls == 1 { + return 0, nil + } + return tc.live, nil + } + store.DeleteCloudAccountFn = func(_ context.Context, _ string) error { + return &pgconn.PgError{Code: "23503"} + } + handler := &Handler{auth: mockAuth, config: store} + + _, err := handler.deleteAccount(ctx, adminRequest(""), "11111111-1111-1111-1111-111111111111") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError (409), got %T: %v", err, err) + assert.Equal(t, 409, ce.code) + details := ce.Details() + require.NotNil(t, details) + assert.Equal(t, tc.wantReason, details["reason"]) + assert.Contains(t, details, tc.wantKey, "re-queried 409 carries the count") + }) + } +} + +// TestDeleteAccount_FKViolationRace_RequeryErrorFallsBack asserts that if the +// re-query itself fails, the handler still returns the bare pending 409 +// rather than surfacing the re-query's 500. +func TestDeleteAccount_FKViolationRace_RequeryErrorFallsBack(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + store := setupAdminMock(ctx) + calls := 0 + store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { + calls++ + if calls == 1 { + return 0, nil + } + return 0, errors.New("connection reset") + } + store.DeleteCloudAccountFn = func(_ context.Context, _ string) error { + return &pgconn.PgError{Code: "23503"} + } + handler := &Handler{auth: mockAuth, config: store} + + _, err := handler.deleteAccount(ctx, adminRequest(""), "11111111-1111-1111-1111-111111111111") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError (409), got %T: %v", err, err) + assert.Equal(t, 409, ce.code) + assert.Equal(t, "pending_executions", ce.Details()["reason"]) +} + // TestDeleteAccount_NonFKError_StillReturns500 asserts that errors which // are NOT FK violations continue to bubble up as wrapped errors (500), // so we don't accidentally swallow real failures as 409s.