Repository navigation
fix(ladder): net in-flight tranches and use append-only persistence to converge to target coverage - #1397
Conversation
Fixes the G2+G4 convergence gap: without netting, each daily run
ignores its own not-yet-fired scheduled tranches, re-measures the
full gap, piles up tranches, and overshoots the low-water floor.
- AllocationInput.InFlightUSDPerHour (*float64, required non-nil):
gap is now min(target-E, lowWater-E) - inFlight so repeated runs
on the same config converge to ~zero rather than accumulate.
- StoreInterface gains GetInFlightLadderCommitUSDHr (SUM of
amount_usd_hr for status IN ('scheduled','fired') per config)
and SaveLadderRunWithTranchesAndSupersede (atomic cancel-and-
replace: cancels prior scheduled tranches then inserts run+tranches
in one tx, at most one generation per config).
- executeLadderRun fetches in-flight before Allocate; persistLadderRun
calls SaveLadderRunWithTranchesAndSupersede.
- All mocks and test helpers updated (analytics, server, shared mock).
- Convergence regression: 3 sequential same-usage runs -> only run 1
allocates, runs 2-3 hold; canary confirms zero-in-flight diverges.
- Integration tests: sum+cancel round-trip, per-config isolation,
atomicity rollback on duplicate tranche ID.
GetInFlightLadderCommitUSDHr summed status IN ('scheduled','fired'),
but fired/completed tranches are executed purchases already folded into
the engine's ExistingUSDPerHour by the provider adapters. Counting them
again double-subtracted from the gap and under-purchased.
- Sum status = 'scheduled' ONLY; fired/completed belong solely to E.
- Use typed pkg/ladder TrancheStatus constants in the sum and the
supersede UPDATE instead of raw string literals (finding 5); the
status set now lives in one place.
- Update the InFlightUSDPerHour engine doc and the StoreInterface docs
to say scheduled-tranches-only (finding 3, engine layer).
- Update the integration test to assert scheduled-only (3.0, was 5.0).
…(L5 finding 2)
Netting scheduled tranches into the gap AND cancelling them every run
were mutually destructive: run2 nets to a Hold, but unconditional
SaveLadderRunWithTranchesAndSupersede cancelled the scheduled ramp
anyway, so run3 re-planned from zero. Oscillation; tranches with fire
dates beyond one cadence window never fired.
Corrected design:
- persistPlannedLadderRun chooses the persist path by whether THIS run
produced >=1 new scheduled tranche (len(trancheRows) > 0):
* >0 -> SaveLadderRunWithTranchesAndSupersede (cancel-and-replace
the config's prior scheduled generation).
* ==0 -> plain SaveLadderRunWithTranches: persist the run row and
PRESERVE the existing scheduled ramp with its original fire
dates (the live ramp the netting just relied on).
- Net effect: run1 builds the ramp; steady-state runs Hold and preserve
it; a drift-UP run generates a new generation and cancel-and-replaces.
- Drift-DOWN over-commit (a Hold preserving a now-too-large ramp) is
accepted for now, bounded by the low-water clamp; documented for a
follow-up shrink pass.
- Fix the executeLadderRun in-flight comment (finding 3, handler layer):
scheduled-tranches-only, no "in-progress purchase executions".
Tests (finding 4): handler-level ladderStatefulStore drives the real
executeLadderRun -> Allocate -> BuildTranches -> persist pipeline.
- Convergence+preservation: 3 constant-usage runs build the ramp once
then Hold, preserving the exact scheduled tranche IDs; no accumulation.
- Drift-up: higher usage re-opens the netted gap, cancels the old
generation, and leaves exactly one live generation.
Both fail on the pre-fix unconditional-supersede code and pass after.
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
cristim
changed the title
…lace (L5) Conditional cancel-and-replace fought the netting on drift-up and silently capped coverage at the old ramp's size. Trace: run1 target8 -> ramp8; run2 target16, inFlight=8 -> gap=8, but supersede CANCELLED the old 8 and inserted a new 8 -> total scheduled 8 vs target 16; run3 stuck at 8 forever. The coverage-decay L5 must prevent, moved to drift-up. Root fix - netting + APPEND-ONLY: - persistLadderRun ALWAYS calls SaveLadderRunWithTranches: it inserts the run's new scheduled tranches and never cancels existing ones. - Remove SaveLadderRunWithTranchesAndSupersede entirely: the StoreInterface entry, the PostgresStore impl + its cancel UPDATE, and every mock/test stub (analytics, server, shared mock). - Why append-only converges: netting already subtracts existing scheduled tranches from the gap, so each run appends exactly the delta to reach target-E. run1 appends 8; drift-up run2 appends 8 more (gap=16-8) -> total 16 = target; run3 nets to gap 0 -> Hold, preserve. No double-commit (netting caps it), no pile-up past target, no re-dating (append keeps prior fire dates), no oscillation. Tests: - Drift-up rewritten to a 3-run APPEND convergence: run1 target8 -> scheduled 8; run2 target16 -> appends to total 16 with run1's IDs still present; run3 -> Hold, preserve 16. (Old assertion codified the bug by treating scheduled==8-while-target==16 as correct.) - Keep the constant-usage preservation test (build once, Hold+preserve). - Add an over-commit guard: constant usage across 3 runs -> total scheduled never exceeds target-E and Hold runs append no tranches. - ladderStatefulStore is now append-only (no supersede path). - Store integration test asserts the append-only contract (both generations stay scheduled; in-flight sums both). - Drift-DOWN (usage drop -> Hold -> too-large ramp preserved) stays a documented, separately-tracked follow-up. All new lint issues (misspell on British spellings) resolved.
926a04d to
cfd4364
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 36 minutes. |
|
Merged to main after independent adversarial money-path review (verdict SHIP, mutation-verified: deleting the netting line makes the convergence test fail) + all CI green. Fixes the drift-up under-commit where the prior conditional cancel-and-replace design capped coverage at ~50% of target as the target rose. Now nets open scheduled tranches into the gap before Allocate and appends only the delta (append-only; netting caps prevent double-commit), converging to target-E over runs; removes SaveLadderRunWithTranchesAndSupersede. Hardening follow-ups tracked in LeanerCloud/cloud-commitments-go#36. |
Summary
Makes the ladder scheduled-task planner converge to the configured target coverage across repeated unattended runs, by (1) netting the config's in-flight scheduled tranches out of the gap and (2) persisting new tranches append-only. Relates to LeanerCloud/cloud-commitments-platform#70 (ladder phase-3 pipeline / scheduled task).
The bug (coverage decay on drift-up)
The prior conditional cancel-and-replace design (and before it, unconditional supersede) fought the netting and silently capped coverage below target on any usage increase:
SaveLadderRunWithTranchesAndSupersedeCANCELLED the old 8 and inserted the new 8, so total scheduled stayed 8 while target was 16.The under-commitment equals the cancelled amount: the exact coverage-decay L5 must prevent, just relocated from the Hold path to the drift-up path. Repeated unattended runs never reach target.
The fix: in-flight netting + append-only
GetInFlightLadderCommitUSDHrsumsstatus = 'scheduled'tranches only (fired/completed are already reflected inExistingUSDPerHour, so counting them again would double-subtract and under-purchase).persistLadderRunALWAYS callsSaveLadderRunWithTranches: it inserts the run's new scheduled tranches and never cancels existing ones.target - E:Removed
SaveLadderRunWithTranchesAndSupersedeThe cancel-and-replace store method was the wrong mechanism. Fully removed: the
StoreInterfaceentry, thePostgresStoreimpl and its cancelUPDATE, and every mock/test stub (analytics, server, shared mock). No dead exported code remains.Convergence proof (3-run, handler-level, real Allocate -> BuildTranches -> persist pipeline)
TestExecuteLadderRun_L5DriftUp_AppendsToTargetAndConverges: run1 target 8 -> scheduled 8; run2 target 16 -> APPENDS to total 16 with run1's tranche IDs still present; run3 -> Hold, preserve 16.TestExecuteLadderRun_L5Convergence_PreservesRampAcrossHolds: constant usage builds once, then Holds preserving the exact scheduled tranche IDs.TestExecuteLadderRun_L5OverCommitGuard_NettingCapsAppends: constant usage over 3 runs never exceedstarget - E, and Hold runs append zero tranches.Fail-before verified: disabling the netting subtraction makes all three tests fail (accumulation to 16/24); restoring passes them. Integration test (
store_postgres_ladder_test.go, testcontainers PG) asserts the append-only store contract and the scheduled-only in-flight sum.Drift-down
A Hold preserving a now-too-large ramp after a usage drop is intentionally accepted for now, bounded by the low-water clamp in
Allocate, and documented in code as a separately-tracked follow-up.Gates
go build, go vet, golangci-lint (0 new issues vs main), gocyclo -over 10 (non-test), pkg/ladder tests, internal/server + internal/config unit tests, and integration tests all pass.