fix(config): let an account with only terminal executions be deleted

[IAmJSD]

Closes #265

Root cause

Migration 000053 made purchase_executions.cloud_account_id ON DELETE RESTRICT, so the FK refuses the delete for every referencing row, whatever its status. The deleteAccount preflight only counts pending/notified rows. An account with one completed, failed, canceled or expired execution passed the preflight, hit the FK, and got a 409 saying "pending purchase(s) must be canceled first" when there was nothing to cancel. CleanupOldExecutions never purges failed rows, so the account could never be deleted through the API.

Reproduced on origin/main (173474e) against real Postgres: DeleteCloudAccount returned SQLSTATE 23503 for an account whose only execution was in each of the 7 terminal statuses.

Fix

• config.TerminalExecutionStatuses (new allowlist): completed, partially_completed, failed, canceled, cancelled, expired, revocation_requested. It is an allowlist on purpose, so a status added to the schema later counts as live and blocks the delete until someone classifies it. A new integration test (TestTerminalExecutionStatuses_CoverSchema) fails if the terminal and live lists stop matching the purchase_executions_status_check constraint exactly.
• DeleteCloudAccount detaches the account's terminal executions (SET cloud_account_id = NULL WHERE cloud_account_id = $1 AND status = ANY(terminal)) inside its existing transaction, just before the DELETE. Live rows stay attached, so the RESTRICT FK still refuses the delete while any are left, and the detach rolls back along with it.
• New CountLiveExecutionsForAccount counts the account's rows with status <> ALL(terminal). deleteAccount now runs its preflight in preflightAccountDelete:
  1. pending/notified rows: the existing fix(migrations): enforce cancellation and timeout on the real migration runner #606 409 (reason: pending_executions, pending_count, pending_execution_ids). The Cancel-All-Then-Delete UX is unchanged.
  2. Any other live row (approved, running, scheduled, paused): a new 409 with reason: active_executions and active_count. Its message says to wait for them to finish, or revoke or delete them, not to cancel them. The frontend shows it through its generic error toast, so no frontend change is needed.
• Corrected the 000053 test comment that said the Cancel-All flow deletes execution rows.

Retention semantics

• Terminal execution rows are kept. Status, recommendations (with their per-rec cloud_account_id), amounts and audit stamps are untouched. Only the row-level cloud_account_id becomes NULL. That matches what purchase_history, savings_snapshots and ri_exchange_history already get from their ON DELETE SET NULL FKs, and it is what this FK did before 000053 tightened it for live rows.
• A live (non-terminal) execution still blocks the delete: first in the preflight with a clear 409, then at the FK if one races in. In-flight executions are never orphaned.
• A detached failed row can still be sent to retry, but the retry cannot buy anywhere else. Its recommendations still name the deleted account, so the executor fails it before submission (credential resolution failed ... account not found, fix(server): exclude uninitialized replicas from readiness #646) and never falls back to ambient credentials.
• revocation_requested counts as terminal: the system does nothing further with it (the revocation is an AWS support case), and no code moves a row out of it, so blocking on it would bring this bug back for those accounts.

Migration 000127

000127_executions_detach_keeps_updated_at: the shared update_updated_at_column() trigger would stamp NOW() on every detached row. Several readers treat updated_at 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 each plan's health window and restart retention on its canceled rows. The migration gives purchase_executions its own trigger function. It keeps OLD.updated_at when the only change in an UPDATE is cloud_account_id going from a value to NULL, and stamps NOW() on every other UPDATE as before. The whole-row comparison only runs inside the detach branch.

• The down migration puts back the 000001 trigger (update_updated_at_column()) and drops the new function.
• 000127 is the next free number: origin/main tops out at 000126, and no open PR adds a migration at 000120 or above.
• Test: 000127_executions_detach_keeps_updated_at_test.go covers a detach-only update (kept), a detach plus another column change (stamped), a status change (stamped), re-attaching (stamped), and the down migration (stamps again, function dropped).

Verification

All tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. No cloud calls.

• Real-Postgres integration (testcontainers), internal/config/store_postgres_delete_account_integration_test.go:
  • TestDeleteCloudAccount_TerminalExecutionsAreDetached, one subtest per terminal status: the delete succeeds, the row survives with its status, cloud_account_id is NULL and updated_at is unchanged. Fails on origin/main (SQLSTATE 23503 for all 7) and passes with this change.
  • TestDeleteCloudAccount_LiveExecutionBlocks, one subtest per live status: the count is 1, the delete fails with 23503, the account survives, and both the live row and a sibling failed row stay attached (the transaction rolls back whole).
  • TestDeleteCloudAccount_DetachIsScopedToAccount, TestCountLiveExecutionsForAccount, TestTerminalExecutionStatuses_CoverSchema.
• Handler unit tests: TestDeleteAccount_ActiveExecutions_Returns409, _PendingTakesPrecedenceOverActive, _OnlyTerminalExecutions_Deletes, _LiveCountErr_Returns500. Three of these fail against the origin/main handler. The existing fix(migrations): enforce cancellation and timeout on the real migration runner #606 tests still pass unchanged.
• pgxmock: the DeleteCloudAccount tests now expect the detach with exactly TerminalExecutionStatuses, plus new tests for a failed detach (rollback) and for CountLiveExecutionsForAccount.
• Mutation probes, 9/9 killed: detach removed; detach of every status; detach not scoped to the account; live count using = ANY; running classified as terminal; expired left unclassified; trigger always bumping; trigger skipping the row diff; down migration leaving the function.
• These pass: go build ./..., go vet ./..., go vet -tags integration ./..., and unit tests for internal/{config,api,mocks,server,analytics} and internal/database/postgres/migrations (which includes TestMigrationsAreTransactional).
• Full integration suites pass: go test -tags integration ./internal/config/ and ./internal/database/postgres/migrations/. In the migrations run, one test hit a testcontainers start-up error once (port "5432/tcp" not found) while other jobs were running at the same time; it passed when re-run on its own.
• golangci-lint v2.10.1 (--allow-parallel-runners, with and without --build-tags integration): 0 issues. gocyclo -over 10 on the changed non-test files: none. git-secrets --scan on the changed files in a temp clone with an isolated HOME: clean. scripts/test-git-secrets-allowlist.sh: 42 PASS.
• Not covered: no real-account or deployed-environment check (no cloud calls), and no browser check of the account page's new 409 toast.

Out of scope / interactions

• fix(config): deleting a cloud account leaves its scoped plans enabled with zero accounts #264 (deleting an account leaves its scoped plans enabled) is not fixed here. This change lets more accounts be deleted, so that state is reachable more often, but it does not change what happens to plans: plan_accounts rows still cascade, and plans keep their current enabled state. One interaction to know about: the ramp-step unit CTE (store_postgres_ramp_step.go) treats rows with no account as root rows. While any account row is left on a step it ignores them, so detaching changes nothing there. If every account on a fanned-out plan is deleted (the fix(config): deleting a cloud account leaves its scoped plans enabled with zero accounts #264 state), the detached children join the step's root row in its NULL partition. ever_bought is unchanged (the root already aggregates its children), and because 000127 keeps updated_at, the representative row is picked on the original write times.
• History rows that lost their account show it the way other NULL-account rows do. The per-rec cloud_account_id is still in recommendations, but nothing resolves a deleted account's name.
• No frontend change: the new active_executions 409 shows through the existing generic error toast.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 14 billable files and costs up to $3.50.}

