fix(config): cancel a deleted plan's open step rows in the same transaction

[IAmJSD]

Root cause

purchase_executions.plan_id is ON DELETE SET NULL (migration 000033). Token approval applies the plan's max_step_commitment (requirePlanStepCap) and the approve-own plan-step rule (enforceHeldApproveConstraints) only while execution.PlanID != "". DeletePurchasePlan deleted the plan row and left its open step rows (pending / notified / paused) in place. Postgres nulled their plan_id, so they stayed approvable as plan-less rows. A token approver with no approve verb (for example the account's contact_email) could then approve them with no cap check, and an approve-own holder's Constraints stopped applying.

Fix

DeletePurchasePlan (internal/config/store_postgres.go) now calls a new cancelOpenPlanExecutionsTx inside its existing transaction, after the managed-plan guard and before the DELETE FROM purchase_plans. One statement flips the plan's pending, notified and paused rows to canceled and deletes their purchase_suppressions, the same way the other cancel paths release suppressions. The cancel and the SET NULL commit together, so no committed state has an approvable row that has lost its plan. An approval racing the delete either reads the plan before it goes (the cap check runs) or loses the status CAS on a canceled row.

This follows the first option in the issue ("cancel or expire pending plan-step rows when their plan is deleted"). I didn't use the second option (mark the row with a column that survives deletion): step_number defaults to 1 for ad-hoc rows too, so no existing column can tell a plan-step row from an ad-hoc one. That option would need a new column and a migration. This fix needs no migration.

Left unchanged on purpose: scheduled rows (already approved under the cap during the delayed-approval window), approved / running rows that are in flight, and settled rows (completed, failed, etc.). Managed plans are unaffected because requireManagedDeleteTx already refuses to delete them while open rows exist.

Verification

All evidence is local unit, pgxmock or testcontainers Postgres. No real cloud account, purchase, email or deploy was involved. Commands ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

Regression test (real Postgres via testcontainers): internal/config/store_postgres_plan_delete_integration_test.go, TestDeletePurchasePlan_CancelsOpenStepRows. It builds a plan with pending, notified and paused rows (the open rows) and scheduled, approved, completed and failed rows (the others), a suppression on the pending row and on the scheduled row, and a pending row on a second plan. Then it deletes the first plan. It asserts:

• the open rows are canceled and detached;
• the approval/run-now status CAS on those rows now fails with ErrExecutionNotInExpectedStatus;
• the other rows and the second plan's row keep their status;
• the pending row's suppression is gone and the scheduled row's suppression is kept.

go test -tags=integration -count=1 -run TestDeletePurchasePlan_CancelsOpenStepRows ./internal/config/

• Before the fix (fix stashed): FAIL. expected: "canceled" actual: "pending" ... pending row of the deleted plan stays approvable
• After the fix: PASS

Mutation probes (each one applied alone, then the test rerun; all were caught):

• drop the plan_id scope (plan_id = $1 OR TRUE): FAIL, "another plan's row is untouched"
• leave paused out of the status list: FAIL, "paused row of the deleted plan stays approvable"
• add scheduled to the status list: FAIL, "scheduled row is not an open step row and keeps its status"
• keep the suppressions (WHERE FALSE AND ...): FAIL, "the canceled row's suppression is released"

Unit (pgxmock): TestPGXMock_DeletePurchasePlan_Success now pins the cancel statement (plan-scoped, the three statuses, the suppression delete, args plan-id, canceled), which runs before the plan delete. The new TestPGXMock_DeletePurchasePlan_CancelFailureKeepsPlan checks that a failed cancel rolls back and never issues the plan delete. Both fail on the code before this PR and pass with it.

Other checks:

• go build ./...: OK
• go vet ./... and go vet -tags=integration ./internal/config/: OK
• go test -count=1 ./internal/config/: ok
• go test -count=1 ./internal/api/: ok
• go test -count=1 ./internal/mocks/: ok
• go test -tags=integration -count=1 -timeout=20m ./internal/config/ (the whole integration suite, including the existing DeletePurchasePlan callers in store_postgres_test.go, store_postgres_db_test.go and store_postgres_retention_integration_test.go): ok (285s)
• golangci-lint v2.10.1 on ./internal/config/..., with and without --build-tags=integration: 0 issues
• gocyclo -over 10 internal/config/store_postgres.go: silent
• scripts/gosec-hook.sh on the changed files: exit 0
• git-secrets: scripts/test-git-secrets-allowlist.sh gave 42 PASS, 0 FAIL. git secrets --scan --cached over every tracked file plus the new test file, in a fresh git init repo with an isolated HOME, exited 0.

Coverage gaps: this was not run against a deployed environment, and the email-link approval was not driven end to end against a real database. The handler-side behavior follows from the store change, because approval's status CAS only accepts pending / notified, and run-now's only accepts pending / notified / paused.

Out of scope (not fixed here)

