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 8e2ae1dc..a0e51ee1 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -588,12 +588,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 @@ -611,46 +611,26 @@ 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. 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{ @@ -663,6 +643,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 95afab02..bf6dd62d 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, @@ -598,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. diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 666d3b14..5f237b29 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -208,6 +208,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 8769f45e..3b4d7d46 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1964,6 +1964,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 @@ -3896,6 +3916,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. @@ -3920,6 +3942,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 + // 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, ` + 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 16e4d3cc..894897eb 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -1779,6 +1779,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() @@ -1795,6 +1796,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() @@ -1811,6 +1813,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() @@ -1820,6 +1823,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 215109db..300a86dd 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/000128_executions_detach_keeps_updated_at.down.sql b/internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at.down.sql new file mode 100644 index 00000000..c32d9699 --- /dev/null +++ b/internal/database/postgres/migrations/000128_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/000128_executions_detach_keeps_updated_at.up.sql b/internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at.up.sql new file mode 100644 index 00000000..488773d6 --- /dev/null +++ b/internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at.up.sql @@ -0,0 +1,37 @@ +-- 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 +-- (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/000128_executions_detach_keeps_updated_at_test.go b/internal/database/postgres/migrations/000128_executions_detach_keeps_updated_at_test.go new file mode 100644 index 00000000..206b15ea --- /dev/null +++ b/internal/database/postgres/migrations/000128_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 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). +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, 128)) + _, 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, 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 000128 trigger function") + }) +} diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 6da82d22..0e0be017 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 }