test(plans): prove legacy writers recheck plans an editor Save changed

[cristim]

Add race tests for the legacy writers that can run while an editor Save
holds the plan: the notifier's new schedule root, a retry successor, a
fan-out child, execution pruning and cloud account deletion. Each test
queues the writer behind a stand-in for the Save's lock footprint, then
checks the outcome in both orders: a writer that waited sees the
committed editor state and skips or refuses, and a writer that won is
seen by the next capture. The shared footprint and lock-wait helpers
move to testhelpers so the Save command's own race tests can swap in
the real command. Two pre-existing gaps are recorded in known_issues
(#998, #999).

Refs #631

Part 5 of 5 of the #631 legacy-writer guards; stacked on #1013; merge bottom first.

Review verdict and evidence

Original review, copied from the per-commit evidence record. Commit fd41ebf3 was reviewed before the rebase onto origin/main; the rebased commit is bcf1d87.

• Reviewed tree 743f15d7bcb98d412261ef07eaa98933e9c78edd (commit fd41ebf3). +354/-15. Proof only (no production code).
• Pass 1: no blockers (two mediums on documentation of the harness seam; fixed); pass 2: clean.
• Evidence: integration -count=5 of the 7 new tests + 6 G2 race tests: all pass, 0 skip; lint 0; pre-commit pass.
• Probes killed: drop retry_execution_id IS NULL in the link CAS; drop status exclusion in CleanupOldExecutions; footprint without account FOR SHARE; managed refusal in SavePurchaseExecutionTx; LockPurchasePlanTx without FOR UPDATE (killed only by the lock-wait timeout); notifier judges the listed copy.
• Stated limits: removing the DatesManaged compare at plan_step_attach.go:69-71 is an EQUIVALENT mutant (every editor write bumps updated_at via the trigger, so managedPlanUnchanged decides first). The footprint stand-in omits the identity/provider advisory locks; the swap contract for the command's own race tests is in testhelpers/lockwait.go.
• Follow-ups filed: fix(cleanup): give the cleanup lambda the live-status guard CleanupOldExecutions has #998 (cleanup lambda status guard), feat(plans): settle a failed-with-submission row so a managed plan can be deleted #999.

Rebase: the five commits were rebased onto origin/main 0ed85aa (27 commits ahead of the old base). git range-diff of the old 5 commits against the rebased 5 shows all five as = (identical patches), so the verdict carries over per the owner's rule. New head SHA of this PR: bcf1d87. No migration is added by any of the five commits.

Review gate

CodeRabbit is not used on this stack (quota; owner decision): the gate is the independent adversarial review above plus the local verification below.

Local verification

Local verification of the rebased stack tip (macOS, Docker Postgres via testcontainers; Linux via CI):

• go build ./... and go vet ./...: pass
• CI-pinned golangci-lint v2.10.1: 0 issues
• go test -race -count=1 ./internal/config/ ./internal/api/ ./internal/purchase/...: config ok, purchase ok, api has one failure, TestAuthPublicRoutes_EveryRouteIsCSRFGuardedOrExempt (route POST /api/purchases/approve/*/details unclassified). It fails identically on a clean origin/main 0ed85aa and is not touched by this stack.
• Integration (-tags integration -p 1 -count=1) on the same packages: 5838 pass, 1 fail (the same test), 14 skip (the known pre-existing skips: 11 TestPostgresStoreDB_* in config; TestGetFederationIaC_TfvarsAutoLoadedByTerraform/aws-cross-account, TestRevokePurchase_FourEyesApproval, TestRevoke_4EyesMode_TODO in api).

Status on main

The change is dormant on main until the editor Save route ships: only plans marked managed (ramp_schedule.dates_managed) are affected, and only that route sets the marker.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 6 billable files and costs up to $1.50.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 29 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 68 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 8c0b4a1d-3bf4-44ca-9410-2afe0b604e59

📥 Commits

Reviewing files that changed from the base of the PR and between d7eb3b0 and 3e1c4eb.

📒 Files selected for processing: 6

• internal/api/handler_plans_managed_add_accounts_integration_test.go
• internal/api/handler_retry_managed_race_integration_test.go
• internal/config/store_postgres_writer_race_integration_test.go
• internal/database/postgres/testhelpers/lockwait.go
• internal/purchase/plan_step_managed_race_integration_test.go
• known_issues/35_legacy_guard_followups.md

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

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

[cristim]

Rebased onto main 0a5f790 (picks up #1003, which fixed the CSRF classification test that failed on the old base); git range-diff vs the previous head is empty, so the review verdict in the description carries. New head SHA: bcf1d87.

[cristim]

Independent review of 418d4a9: no actionable findings

This review covers the pre-rebase SHA 418d4a9. The merge driver will record the range-diff to the new head (bcf1d87). I ran git range-diff on the PR's own commit: it shows = (identical patch).

Findings: none blocking. NON-BLOCKING: awaitLockWait in the add-accounts tests matches any lock wait (pattern ""), whereas the new tests use statement-specific patterns. It is weaker but sound with one writer.

What I attacked:

• Interleaving is real: each test holds the footprint transaction, starts the writer in a goroutine, and blocks in WaitForLockWait on pg_stat_activity (wait_event_type = 'Lock', statement LIKE) before the command writes and commits. The writer therefore read its stale state before the commit. Await fails fast at 30s.
• Mutation probe 1: notifier judges the listed plan copy (locked = plan in claimPlanStep): TestClaimSkipsPlanTheCommandConvertedWhileItWaited FAILS ("Should be false"). Reverted.
• Mutation probe 2: dropped AND retry_execution_id IS NULL from the retry link CAS: TestRetryLosesToCommandThatLinkedOriginalFirst FAILS (409 not returned). Reverted. Worktree is clean.
• Ran integration tests (testcontainers) on api, config and purchase with -run Race|Waits|Queues|Recheck|Claim|Retry: 290 passed, 0 failed.
• known_issues/35_legacy_guard_followups.md follows CONTRIBUTING.md "Known Issues Sweep": # Known Issues: ... heading, audit-status line (2 needs triage, 0 resolved, matches the two entries), ## SEVERITY: sections with Files, Description, Why deferred, Status, and the issue refs #998 and #999.
• Production code: none changed; the only non-test file is the testhelpers helper, which documents the swap contract.

Gate: independent review of this SHA (this comment). CI not applicable until retarget. CodeRabbit not used (owner decision). Local verification as above. Labels mirror the stack. 354 additions-only diff under the cap.

[cristim]

The independent review above covers 418d4a9. claude-cc-631-driver rebased the stack onto main 0a5f790 (includes #1003).
Pure rebase: git range-diff 62f0cce0..418d4a93 aeb87008..bcf1d876:

1:  418d4a93 = 1:  bcf1d876 test(plans): prove legacy writers recheck plans an editor Save changed

The PR's own commits are unchanged, so the verdict carries to bcf1d87 (owner #631 rule).

[cristim]

The independent review above covers bcf1d87. Rebased onto main after #1010 merged (0117179); #1011 retargeted to main.
Pure rebase: git range-diff aeb87008..bcf1d876 7e7d99bc..16dba677:

1:  bcf1d876 = 1:  16dba677 test(plans): prove legacy writers recheck plans an editor Save changed

The PR's own commits are unchanged, so the verdict carries to 16dba67 (owner #631 rule).

[cristim]

The independent review above covers 16dba67. Rebased onto main after #1011 merged (173474e); #1012 retargeted to main.
Pure rebase: git range-diff 7e7d99bc..16dba677 832a2e51..ea61b6a1:

1:  16dba677 = 1:  ea61b6a1 test(plans): prove legacy writers recheck plans an editor Save changed

The PR's own commits are unchanged, so the verdict carries to ea61b6a (owner #631 rule).

[cristim]

The independent review above covers ea61b6a. Rebased onto main after #1012 merged; #1013 retargeted to main.
Pure rebase: git range-diff 832a2e51..ea61b6a1 8aa0ce7d..dc8a4e83:

1:  ea61b6a1 = 1:  dc8a4e83 test(plans): prove legacy writers recheck plans an editor Save changed

The PR's own commits are unchanged, so the verdict carries to dc8a4e8 (owner #631 rule).

[cristim]

The independent review above covers dc8a4e8. Rebased onto main after #1013 merged (d7eb3b0); #1014 retargeted to main.
Pure rebase: git range-diff 8aa0ce7d..dc8a4e83 d7eb3b0d..3e1c4eb0:

1:  dc8a4e83 = 1:  3e1c4eb0 test(plans): prove legacy writers recheck plans an editor Save changed

The PR's own commits are unchanged, so the verdict carries to 3e1c4eb (owner #631 rule).

[cristim]

Gate record for 3e1c4eb (origin/main CLAUDE.md "Review gate"):

• Independent review of 418d4a9 (non-author): no actionable findings.
  • The race tests really interleave. Each waits on a pg_stat_activity lock wait before the editor commits.
  • known_issues/35 follows the CONTRIBUTING.md "Known Issues Sweep" format.
• 3e1c4eb is a pure rebase of 418d4a9 (via bcf1d87, 16dba67, ea61b6a and dc8a4e8) onto main after fix(config): refuse legacy writes to editor-managed plans #1010-fix(plans): guard delete, reschedule and resume of managed plans #1013. All range-diffs are = (recorded above), so the verdict carries under the owner feat(plans): plan steps resolve real recommendations and show real counts and prices (tier B of #609) #631 rule.
• CI on 3e1c4eb: CI Success and Run pre-commit hooks pass.
• Local verification (macOS, testcontainers):
  • 290 race and integration tests pass.
  • Mutation probes: judging the listed plan copy in plan_step_attach.go fails the matching race test, and dropping retry_execution_id IS NULL from the retry CAS fails its race test.
  • Test-only change; no real-account run.
• CR skipped: quota-limited; covered by the independent review plus local verification.
• Labels mirrored from feat(plans): plan steps resolve real recommendations and show real counts and prices (tier B of #609) #631 (Refs feat(plans): plan steps resolve real recommendations and show real counts and prices (tier B of #609) #631).

Merging at 3e1c4eb with --match-head-commit. This completes the #631 legacy-writer guard stack (#1010-#1014).