[IAmJSD]

Closes #264

Root cause

plan_accounts.account_id is ON DELETE CASCADE (000011), and DeleteCloudAccount never looked at plans. When an account was the only target of an enabled plan, the delete removed that plan's last plan_accounts row and left the plan enabled. That is the unscoped "universal plan" shape that migration 000060 deleted. The notification sweep then kept creating executions for it with no account to buy in. #1020 lets more accounts be deleted (accounts whose executions are all terminal), so this state now comes up more often.

Fix

All in internal/config/store_postgres.go, inside DeleteCloudAccount's existing transaction. No migration.

• New disableSoleAccountPlansTx: runs just before the DELETE. It sets enabled = false on every enabled plan whose only plan_accounts row is the account being deleted, and returns their IDs. After the commit, the IDs are logged at info level. Plans that keep another account are not touched. Plans that are already disabled are not rewritten, so their updated_at (the plan edit token, 000102) does not move. A plan with no accounts at all is not touched either; createPlan writes the plan and its accounts in separate calls, so a plan can briefly exist with none.
  • I chose disable over refusing the delete because it keeps the plan and its history. The admin can re-target it and turn it back on. The sweeps already skip disabled plans (notifications.go: !plan.Enabled), and validatePurchaseScheduling refuses a disabled plan.
  • If the delete is refused (for example by a live execution through the 000053 RESTRICT FK), the disable rolls back with it.
• The account row is now locked first (SELECT 1 FROM cloud_accounts WHERE id = $1 FOR UPDATE). A writer that is attaching this account to a plan (SetPlanAccounts inside a transaction that has not committed yet) holds a KEY SHARE lock on the account row through the plan_accounts FK check. Without the lock, the plan query would run before that writer commits. The DELETE would then wait for the writer and cascade its new row away, which leaves that plan enabled with zero accounts. With the lock, the delete waits for the writer first, so the plan query sees the writer's committed rows.
• TestAccountDeleteWaitsForCommandAccountShare (the editor-footprint race test) now waits on the new lock statement instead of on DELETE FROM cloud_accounts, because that is now where the delete queues behind the footprint's account FOR SHARE. Its assertions are unchanged.

