fix(plans): lock the plan before completing a target-met step

[cristim]

What

CompleteTargetMetStepTx now locks the plan before writing the root execution, refuses a managed call whose root is missing or not the exact resolved root, and filters the final CAS by plan_id and step_number in both modes. Includes a fixture call fix (drops the total arg from the target-met clock fixture).

Why

M2 managed-ramp chain for #631: the old order wrote the root before locking, and its CAS matched only execution id, status and empty recommendations, so a direct call could complete another plan's root or materialize an absent managed root.

Local verification

• go build ./... and go test -race ./internal/config/... ./internal/purchase/... on this tip (macOS).
• Fixture-backed: native PostgreSQL negative table through the real method (missing root, foreign-plan root, wrong step, duplicate roots, ...). Not a real-account scenario.

Stack (M2 chain, merge in order)

1. scheduling PR: fix(plans): preserve managed next-root dates on completion #906
2. allocation PR: fix(plans): allocate marked ramps by exact step count #907
3. claim PR: fix(purchase): claim the exact managed schedule root #908
4. this PR (fix(plans): lock the plan before completing a target-met step #909, base claim branch)

Refs #631

🤖 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.}

• 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 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. Your 64 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: 57e4dc5f-8d0a-49c0-8f29-ccb75ec69783

📥 Commits

Reviewing files that changed from the base of the PR and between 4f97e95 and c37a90b.

📒 Files selected for processing: 3

• internal/config/store_postgres_managed_target_met_integration_test.go
• internal/config/store_postgres_plan_schedule_clock.go
• internal/config/store_postgres_ramp_target_met.go

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

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

[cristim]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Independent review of ac521dd: no actionable findings

Diff reviewed: git diff ae15eebabc1cc2c7e7b34cc78248293b97370eca...ac521ddb (3 files, +128/-9). The base tip I used is the current head of #908, so the stack is consistent.

Findings

1. Nit, non-blocking: no test exercises two real transactions racing. The test file proves refusal before write inside one tx. I wrote a scratch concurrency test (not committed): a purchase-side tx holds the root row via SaveExecutionIfStatusTx and moves it to running uncommitted; target-met in a second tx blocks on the row lock (store_postgres_plan_schedule_clock.go:59, FOR UPDATE), then on commit refuses with ErrExecutionNotInExpectedStatus, leaves the root running and the ramp at step 1. No hang. Passes on real Postgres.
2. Nit: the foreign managed subcase passes even with the managed guard deleted, because the new plan_id CAS also catches it. Redundant defense, not a defect.

Lock ordering and scope

• Only production caller is plan_step_attach.go:246 (targetMet), which runs inside a claim that already holds the plan lock. The new LockPurchasePlanTx there is a same-transaction re-lock (no-op), and it still re-reads after the caller's baseline write, so the optimistic-lock check in UpdatePurchasePlanTx is unaffected.
• Order is now plan, then execution rows (ResolveManagedPlanRootTx FOR UPDATE ordered by execution_id), then the CAS. The old method wrote the execution before locking the plan, i.e. execution then plan, the reverse of the claim path and of CompletePlanStep / UpdatePlanTx (plan only). The PR removes that inversion. Other execution-first lockers (lockExecutionRow, store_postgres_execution_guarded.go:28) take no plan lock in the same tx, so I found no cycle.
• Lock duration is the caller's transaction, unchanged. Added work is one extra SELECT ... FOR UPDATE on the step's rows, scoped to one plan and one step.
• Can target-met complete a step a purchase is executing? Not in managed mode: the root must be the exact resolved root, and the CAS needs pending/notified with empty recommendations; a root with recs or any other status is refused (test cases occupied, scheduled, plus the concurrency probe above). Legacy rootID=="" still inserts a new row without looking at existing step rows; that is pre-existing and out of scope.
• Behavior difference from the plan file: the guard does not compare stepNumber == CurrentStep+1 explicitly; the step_number = $7 CAS gives the same refusal (cases wrong step, other step root).

What I attacked