• Rows orphaned before this change. Any open row whose plan was already deleted on an existing deployment still has plan_id IS NULL and can still be approved through the same uncapped path. A backfill would need to tell those rows apart from genuine ad-hoc rows, and no current column does that reliably (step_number defaults to 1, and created_by_user_id is not guaranteed on every ad-hoc writer). If a maintainer wants one, it belongs in a follow-up with a migration (next free number after the open fix(config): let an account with only terminal executions be deleted #1020/000127 and fix(scheduler): record partial multi-subscription sweeps in collection state #1022/000128 would be 000129).
• scheduled rows of a deleted plan still fire at their scheduled time. They were approved while the plan existed, so the cap was already applied, but whether deleting a plan should also revoke them is a product question.

Labels

A maintainer, please mirror #862's triage labels onto this PR: priority/p2, severity/medium, triaged, urgency/this-quarter, impact/few, effort/s, type/bug.

Closes #862

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

Or wait 54 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: 198c473e-e9f3-4b6a-be7c-1bf48b28feb1

📥 Commits

Reviewing files that changed from the base of the PR and between af374cc and 1c2a980.

📒 Files selected for processing: 3

• internal/config/store_postgres.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/store_postgres_plan_delete_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 1c2a980: draft: blocks ready

No code defects found. The remaining blockers are gate items only (CI not finished, one test I could not run here).

Findings

• None BLOCKING in code.
• NON-BLOCKING (already acknowledged in the PR body): open rows orphaned before this change still have plan_id NULL and stay approvable on the uncapped path; scheduled rows of a deleted plan still fire. Both need a product call or a migration, so a follow-up issue is the right place. Worth filing before merge so it is tracked.

What I attacked

• Atomicity: DeletePurchasePlan (internal/config/store_postgres.go:869-891) runs lock, managed guard, cancel, delete inside one WithTx. A cancel error returns before the DELETE and rolls back; the new pgxmock test CancelFailureKeepsPlan covers it. The cancel and the FK SET NULL land in the same commit, so no committed state has an open plan-less row from a deletion.
• Race: the plan row is locked FOR UPDATE (LockPurchasePlanTx). An approval that wins the status CAS first leaves an approved row that already passed the cap check; one that loses sees canceled. The same plan lock covers the createPlan rollback caller (internal/api/handler_plans.go:202).
• "Open" statuses: I listed every status allowed by purchase_executions_status_check on a fully migrated scratch Postgres 17. pending, notified, paused are the pre-approval states. scheduled, approved, running and settled states are correctly left alone. revocation_requested is only reachable from completed (handler_purchases.go:1897), so it is not approvable. canceled is written with the canonical spelling and the CHECK accepts it.
• Scoping: the UPDATE is keyed on plan_id = $1, a global UUID primary key. Tenant access is enforced before the call (requirePlanMutationAccess, handler_plans.go:430). No cross-plan or cross-account rows can be touched. Suppression delete is limited to the canceled execution ids.
• SQL run for real: I applied all up migrations to a scratch Postgres 17 and ran the exact CTE plus the plan delete in a rolled-back transaction with 3 open rows, a scheduled row, a revocation_requested row, a second plan's pending row and two suppressions. Result: the 3 open rows became canceled with plan_id NULL, the others kept their status, the second plan's row was untouched, only the scheduled row's suppression remained.
• Other stores: the only config.StoreInterface implementations are PostgresStore and mocks.MockConfigStore (a recorder with no behavior to diverge). No other store needs the change.
• Ramp ledger trigger (migration 000124): ramp_ledger_holds treats canceled as not holding, which is consistent with a plan being deleted.
• Mutation probe (pgxmock): removed paused from the status list; TestPGXMock_DeletePurchasePlan_Success failed on the SQL regexp. Reverted.
• Tests at this SHA: go test ./internal/config/ ./internal/mocks/ 864 passed; go vet -tags=integration ./internal/config/ clean.

Gate status (origin/main Review gate)

• Independent review of the full diff: this comment, at 1c2a980.
• Findings resolved: none open. CodeRabbit skipped (draft); no unresolved threads.
• CI on the SHA: not green yet. pre-commit succeeded; CI - Build & Test was queued at the time of review (fork run, may need approval). This is a gate gap.
• Local verification: the testcontainers integration test TestDeletePurchasePlan_CancelsOpenStepRows could not run in my environment (no Docker daemon, it skips). I replaced it with the manual Postgres 17 SQL run above, which exercises the same statement and data shape. I did not independently re-run the author's fail-before/pass-after on the integration test; CI's Integration Tests job on this SHA should confirm it.
• Labels: mirror #862 (priority/p2, severity/medium, triaged, urgency/this-quarter, impact/few, effort/s, type/bug). Present on the PR.
• Size: 27 additions in production code, within the cap; 125 additions total.

Blocks ready

1. CI CI - Build & Test (including Integration Tests) green on 1c2a980.
2. Mark the PR ready for review.
3. Optional: file the follow-up issue for pre-existing orphaned rows and scheduled rows.

[IAmJSD]

Filed #1048 for the two leftover cases (rows orphaned before this change, and scheduled steps of a deleted plan), as suggested.

[cristim]

Gate record for 1c2a980 (origin/main CLAUDE.md "Review gate"):

• Independent review (fresh cold-brief agent, non-author) of 1c2a980: no code defects (fix(config): cancel a deleted plan's open step rows in the same transaction #1046 (review)). Atomicity, open-status set and plan scoping checked; a mutation probe dropping paused was caught by the pgxmock test.
• CI on 1c2a980: latest CI - Build & Test and pre-commit runs completed/success; CI Success and Run pre-commit hooks pass. The CI Integration Tests job ran the testcontainers regression TestDeletePurchasePlan_CancelsOpenStepRows against real Postgres and it passed. No main change since CI ran overlaps this PR's directories (checked by the merge driver).
• Local verification: the reviewer ran all migrations on a scratch Postgres 17 and executed the cancel statement plus the plan delete in a rolled-back transaction (open rows canceled, other statuses and another plan's row untouched, only the canceled rows' suppressions removed). Fail-before/pass-after is in the PR body. No deployed environment or real account.
• Out of scope, recorded in the PR body: rows orphaned before this change, and whether scheduled rows of a deleted plan should be revoked (product question).
• CR skipped: quota-limited; covered by the independent review plus local verification.
• Labels mirrored from fix(api): deleting a plan leaves its pending step rows uncapped on token approval #862.

Marking ready (owner-authorized) and merging at 1c2a980 with --match-head-commit.