fix(plans): preserve managed next-root dates on completion

[cristim]

What

Persist and preserve the managed next-root date when a ramp step completes (dates_managed), so completing a step through CompletePlanStep or CompleteTargetMetStepTx no longer replaces an uneven managed schedule with the legacy formula. Also fixes the same-store sibling fixture (managedClockSibling started a second Postgres container, so the incoming-retry link lived in another database) and drops the unused total fixture parameter.

Why

M2 managed-ramp chain for #631: the managed schedule root dates must survive step completion before allocation, claim and target-met can rely on them.

Local verification

• go build ./... and go test -race ./internal/config/... ./internal/purchase/... on this tip (macOS).
• Fixture-backed: native PostgreSQL integration tests with synthetic data; no real provider calls. Not a real-account scenario.

Stack (M2 chain, merge in order)

1. this PR (fix(plans): preserve managed next-root dates on completion #906, base main)
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. target-met PR: fix(plans): lock the plan before completing a target-met step #909

Refs #631

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Ramp schedules can now use managed dates to determine when the next step is eligible to run.
  • When a managed schedule completes a step, its next date is retained only when a valid next step is available; otherwise, the managed schedule stops advancing.
• Bug Fixes
  • Concurrent changes to a plan are detected during ramp-position updates, helping prevent schedule progress from overwriting newer plan changes.
  • Schedules that do not use managed dates continue to follow their existing date behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 683d0045-27b6-4d3c-89d9-534d8f0ebadf

📥 Commits

Reviewing files that changed from the base of the PR and between e003025 and 36d385e.

📒 Files selected for processing: 4

• internal/config/store_postgres.go
• internal/config/store_postgres_plan_schedule_clock.go
• internal/config/store_postgres_plan_schedule_clock_integration_test.go
• internal/config/types.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

The plan schedule now has a DatesManaged flag. Managed schedules use a new PostgreSQL persistence path that selects the next execution date from an eligible root. Integration tests cover managed completion, blocked roots, and legacy date calculation.

Changes: Managed Ramp Schedule Dates

Layer / File(s)	Summary
Managed-date contract and routing internal/config/types.go, internal/config/store_postgres.go	RampSchedule adds the DatesManaged field. When the field is true, persistRampPosition delegates to managed persistence.
Managed-date persistence and root selection internal/config/store_postgres_plan_schedule_clock.go, internal/config/store_postgres_plan_schedule_clock_integration_test.go	Managed persistence selects a next date from an eligible next-step root and updates the plan only when updated_at matches. Integration tests cover eligible and blocked roots, conflicts, final-step completion, and legacy date calculation.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant persistRampPosition
  participant persistManagedRampPosition
  participant PostgreSQL
  persistRampPosition->>persistManagedRampPosition: Delegate when DatesManaged is true
  persistManagedRampPosition->>PostgreSQL: Read next-step executions
  PostgreSQL-->>persistManagedRampPosition: Return execution rows
  persistManagedRampPosition->>PostgreSQL: Update ramp step and execution dates when updated_at matches

Loading

Merge Risk: ⚪ Minimal · up to 36d38

The change keeps managed next-root dates when ramp steps complete, and no concrete merge-blocking risk was found.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: preserving managed next-root dates when plan steps complete.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

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

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

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]

CR round 1: zero actionable items. The docstring-coverage pre-merge warning is dismissed: this repo's comment policy is no comments unless they state a why the code cannot, so we do not add docstrings to hit a percentage. CI is green on 36d385e (the earlier Integration Tests failure was a nanosecond-vs-microsecond time comparison in the new test, fixed in 36d385e). Stacked PRs #907-#909 get no CI or CR run until their base is main, so they are re-gated after each parent merges.

[cristim]

Independent review of 36d385e: no actionable findings