Ramp-step interaction (from #1020's notes): this does not touch purchase_executions. The detach from #1020 and the ramp-step unit CTE in store_postgres_ramp_step.go work as before. The plan is now disabled as well, so no new step rows are created for it.

Verification

Unit tests use pgxmock and integration tests use real Postgres through testcontainers. No cloud calls, no deploy. All tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid.

New real-Postgres tests in internal/config/store_postgres_delete_account_integration_test.go:

• TestDeleteCloudAccount_DisablesPlansLeftWithoutAccounts: covers a sole-account plan (disabled), a plan shared with another account (stays enabled, updated_at unchanged), an already disabled plan (updated_at unchanged), a plan on another account (untouched), and a plan with no accounts (untouched).
• TestDeleteCloudAccount_RefusedDeleteKeepsPlanEnabled: a running execution blocks the delete, and the plan stays enabled.
• TestDeleteCloudAccount_DisablesPlanThatGainedAccountConcurrently: an open transaction moves a plan from account B to account A. The delete of A is started, the test waits until the delete is blocked on a lock, then the writer commits. The plan must end up disabled.

Fail before / pass after:

# on origin/main 34c91bbd with only the new tests added
go test -tags integration ./internal/config/ -run 'TestDeleteCloudAccount_' -count=1
--- FAIL: TestDeleteCloudAccount_DisablesPlansLeftWithoutAccounts   (Should be false: plan still enabled)
--- FAIL: TestDeleteCloudAccount_DisablesPlanThatGainedAccountConcurrently
FAIL
# with the fix
go test -tags integration ./internal/config/ -run 'TestDeleteCloudAccount_|TestAccountDeleteWaitsForCommandAccountShare|TestCountLive|TestTerminalExecutionStatuses' -count=1 -v
all 9 PASS

RefusedDeleteKeepsPlanEnabled passes on main as well. It guards the rollback and is not a regression test.

Mutation probes, 5/5 killed by the integration tests:

Mutation	Killed by
lock removed (FOR UPDATE dropped)	DisablesPlanThatGainedAccountConcurrently, TestAccountDeleteWaitsForCommandAccountShare
NOT EXISTS (other account) dropped	DisablesPlansLeftWithoutAccounts
EXISTS (this account) dropped	DisablesPlansLeftWithoutAccounts (no-account plan)
p.enabled filter dropped	DisablesPlansLeftWithoutAccounts (updated_at of the already disabled plan)
disable call removed	both disable tests

pgxmock: the existing DeleteCloudAccount tests now expect the lock and the disable query. New tests: TestPGXMock_DeleteCloudAccount_LockFailsRollsBack and TestPGXMock_DeleteCloudAccount_DisablePlansFailsRollsBack.

Other checks:

• go build ./..., go vet ./internal/config/, go vet -tags integration ./internal/config/
• go test ./internal/config/ -count=1, go test ./internal/mocks/ -count=1
• Full suite: go test -tags integration ./internal/config/ -count=1
• golangci-lint v2.10.1 run --allow-parallel-runners ./internal/config/, with and without --build-tags integration: 0 issues
• gocyclo -over 10 internal/config/store_postgres.go: silent
• git secrets --scan on all tracked files, in a fresh temp repo with an isolated HOME: clean. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.
• The gosec pre-commit hook was not run because gosec is not installed on this machine. The change only adds parameterized SQL.

Not covered: no deployed-environment or real-cloud check.

Out of scope

• Executor guard. The issue also suggests making executePurchase refuse a plan-scoped execution whose plan has zero accounts. I left that out. Many internal/purchase tests (and the MockConfigStore default) drive plan-backed executions through the single-account path with zero plan accounts, and the issue fix(purchase): a plan crossing zero-to-N cloud accounts between attempts silently changes every idempotency token #97 pinned-single lineage path re-drives that shape on purpose. Changing it is a separate executor decision. Today a plan-backed execution with no recommendations is already refused (ErrPlanStepNoRecommendations), and recommendations that name the deleted account fail credential resolution (fix(server): exclude uninitialized replicas from readiness #646).
• Re-enabling with zero accounts. PUT /api/plans/:id with enabled: true does not check that the plan has accounts, so an admin can turn a disabled plan with no accounts back on. validatePurchaseScheduling still refuses to schedule it (409). A follow-up could add the same check to the update handler.
• Already-created rows. Existing pending or notified root executions for a disabled plan (rows with no account) are not canceled here. They do not reference the account, so they never blocked the delete.
• Frontend. The account-delete response does not list which plans were disabled, and the UI does not tell the admin. The backend logs the IDs. Showing them in the UI would need a response field plus a frontend change, both left out of this PR.
• Deadlock window. A SetPlanAccounts that re-sends {A} for a plan whose only account is already A, at the same moment A is deleted, can deadlock with the delete (plan FOR SHARE then account, against account FOR UPDATE then plan). Postgres detects it and rolls back one side whole. Nothing is left half-done, and the request can be retried.

Maintainer: please mirror the issue's triage labels on this PR: priority/p2, severity/medium, triaged, urgency/this-quarter, impact/few, effort/s, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

Or wait 59 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: 53648d28-a2e6-4900-b08f-96f9b144d6ca

📥 Commits

Reviewing files that changed from the base of the PR and between 34c91bb and 077fdc2.

📒 Files selected for processing: 4

• internal/config/store_postgres.go
• internal/config/store_postgres_delete_account_integration_test.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/store_postgres_writer_race_integration_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 of 077fdc2: draft: blocks ready

Findings

1. BLOCKING. internal/config/store_postgres.go:3933-3942 and 3974-3978 (lock + disableSoleAccountPlansTx): two accounts of the same plan deleted concurrently leave the plan enabled with zero accounts. The lock is per account row (FOR UPDATE ... WHERE id = $1), so deleting A and B takes different locks and nothing serializes them. Under READ COMMITTED each transaction's NOT EXISTS (other account) still sees the other account's uncommitted plan_accounts row, so neither disables the plan; each DELETE then cascades its own row away and both commit. Scenario: plan P enabled with accounts {A, B}, admin (or two admins, or a bulk cleanup) deletes A and B in parallel. Reproduced on real Postgres (testcontainers, postgres:16): a scratch test seeding {A,B} plan and calling DeleteCloudAccount(A) and DeleteCloudAccount(B) from two goroutines released together left the plan enabled with 0 accounts in 39 of 40 iterations. That is exactly the state #264 is meant to remove, and it is the common outcome, not a rare window. The PR's concurrency test covers only an account being attached concurrently. Suggested fix: lock the candidate plan rows before deciding, in a fixed order, e.g. SELECT id FROM purchase_plans WHERE id IN (SELECT plan_id FROM plan_accounts WHERE account_id = $1) ORDER BY id FOR UPDATE before the disable, so the second transaction waits for the first to commit and then re-evaluates (READ COMMITTED re-reads plan_accounts after the wait). Alternatively re-check "enabled plans with zero accounts" after the DELETE in the same transaction and disable those. Add a test with two concurrent deletes (loop or a lock-wait gate like the existing one).

2. NON-BLOCKING, same class: a SetPlanAccounts that removes B from {A, B} concurrently with the delete of A is not serialized either (SetPlanAccounts locks the plan FOR SHARE, but the delete's UPDATE only touches plans that match its WHERE, so it sees B before that commit). The plan-row lock in finding 1 would cover this too. The PR body already documents a related deadlock window; the plan-lock ordering fix should be checked against it.

3. NON-BLOCKING: the PR body lists no tests for managed/ramp plans. disableSoleAccountPlansTx sets enabled = false directly and does not go through the #631 legacy-writer guards or touch dates_managed; I found no reason it must (it does not write dates or tranches), but a one-line test that a managed plan is disabled with its schedule columns unchanged would pin that.

What I attacked

• Tenant scoping: SQL is keyed on the account UUID and plan_accounts; no cross-account or cross-plan rows are touched. Plans on other accounts and shared plans stay enabled with unchanged updated_at (PR test passes; I also ran it).
• Idempotency and refused delete: already disabled plans are not rewritten; a refused delete rolls the disable back (TestDeleteCloudAccount_RefusedDeleteKeepsPlanEnabled passes).
• Concurrent attach (the PR's case): lock-first ordering holds; test passes.
• Concurrent delete of two accounts of one plan: FAILS, finding 1.
• Mutation probe (1 of a handful, core logic): dropped the NOT EXISTS (other account) clause; TestDeleteCloudAccount_DisablesPlansLeftWithoutAccounts fails. Reverted. The race above survives the real code, not a mutant.
• Tests at this SHA: go test ./internal/config/ 849 pass; integration TestDeleteCloudAccount_* plus TestAccountDeleteWaitsForCommandAccountShare 20 pass.
• Scheduler and "disabled": the PR cites notifications.go skipping !plan.Enabled and validatePurchaseScheduling refusing a disabled plan; I did not find a contradiction, but disabling does not cancel already-created pending executions (the PR notes this as out of scope).
• History/audit: the disable is a direct UPDATE with no history row; updated_at is left to whatever trigger exists (not re-verified). The deletion is only logged at info.

Gate status (origin/main Review gate)

• Independent adversarial review of the full diff naming the SHA: this comment; verdict blocks ready.
• Actionable findings resolved: no, finding 1 is open. CodeRabbit skipped (draft); no threads.
• CI on 077fdc2: all listed checks pass (Unit, Integration, E2E, Lint, pre-commit, security); fork run approval not an issue as checks ran.
• Local verification on macOS: real Postgres integration tests run and pass, but they do not exercise finding 1.
• Verdict, SHA and evidence on the PR: this comment.
• Labels: match the closing issue per the PR body list (priority/p2, severity/medium, triaged, urgency/this-quarter, impact/few, effort/s, type/bug).
• Size: 47 additions in production code, 165 in tests, under the cap. The branch is 2 commits behind main; re-check after rebase.

Blocks ready

• Finding 1 (two-account concurrent delete leaves plan enabled with zero accounts) plus a regression test for it.

[IAmJSD]

@cristim This merged at 077fdc2 with the review's BLOCKING finding 1 still open. Two concurrent deletes of a plan's accounts can still leave the plan enabled with zero accounts. Was that deliberate? I've tracked it as #1072, and the fix is written: plan rows are locked in id order before the account row, which also serializes SetPlanAccounts. It has fail-before tests for both races. I'll open the PR once I can re-run the full integration suite locally, since this machine is low on disk right now.