[IAmJSD]

Closes #1072

Follow-up to #1065. This fixes the BLOCKING finding in the maintainer's review: #1065 (review). #1065 merged at 077fdc2 with that finding still open.

The races

#1065 locked only the deleted account's row before disableSoleAccountPlansTx decided which plans to turn off. Under READ COMMITTED that decision could rely on plan_accounts rows that another transaction was about to remove:

1. Two deletes of one plan's accounts (the review's finding 1). Deleting A and deleting B take different account locks. Each transaction's NOT EXISTS (other account) still sees the other account's committed row, so neither disables the plan. Both cascades then commit, and the plan is left enabled with zero accounts.
2. SetPlanAccounts narrowing {A, B} to {A} while A is deleted (finding 2). On main this shows up as a deadlock, not a lost update. SetPlanAccounts deletes every row of the plan and then reinserts (P, A), which needs KEY SHARE on A. The delete already holds A FOR UPDATE and then needs the row SetPlanAccounts deleted. Postgres aborts one side (SQLSTATE 40P01).

Fix: lock order

DeleteCloudAccount now calls the new lockAccountForDeleteTx first. It takes these locks in order:

1. Every plan with a plan_accounts row for the account, ORDER BY p.id FOR UPDATE OF p (lockAccountPlansTx).
2. The account row, FOR UPDATE (as in fix(config): disable plans left with no accounts when their account is deleted #1065).
3. The same plan query again. This catches any plan that gained the account while step 2 waited for the writer attaching it.

After that come the registration reset, the terminal-execution detach, the disable and the DELETE, all unchanged and all in the same transaction.

• Race 1: the second delete waits on the plan lock. Once the first commits, it reads plan_accounts again, sees only its own account, and disables the plan.
• Race 2: SetPlanAccounts reads the plan FOR SHARE, which conflicts with FOR UPDATE, so the two run one after the other. The plan is locked before the account, which is the same order SetPlanAccounts uses (plan, then the account's KEY SHARE when it inserts), so the deadlock is gone. If the delete commits first, SetPlanAccounts fails on the FK. If SetPlanAccounts commits first, the delete sees {A} and disables the plan.
• Why step 3 is needed: a writer can move plan P onto A while the delete of A waits in step 2. Meanwhile the delete of another of P's accounts locks P, decides to keep it, and is still committing. Without the relock, the delete of A would decide on the stale set and leave P enabled with no accounts. TestDeleteCloudAccount_RelocksPlanThatGainedAccountDuringWait covers this.
• Editor footprint race test: TestAccountDeleteWaitsForCommandAccountShare now waits on %FOR UPDATE OF p%, because the delete queues on the plan lock first now, behind the footprint's plan FOR UPDATE. Its assertions are unchanged.

I chose the plan-row lock over the review's alternative (re-checking after the DELETE) on purpose. With the re-check, two concurrent deletes would each still see the other's uncommitted cascade as committed rows, so neither would disable the plan.

Remaining deadlock window

A plan that gains the account only after step 1 is locked in step 3, after the account row. If a second SetPlanAccounts on that same plan re-adds the account at that moment, it holds the plan FOR SHARE and waits for the account, while the delete holds the account and waits for the plan. Postgres detects this and rolls one side back whole, so nothing is left half-done and the request can be retried. It needs a plan-account write that commits during the delete's account wait, followed by a second one on the same plan.

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 tests in internal/config/store_postgres_delete_account_integration_test.go:

• TestDeleteCloudAccount_ConcurrentDeletesOfOnePlansAccounts (finding 1): a gate transaction holds the plan's plan_accounts rows FOR UPDATE. Both deletes reach their cascade after deciding, and the test waits for 2 lock waiters before releasing the gate. This forces the race on every run.
• TestDeleteCloudAccount_ConcurrentSetPlanAccounts (finding 2): 20 iterations of the delete of A racing SetPlanAccounts(P, [A]). Any 40P01, or a plan that ends up enabled with no accounts, fails the test.
• TestDeleteCloudAccount_RelocksPlanThatGainedAccountDuringWait: lock-gated. A writer moves P from {B} to {A, C}, and A and C are deleted while it is still open. The delete of C is held at its cascade by a gate.
• TestDeleteCloudAccount_DisablesManagedRampPlanKeepingSchedule (finding 3): a dates_managed ramp plan is disabled, and ramp_schedule, ramp_baseline, ramp_ledger_state, services and next_execution_date are unchanged.

Fail before: origin/main's (8caed39) store_postgres.go with the new tests:

go test -tags integration ./internal/config/ -run 'TestDeleteCloudAccount_(Concurrent|Relocks|DisablesManaged)' -count=1 -v
--- FAIL: TestDeleteCloudAccount_ConcurrentDeletesOfOnePlansAccounts   both accounts are gone, so the plan must be disabled
--- FAIL: TestDeleteCloudAccount_ConcurrentSetPlanAccounts            iteration 1: deadlock: failed to insert plan account: ERROR: deadlock detected (SQLSTATE 40P01)
--- FAIL: TestDeleteCloudAccount_RelocksPlanThatGainedAccountDuringWait A and C are both gone, so the plan must be disabled
--- PASS: TestDeleteCloudAccount_DisablesManagedRampPlanKeepingSchedule  (guard, not a regression test)

Pass after: all 13 TestDeleteCloudAccount_* tests, TestAccountDeleteWaitsForCommandAccountShare, TestCountLiveExecutionsForAccount and TestTerminalExecutionStatuses_CoverSchema pass. The relock test passed 3/3 with -count=3.

Mutation probes (integration delete tests plus the footprint race test):

Mutation	Result
first plan lock removed	killed: ConcurrentSetPlanAccounts (40P01), TestAccountDeleteWaitsForCommandAccountShare
second plan lock (relock) removed	killed: RelocksPlanThatGainedAccountDuringWait
plan lock FOR UPDATE changed to FOR SHARE	killed: 4 tests
account row lock removed	killed: DisablesPlanThatGainedAccountConcurrently, RelocksPlan…
whole lock sequence removed	killed: 5 tests
disable call removed	killed: 6 tests
ORDER BY p.id removed	survived (see below)

ORDER BY is there so two deletes that share several plans take the plan locks in the same order. On these small fixtures both scans return purchase_plans in heap order anyway, so no deterministic test catches its removal. I kept it as cheap deadlock insurance.

pgxmock: every DeleteCloudAccount test now expects plan lock, account lock, plan lock. New: TestPGXMock_DeleteCloudAccount_PlanLockFailsRollsBack, which fails the first and then the second plan lock and checks for a rollback.

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 passes (370s). One earlier run of the full suite failed once in TestRampRetention_FreshEditsRequireContinuousUnheldLineage. That test does not touch account deletes or plan_accounts (it exercises UpdatePurchasePlan and the ramp ledger trigger). It passed 3/3 on its own, and the next full-suite run was clean. I did not capture that failure's output, so I can't say more about its cause.
• 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. The lock sequence is split into lockAccountForDeleteTx so DeleteCloudAccount stays under 10.
• 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.

Not covered: no deployed-environment or real-cloud check. gosec was not run because it is not installed here; the change only adds parameterized SQL.

Out of scope

These are carried over from #1065 and not changed here:

• The executor guard for plans with zero accounts.
• Re-enabling a plan with no accounts through PUT /api/plans/:id.
• Canceling existing root executions of a disabled plan.
• Telling the admin in the UI which plans were disabled (needs a frontend change).

Commits

289c99dd is the WIP commit of the plan-lock change. 656eb4a8 merges origin/main; its conflicts were only between the squash-merged #1065 and the branch's own copy of the same code. 97015fcc adds the relock test, the pgxmock and race-test updates, the helper split and the comment narrowing.

Maintainer: please mirror the issue's triage labels on this PR: priority/p2, severity/medium, 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

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 8caed39
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3690675c-9744-4bbb-b45e-14a843e03ba6

📥 Commits

Reviewing files that changed from the base of the PR and between 8caed39 and 97015fc.

📒 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]

VERDICT: APPROVE 97015fc

Base: merge-base 8caed39, 39 commits behind origin/main 56e226a. git merge-tree --write-tree origin/main 97015fcc merges clean (tree d65316b8). All tests below ran on that merged tree.

Findings

• nit (process) - largest non-merge commit is 077fdc2 at 213 changed lines (the pre-squash fix(config): disable plans left with no accounts when their account is deleted #1065 commit carried in branch history); 289c99d 145, 97015fc 154. Under the 400 cap. Merge commit 656eb4a excluded.
• nit internal/config/store_postgres.go:4019 - FOR UPDATE OF p also blocks child-row inserts that take FK KEY SHARE on the plan (purchase_executions, purchase_history) for the delete's lifetime. FOR NO KEY UPDATE passes every race test (probe M4 below fails only the waiter test, whose pg_stat_activity pattern matches the literal FOR UPDATE OF p). FOR UPDATE is the safer choice because it also serializes any future plan_accounts writer that relies only on the FK lock, so I would keep it; noting the trade-off only.
• nit internal/config/store_postgres.go:4005-4011 - the residual deadlock (plan gained after step 1, then re-touched by a second SetPlanAccounts during step 3) is documented in the PR body but not in the code comment. 40P01 rolls back whole, so it is safe; a one-line note would help the next reader.

Lock ordering, checked against every lock site in internal/ (non-test):

• SetPlanAccounts / setPlanAccountsTx (store_postgres_plan_writes.go:66, store_postgres.go:4330): plan FOR SHARE (requireManagedAccountsTx, getPlanServicesForShareTx), then accounts FOR SHARE, then plan_accounts KEY SHARE. Plan then account, same as the delete.
• UpdatePurchasePlan, CompletePlanStep, RescheduleAwaitingExecution, claimPlanStep, notifications: LockPurchasePlanTx (plan FOR UPDATE), one plan per tx, accounts only after. Consistent.
• Execution/history inserts: FK triggers check plan_id (000001/000033) before cloud_account_id (000011/000053) by trigger OID order, so plan then account.
• Two deletes sharing plans lock plans ORDER BY p.id; ladder paths lock ladder_configs/runs/tranches only (FOR NO KEY UPDATE OF c), never purchase_plans.
• plan_accounts writers: only setPlanAccountsTx and the cloud_accounts/purchase_plans cascades. Only DELETE FROM cloud_accounts is in DeleteCloudAccount.
• Transaction scope: all three locks, registration reset, detach, disable and DELETE share one tx; errors return through defer rollbackTx. pgxmock covers both plan-lock failure points.

Local results (macOS, Docker testcontainers, merged tree)

• go build ./...; go vet ./internal/config/; go vet -tags integration ./internal/config/ ./internal/database/...: clean.
• go test ./internal/config/ -count=1: 869 passed.
• go test -tags integration ./internal/config/ -run 'TestDeleteCloudAccount|TestAccountDeleteWaitsForCommandAccountShare|TestCountLiveExecutionsForAccount|TestTerminalExecutionStatuses|SetPlanAccounts|PlanAccounts': all 22 pass, ok 26.3s.

Attacked:

• Mutation probes in a fresh scratch export, restored after each:
  M1 relock removed -> RelocksPlanThatGainedAccountDuringWait fails. Killed.
  M2 first plan lock removed -> ConcurrentSetPlanAccounts and TestAccountDeleteWaitsForCommandAccountShare fail. Killed.
  M3 FOR UPDATE -> FOR SHARE -> 4 tests fail. Killed.
  M4 FOR UPDATE -> FOR NO KEY UPDATE (informational) -> only the waiter-pattern test fails.
• EvalPlanQual: after waiting on step 1, a plan that lost A is still locked (harmless); a plan that gained A is caught by step 3, and no later gain is possible because a (P, A) insert needs KEY SHARE on A, which the account FOR UPDATE blocks.
• Checked issue fix(config): concurrent deletes of a plan's accounts still leave it enabled with zero accounts (#1065 follow-up) #1072 text: findings 1 and 2 are both addressed and covered by deterministic or iterated real-Postgres tests.
• Full -tags integration suite not run (orchestrator owns full verification).

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 97015fc

Findings:

• [non-blocking] internal/config/store_postgres.go:4010-4013 - The lockAccountPlansTx doc says "A fixed order keeps two account deletes that share plans from deadlocking". That overstates it, and the PR body's "Remaining deadlock window" names only the SetPlanAccounts variant.
  • The step-3 relock runs after the account lock, so two deletes can still deadlock.
  • I reproduced it deterministically on head in a scratch integration test:
    • Setup: plans lo<hi by id, hi={A,B}, lo={B}. A writer holds lo FOR SHARE and inserts (lo,A).
    • Delete(A) locks hi and waits on A. Delete(B) waits on lo.
    • When the writer commits, Delete(B) takes lo and waits on hi, while Delete(A)'s relock waits on lo.
    • errA is "failed to lock plans of cloud account: ... 40P01".
  • Integrity holds: A rolls back whole, and the handler returns a retryable 500.
  • Suggested fix: reword the comment and add this case to the documented window. A retry on 40P01 in DeleteCloudAccount is optional.

Attacked:

• Lock order against every writer of plans, plan_accounts and accounts. All of them take the plan before the account, the same order as the delete:
  • SetPlanAccounts takes the plan FOR SHARE first (managed_guard.go:75), then the accounts FOR SHARE.
  • The editor path takes the plan FOR UPDATE, then executions, then accounts.
  • CompletePlanStep takes the plan FOR UPDATE (store_postgres.go:692).
  • FK insert triggers: from pg_trigger, the plan_id FK check fires before the cloud_account_id check on purchase_executions, purchase_history and plan_accounts.
  • The deadlock with the execution-retry path already existed at the account lock on base. This PR does not add it.
• Lock scope: only plans with a plan_accounts row for the account are locked. Shared plans are locked but not written, and the disable SQL is unchanged.
• Waits: there is no NOWAIT and no lock_timeout outside the ladder code, so the writers' transactions bound the waits. Acceptable.
• Transaction: all three locks, the reset, the detach, the disable and the DELETE run in one transaction with a deferred rollback. pgxmock covers failures at the first and second plan lock.
• ORDER BY with FOR UPDATE: LockRows sits above Sort, so locks are taken in id order.
• Merge: git merge-tree --write-tree origin/main <head> is clean. No migration.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All four were caught:
  1. FOR UPDATE changed to FOR NO KEY UPDATE: RelocksPlanThatGainedAccountDuringWait and TestAccountDeleteWaitsForCommandAccountShare FAIL.
  2. Relock removed: RelocksPlan... FAILS.
  3. First plan lock removed: ConcurrentSetPlanAccounts and TestAccountDeleteWaitsForCommandAccountShare FAIL.
  4. Every plan locked instead of only the account's: DisablesPlanThatGainedAccountConcurrently FAILS.
• Blast radius: the only caller of DeleteCloudAccount is api/handler_accounts.go:618. It already checks scope and maps 23503. The new contention is that FK inserts on the account's plans block for the length of the delete, which is acceptable.

Verified locally, on a git archive export with no cloud credentials, heavy runs under the shared lock:

• go build ./..., go vet ./internal/config/ and go vet -tags integration ./internal/config/: ok. gofmt -l on the 4 changed files: clean.
• go test -count=1 ./internal/config/: ok.
• go test -tags integration -count=2 ./internal/config/ -run 'TestDeleteCloudAccount_|TestAccountDeleteWaitsForCommandAccountShare|TestPGXMock_DeleteCloudAccount': ok.
• Fail-before, with the new integration tests on base:
  • ConcurrentDeletesOfOnePlansAccounts, ConcurrentSetPlanAccounts (40P01) and RelocksPlanThatGainedAccountDuringWait FAIL.
  • DisablesManagedRampPlanKeepingSchedule passes. It is a guard test, as the body says.
• Not run: the full internal/config integration suite.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Local verification at 97015fcc11898e78caf66806c4204a6bd2d70e48 merged onto current main (7e5b58e6; merge-tree 12c77555; macOS):

• go build ./..., go vet ./... and go vet -tags integration ./... pass.
• go test -race -short -count=1 on the packages this PR changes passes: ./internal/config.
• The full go test -race -short ./... suite passes on the tree with every PR in this merge train applied (tree 99ee50e2).
• Nothing was applied to any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent, at IAmJSD's request, after 2 independent reviews and local verification (see the review and verification comments above, all at 97015fcc11898e78caf66806c4204a6bd2d70e48).