Base used: origin/main (d6ffd5d, includes #892 and #899). This PR is the stack root, base is main. Diff is 4 files, +319 additions (under the 400 cap). git merge-tree --write-tree origin/main 36d385eb is clean, so the moved main causes no conflict.

Findings (both non-blocking)

1. Nit / follow-up, internal/config/store_postgres_plan_schedule_clock.go:67-77: the next-root read (ListExecutionsForPlanStepTx plus the incoming-retry EXISTS) is not locked against writers of purchase_executions. The plan row lock serializes completions with each other, but a reschedule, cancel or new retry link on the step N+1 root that commits between the read and the commit leaves a stale next_execution_date. This is acknowledged in the plan (plans/631-managed-scheduling-m2-20261015.md, generic-writer cuts) and is not reachable yet: dates_managed is only settable by SQL, because the API PlanRequest rebuilds RampSchedule from preset fields (internal/api/types.go:832-870). Please confirm #907-#909 or a later cut own it before the marker is exposed.
2. Nit, same file :30-32: a blocked or missing root writes next_execution_date = NULL with no log line or reason, so an operator sees a managed plan that silently stops. The tests assert this as intended. A debug or warn log naming the blocking cause would help.

Date handling (the requested attack)

• Timezone, month-end, leap year: the new code does no date arithmetic. It copies PurchaseExecution.ScheduledDate (a timestamptz column, migration 000001:79) into purchase_plans.next_execution_date (also timestamptz, 000001:67) as an instant. There is no AddDate, Truncate or local-zone conversion in the diff, so month-end and leap-year drift cannot arise here. The only formula use is the unchanged unmanaged branch (GetNextPurchaseDate).
• Completion racing the scheduler: CompletePlanStep and CompleteTargetMetStepTx both take LockPurchasePlanTx (FOR UPDATE) before advanceRampStep and persistRampPosition; the managed write is a compare-and-set on updated_at, returning ErrPurchasePlanConflict on mismatch. Two concurrent completions cannot both advance (advanceRampStep rejects the second via ErrRampStepCountedBySibling). Two non-canceled canonical roots produce ErrPurchasePlanConflict and roll back the progress (test "duplicate" asserts step stays 1). The residual unlocked-read window is finding 1.
• Preserved date in the past: possible (a pending or scheduled root whose date has passed is copied as is). It cannot trigger a purchase from the plan date. Purchase eligibility is per execution row (internal/purchase/manager.go:900 skips rows not yet due; a due row is the existing root, independent of this PR). The plan-date consumers on main are guarded: plan_step_attach.go:93 refuses an existing row once the plan date has passed, :104 leaves an empty root due inside the veto window to the scheduler, and attachScheduledDate (:352-357) never schedules earlier than now + NotificationDaysBefore. shouldRetryOverduePlan (notifications.go:97) treats a past date as overdue only until the grace window ends, the same as the legacy formula date. A NULL date makes planUnchanged fail (plan_step_attach.go:133), so a stopped clock skips rather than buys.

What I attacked

• Ran go test -tags integration -run TestManagedCompletion ./internal/config/ at the head SHA with testcontainers Postgres 16: all 3 test functions and all subtests pass; go build ./... and go vet -tags integration ./internal/config/ clean.
• Mutation 1 (remove the 3-line DatesManaged delegate in persistRampPosition, equivalent to parent behavior): TestManagedCompletionKeepsNextRootAndRawPlan fails for both "bought" and "target met" ("Should be true" on the date equality). This is the fail-before proof; the tests set the marker via raw SQL, so they compile and run on the parent too.
• Mutation 2 (drop the incoming retry_execution_id check): StopsAtBlockedNextRoot/incoming_other_step and /incoming_other_plan fail.
• Read managedPlanNextRoot against the table of cases in the test (missing, canceled, failed, paused, legacy, duplicate, zero date, delayed-fire, executed/completed/skip/purchased provenance, canceled predecessors, live child, replacement): each maps to a branch and is exercised. Finished ramp returns nil before the marker check (planScheduleNextDateTx:37-39). jsonb_set(... '{current_step}') leaves unknown nested ramp/service metadata and the baseline untouched (asserted byte-equal).
• Not run: mutation probes beyond these two (optional per owner rule); the unmanaged path is covered by the existing suite plus the legacy assertion in TestManagedCompletionFinishedAndLegacyClock.

Gate status (origin/main Review gate)

• Independent review: this comment, at 36d385e.
• Findings resolved: none actionable; two nits above.
• CI on SHA: gh pr checks 906 shows 25/25 SUCCESS; runs for this commit: "CI - Build & Test" and "pre-commit" both success.
• CodeRabbit: commit status "Review completed" success on 36d385e; 0 inline comments and 0 review threads, so none unresolved. The author's dismissal of the docstring-coverage warning is on the PR; I agree with it given the repo comment policy.
• Local real-path evidence on the PR: the body states go build and go test -race ./internal/config/... ./internal/purchase/... on macOS with native PostgreSQL fixtures, labeled as not a real-account scenario. I reproduced the config integration tests above. I did not re-run ./internal/purchase/....
• Labels: match issue #631 (priority/p1, severity/high, urgency/this-sprint, impact/many, effort/xl, type/feat, triaged). The PR closes no issue (Refs #631), so the mirror is against #631.
• Verdict recorded: this comment. Any new commit or rebase restarts the gate; an empty git range-diff after a pure rebase keeps this verdict.

[cristim]

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

• Independent review of 36d385e (non-author, cold brief): no actionable findings. Two non-blocking notes:
  • The next-root read is not locked against concurrent purchase_executions writers. This is unreachable while dates_managed can only be set from SQL; the later M2 cuts own it before the marker is exposed.
  • A blocked or missing root writes NULL next_execution_date without logging the cause.
• CI on 36d385e: 25/25 checks pass, including CI Success and Run pre-commit hooks.
• CodeRabbit: review completed on 36d385e, 0 review threads.
• Local verification (macOS):
  • The TestManagedCompletion* tests pass against testcontainers Postgres 16.
  • Fail-before proof: removing the managed delegate in persistRampPosition (the parent behaviour) fails the "bought" and "target met" subtests.
  • Removing the incoming retry_execution_id check fails two subtests.
  • Evidence is fixture-backed only. There was no real-account run and no purchase.
• Labels mirrored from feat(plans): plan steps resolve real recommendations and show real counts and prices (tier B of #609) #631. Merges cleanly onto main d6ffd5d.

Merging at 36d385e with --match-head-commit. #907 will be retargeted to main and rebased next.