Or wait 38 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: b319ffe1-8d16-4a27-9b3d-c1eebfc05c96

📥 Commits

Reviewing files that changed from the base of the PR and between 173474e and 6fd7d9f.

📒 Files selected for processing: 14

• internal/analytics/collector_test.go
• internal/api/handler_accounts.go
• internal/api/handler_accounts_test.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/config/store_postgres_delete_account_integration_test.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/types.go
• internal/database/postgres/migrations/000053_executions_account_fk_restrict_test.go
• internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.down.sql
• internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at.up.sql
• internal/database/postgres/migrations/000127_executions_detach_keeps_updated_at_test.go
• internal/mocks/stores.go
• internal/server/test_helpers_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review verdict: CLEAN at 6fd7d9f

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief, focused on data loss and migration safety. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none. No execution or audit row is ever deleted.

What an account delete does to each referencing table:

Table	On delete
account_credentials, account_service_overrides, plan_accounts (000011:55,81,115), recommendations_cache (000030), ladder_configs (000079)	CASCADE (unchanged)
purchase_history, savings_snapshots, ri_exchange_history, account_registrations, aws_bastion_id	SET NULL (unchanged)
purchase_executions	RESTRICT (000053). This PR detaches terminal rows (cloud_account_id = NULL) in the same transaction before the DELETE. Live rows stay attached, so the FK still refuses the delete.

Terminal statuses (TerminalExecutionStatuses, an allowlist): completed, partially_completed, failed, canceled, cancelled, expired, revocation_requested. Approved, running, scheduled, paused, pending, notified and any future status count as live.

Concurrency:

• The detach and the DELETE share one transaction (store_postgres.go ~:3878).
• A concurrent execution insert takes a FK key-share lock, so the DELETE blocks or fails with 23503, which maps to 409.
• A row that turns live mid-statement is re-evaluated on its new status.
• No race was found that orphans an in-flight execution.

Migration 000127:

• Main tops out at 000126, so there is no collision.
• It is a BEFORE UPDATE trigger on purchase_executions that keeps OLD.updated_at only when the sole change is cloud_account_id going to NULL. A detach therefore doesn't count as a lifecycle event for health scoring or retention.
• It is idempotent, the down migration restores the 000001 trigger, and there is no table rewrite.

Question for the owner (non-blocking): revocation_requested counts as terminal. On main it only records the user's refund intent; the AWS revocation itself is a manual support case (handler_purchases.go:1872-1876), so no automated process depends on the account link. But once an account is deleted with an open revocation request, the execution loses its account reference. Is the AWS account ID still recoverable elsewhere on the row (e.g. its recommendations payload) for whoever files the support case? If not, consider treating revocation_requested as live so such accounts can't be deleted until the revocation is resolved. @cristim

What was attacked and how:

• go vet ./internal/... is clean. go test -race -count=1 ./internal/api/... ./internal/config/... passes.
• -tags integration -v on config and migrations, against Docker Postgres for real: 93 PASS, 0 FAIL, 0 SKIP, ending at migration version 127.
• Mutation probes, both killed:
  • Adding "running" to the terminal set fails TestDeleteCloudAccount_LiveExecutionBlocks/running.
  • Deleting executions instead of detaching them gives 11 failures, including the integration TerminalExecutionsAreDetached tests.
• Fail-before: a compile failure, since the interface method is new.

Gate:

• Labels are mirrored from fix(config): an account with any terminal execution can never be deleted #265.
• Needs the author to mark the draft ready, then green CI, CodeRabbit, and an answer to the revocation question.

[cristim]

Merge-order note from the #1022 review: this PR adds migration 000127, and #1022 adds 000128. The runner is golang-migrate (migrations/migrate.go:62, m.Up()), which only applies versions above the DB's current one. This PR must merge before #1022, or 000127 will be silently skipped on databases that already ran 000128. Otherwise renumber whichever lands second. (claude-cc-reviewer)