• Ran go test -tags integration -run 'TestManagedTargetMetRefusesBeforeWrite|TestManagedCompletionKeepsNextRootAndRawPlan' ./internal/config/ at this SHA (Docker Postgres): pass. go build ./..., go vet -tags integration, go test ./internal/config ./internal/purchase: pass.
• Fail-before: restored the two production files from #908 (ae15eeb), kept the new test: 9 of 11 subcases fail (missing, foreign, finished, wrong step, duplicate, legacy foreign, legacy other step, beside root, other step root); occupied and scheduled pass on the parent because the old CAS already refused them. Pass-after: all 11 pass.
• Mutation probes (reverted): managed guard forced off fails missing, duplicate, beside root; removing plan_id/step_number from the CAS fails legacy foreign and legacy other step.
• Concurrency probe described in finding 1.
• golangci-lint crashed on this machine (tool panic), so lint is not verified locally.

Gate status

• Independent review: this comment, names the SHA.
• Findings resolved: none actionable.
• CI on SHA: not applicable until retarget (CodeRabbit reports "reviews disabled for this base branch").
• Local real-path verification: native Postgres run above, fixture-backed, not a real-account scenario. The PR body records its own run; this verdict and my evidence are recorded here.
• Labels: priority/p1, severity/high, urgency/this-sprint, impact/many, effort/xl, type/feat, triaged present; the PR has no closing issue (Refs #631), so no mirror check is possible beyond that.

[cristim]

Rebased onto the rebased #908 (06a3368). Pure rebase, git range-diff ae15eeba..ac521ddb 06a3368e..90ae1383:

1:  c634d607 = 1:  addde59b fix(plans): lock the plan before completing a target-met step
2:  ac521ddb = 2:  90ae1383 test(plans): drop the total arg from the target-met clock fixture call

Empty range-diff, so the verdict on ac521dd carries to 90ae138. Retarget to main after #908 merges.

[cristim]

Rebased onto the rebased #908 (67eed57). Pure rebase, git range-diff 06a3368e..90ae1383 67eed572..8e0707b8:

1:  addde59b = 1:  cdb20cc0 fix(plans): lock the plan before completing a target-met step
2:  90ae1383 = 2:  8e0707b8 test(plans): drop the total arg from the target-met clock fixture call

Empty range-diff, so the verdict on ac521dd carries to 8e0707b. Retarget to main after #908 merges.

[cristim]

Rebased onto main after #908 merged (squash 4f97e95); retargeted to main. Pure rebase, git range-diff 67eed572..8e0707b8 origin/main..c37a90b8:

1:  cdb20cc0 = 1:  8c31c497 fix(plans): lock the plan before completing a target-met step
2:  8e0707b8 = 2:  c37a90b8 test(plans): drop the total arg from the target-met clock fixture call

Empty range-diff, so the independent verdict on ac521dd carries to c37a90b. Gate needs CI green on c37a90b.

[cristim]

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

• Independent review of ac521dd: no actionable findings.
  • Lock order is plan, then execution rows by id, then CAS. That removes the inversion fix(purchase): claim the exact managed schedule root #908 noted, and no cycle exists.
  • Target-met cannot complete a step that a purchase is executing in managed mode.
• c37a90b is a pure rebase of ac521dd (via 90ae138 and 8e0707b) onto main after fix(plans): preserve managed next-root dates on completion #906, fix(plans): allocate marked ramps by exact step count #907 and fix(purchase): claim the exact managed schedule root #908. Every git range-diff is empty (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 c37a90b: all checks pass, including CI Success and Run pre-commit hooks.
• Local verification (macOS, Docker Postgres):
  • The integration tests pass.
  • Scratch race test: a purchase tx holding the root makes target-met block on FOR UPDATE, then refuse with ErrExecutionNotInExpectedStatus. Nothing hangs.
  • Fail-before: 9 of 11 subcases fail with fix(purchase): claim the exact managed schedule root #908's production files restored.
  • Two mutation probes are killed.
  • No real-account run or purchase.
• 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.

Merging at c37a90b with --match-head-commit. This completes the #631 M2 stack (#906-#909).