fix(ladder): stop subtracting existing commitment from residual baselines

[IAmJSD]

Refs #282 (head 284a500). This is the shared-engine part of the fix. It doesn't close the issue yet (see "Why not Closes" below).

Root cause

The AWS ladder baseline is a residual series. It counts only EC2 On Demand Instances spend, so it is the usage that existing RIs and SPs left uncovered. Allocate treated every baseline as total demand and ran gap = min(target, low_water) - existing. That subtracted the existing commitment a second time. On a partly covered account this returns a "target already met" Hold while usage is still uncovered.

I re-checked current main (740dfbd) before changing anything. #350, #404 and #413 added building blocks and scoped the residual observations. None of them changed this formula, so the bug still reproduces (evidence below).

Fix

• New UsageBaseline.Basis (BaselineBasis). BaselineBasisTotal is the default and also covers the empty legacy value. BaselineBasisResidual marks a series that is already net of existing commitments. Any other value is an error.
• On a residual baseline, Allocate sizes gap = low_water * target% - in_flight and doesn't subtract existing. The base split also doesn't subtract the existing base from the stable level. The OD it buys never exceeds the observed residual.
• Target below 100% with existing commitment in any layer: the true gap is pct*(C+R) - C, where C is the covered usage the residual leaves out. It is at most pct*R, but it can't be sized without C. So the run is held, with an explanation and the upper bound, instead of being guessed. If even that upper bound is below the $0.01 minimum, it is the usual "target already met" Hold.
• With no existing commitment in any layer, the residual and total bases give the same result at any target.
• On a residual basis, expiring commitment is not added back. Its coverage is still in effect during the observed window, and turning native commitment dollars into covered usage needs rates. Usage it stops covering later shows up in later observations as a top-up, never an over-buy.
• Rationales on residual runs append baseline_basis=residual (existing commitment already excluded from low_water, not subtracted again). existing= is still shown.
• The total-basis path is unchanged byte for byte: the existing tests pass untouched and its rationales gain nothing.
• The InFlight doc now states the residual contract. Callers must include active commitment that the observed series doesn't reflect yet, for example one bought after the last CE day because of reporting lag. Otherwise it is re-planned.

Worked numbers (from the tests)

Target 100%, buffer 0, no in-flight, Compute SP rate 0.7 unless noted.

Case	Existing	Residual low water	Before (total formula)	After (residual basis)
Issue replay	$6/hr Compute SP	$4/hr	Hold "target already met", gap 4 - 6 = -2	buy OD $4/hr = commitment $2.80/hr
Uncovered control (rate 1)	0	$10/hr	OD 10	OD 10 (same)
Zero existing, 80% (rate 1)	0	$10/hr	OD 8	OD 8 (same)
Fully covered (rate 1)	$6/hr	$0 / $0.005 / $0.02	Hold	Hold / Hold / OD 0.02
Partial target 80% (rate 1)	$6/hr	$4/hr	Hold (gap 3.2 - 6)	Hold "cannot size a partial target", upper bound $3.20
In flight	$6/hr	$4/hr	Hold	$2.80 in flight at 0.7 (= OD 4): Hold. $1.40 at 0.7 (= OD 2): buy OD 2 = commitment 1.40
Expiring (rate 1)	$6/hr, $2/hr expiring	$4/hr	Hold (4 - (6 - 2) = 0)	OD 4 (not 4 + 2)
Base split (rate 1, buffer 10%, stable 5)	$3/hr EC2 Instance SP	$10/hr	n/a	base 5 (not 5 - 3), flex 4, buffer 1, total 10

A seeded property test runs 2000 random cases over existing, expiring, in-flight, rates, buffer, cap and target. It checks that bought OD ≤ max(0, R - in_flight_od) ≤ R, that commitment ≤ OD, and that a partial target with existing commitment never buys.

Verification

All commands were run locally on macOS (Go workspace, five modules), with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. No cloud APIs were called and nothing was purchased.

• make build, make vet, make test-unit: pass.
• (cd pkg && GOWORK=off go test -race -short -count=1 ./...): pass.
• (cd providers/aws && GOWORK=off go build -mod=readonly ./... && GOWORK=off go test -mod=readonly -race -short -count=1 ./ladder/... ./recommendations/...): pass. The provider module is unchanged, and the full AWS suite also ran inside make test-unit.
• golangci-lint v2.10.1 run --allow-parallel-runners ./... in pkg: 0 issues.
• gocyclo -over 10 on the changed non-test files: silent. Allocate is at 10.
• git-secrets --scan on the changed files with the AWS patterns registered: clean. A positive control, an account_id literal with a non-allowlisted 12-digit value, was flagged.
• scripts/gosec-hook.sh on the changed files: clean. markdownlint-cli@0.47.0 on CHANGELOG.md: clean. git diff --check: clean.

Fail-before / pass-after. The new tests use the new field, so on main they fail to compile, not on behavior. The behavioral fail-before evidence is two things:

1. The issue replay also asserts the total-basis control. It still returns Hold ... gap=$-2.0000/hr with these inputs, which is the main behavior.
2. A mutation that restores the pre-fix formula for residual baselines (existingForBasis always returns existing) fails 6 of the new tests, including the issue replay.

Mutation probes on the fix. Each was applied in a scratch copy and the full pkg/ladder suite was run.

Mutation	Result
Pre-fix formula on the residual basis	killed (6 tests)
Total basis also skips existing (over-buy)	killed (11 tests, mostly existing ones)
No partial-target Hold	killed (partial-target test + property test)
Partial-target Hold also at 100%	killed
Partial-target Hold before the target-met check	killed
Residual base split keeps existing base	killed
hasExistingCommitment always false / always true	killed / killed
Ignore target % on the residual basis	killed
Residual skips in-flight	killed
Validate accepts any basis	killed

End-to-end scratch check (not committed). I wrote a temporary test through the real aws/ladder.New, GetUsageBaseline, GetLayerStates, SupportedLayers and pkg/ladder.Allocate, run in workspace mode. It used boundary fakes, a 30-day series and a one-year Compute SP. Without the AWS change it reproduces the issue: partly-covered residual=4 existing=6 allocations=0, gap=$-2. With a temporary one-line AWS change (Basis: ladder.BaselineBasisResidual in GetUsageBaseline) it gives compute-sp 4.00. The uncovered control stays at 10 and the fully covered case stays a Hold. All AWS ladder tests pass with that change in workspace mode.

Why not Closes, and the follow-up

The AWS module pins the published pkg (v0.0.0-20261015085730-498b74f31f01). Under the CI step "Verify standalone provider modules" (GOWORK=off), the one-line AWS adoption fails with unknown field Basis. So it has to land after this pkg version is published, the same split as #352 → #353 → #404.

The follow-up is: bump the AWS pkg pin, set Basis: BaselineBasisResidual in providers/aws/ladder/baseline.go, and add the end-to-end regression above as a committed test.

That follow-up should also decide the CE-lag case. A commitment bought after the last observed CE day is in existing but not yet in the residual. Under the documented InFlight contract the platform must net it, and today the platform deliberately leaves completed tranches out of in-flight. Until that is handled, the residual basis can re-plan such a purchase for a few days. The executor is still disabled (platform #70/#118), so no money moves.

Out of scope (still tracked by #282 / #349)

• Sizing targets below 100% from a residual baseline with existing coverage. This needs covered on-demand-equivalent observations, for example GetSPCoveredSeries (feat(aws/recommendations): daily SP covered series and SP on-demand-equivalent ratio #350) plus RI covered usage.
• Normalizing existing and expiring commitment to OD units (E/k, cohort rates).
• Pre-buying replacements for expiring commitment on a residual basis.
• Platform pin bump, in-flight accounting for CE lag, persisted tranche rates, executor adoption.
• Platform plan display (assemble...): it computes its own target/existing/gap summary and will want to show the basis after the pin bump.
• The Basis field is untagged, so a platform JSON marshal of LadderPlan.Baseline gains "Basis":"".

Maintainer: please mirror the issue's labels on this PR (priority/p2, severity/medium, triaged, urgency/this-sprint, impact/few, effort/m, type/bug). I don't have label access on this repo.

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

Or wait 33 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-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: e5dca6e6-6168-4bce-9253-8f38bde1fdf6

📥 Commits

Reviewing files that changed from the base of the PR and between 2bf23fb and ee57bfd.

📒 Files selected for processing: 6

• CHANGELOG.md
• pkg/ladder/allocate.go
• pkg/ladder/allocate_residual.go
• pkg/ladder/allocate_residual_test.go
• pkg/ladder/capability.go
• pkg/ladder/types.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 verdict: CLEAN at 284a500

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

What changed in the math:

• Before, Allocate always computed gap = min(target, low_water) − existing. That is right for a total-demand baseline, but it double-subtracts on an uncovered-only (residual) series. That is the #282 reproduction: 4 − 6 = −2, a false "target met".
• A new UsageBaseline.Basis field defaults to total (empty or "total"), and that path is unchanged. With Basis="residual", existing coverage counts as 0 in the gap and in the base split nets (allocate.go:~185, existingForBasis; allocate_residual.go:~66).
• A residual baseline with a target below 100% and any existing commitment is held, not guessed, because the true gap needs the covered usage that a residual series omits.

Blocking findings: none.

Over-buy analysis:

• Nothing sets Residual in this PR, and AWS GetUsageBaseline is untouched, so production behaviour is identical.
• On the residual path, existing coverage is not counted as demand. The AWS series is unblended cost filtered to "On Demand Instances", which excludes covered RI/SP usage.
• The partial-target Hold removes the main over-buy path. Units stay in OD dollars, rates are applied later in buildResult as before, and big.Rat is used throughout.
• API: additive only (BaselineBasis, UsageBaseline.Basis, plus a doc change on the capability interface). No platform break; JSON gains "Basis":"".

Non-blocking notes:

1. Adoption risk (follow-up PR): a commitment bought after the last Cost Explorer day is already in existing but not yet in the residual, so the residual basis would no longer net it. The AWS follow-up must not set Basis=Residual until in-flight accounting covers that lag. Your body discloses this, and the ladder executor is currently disabled.
2. hasExistingCommitment (allocate_residual.go:~96) holds a partial-target run on any existing amount in any layer. That is the safe direction.
3. There is no AWS end-to-end test yet; it is deferred to the follow-up.

What was attacked and how (local macOS, GOTOOLCHAIN=go1.26.9):

• go vet ./ladder/... is clean, and go test -race -count=1 ./ladder/... passes 293.
• Fail-before: with the non-test files reverted to main, the new tests fail to compile (undefined: BaselineBasis), as the body says.
• Mutation probes:
  • Killed: restoring the pre-fix formula, changing the Hold threshold from 100 to 50, hasExistingCommitment changed to >= 0, and keeping nets in the base split.
  • Survived (cosmetic): the target-met rationale text.
• Issue #282's reproduction matches the pre-fix formula.

Gate:

• No closing issue (refs #282).
• CI must be green at this SHA; fork runs may need maintainer approval.
• The draft must be marked ready.

[cristim]

Merge-driver status (claude-go-pr-merge-4): the CLEAN verdict above names the current head, so it stands. CI is green on that head and labels are mirrored. @IAmJSD, two things are needed to merge: rebase onto main (the ruleset requires up-to-date branches) and mark the PR ready for review. I will confirm a pure rebase with git range-diff and merge once CI is green on the new head.

[IAmJSD]

Brought up to date with main in cdc43c2 as a merge commit rather than a rebase, so no force-push was needed. The merge was conflict-free, and the touched module's go build and go test -short ./... pass locally (GOWORK=off; AWS config files nulled). git diff <previous head> cdc43c2 contains only main's changes.

[cristim]

The head moved to cdc43c28 (a merge of main ca5e743f into the reviewed 284a5001). This is a pure main merge, so the CLEAN verdict for 284a5001 carries over:

• git range-diff shows the PR commits identical (=).
• The new head's tree 9f150be2 equals git merge-tree --write-tree origin/main 284a5001 exactly, so there are no hand edits in the merge.

CI is re-running on cdc43c28. Still a draft: @IAmJSD, please mark it ready for review.

[IAmJSD]

Merged main again in 2611eb5 to pick up #485. CI now runs main's scripts/run-gosec.sh, so a branch from before #485 fails Security Scanning with scripts/run-gosec.sh: No such file or directory. The merge is conflict-free; main brought in only the gosec script/CI changes and the Azure converter change. No changes to this PR's own code.

[cristim]

The head moved to 2611eb5b, a merge of main into the reviewed head. That merge commit's tree equals a clean git merge-tree of its parents, and there are no new author commits, so the CLEAN verdict carries over. CI is green on 2611eb5b (pre-commit and Build & Test). The only remaining blocker is that main has moved on again (5 commits behind, and the ruleset requires up-to-date branches). I'm asking the owner to authorize a maintainer update of this branch. @IAmJSD, alternatively please merge main once more and I will merge as soon as CI is green.

[IAmJSD]

Merged main again in ee57bfd (no conflicts). The affected module's build and go test -short ./... pass under GOWORK=off with AWS config files nulled.

[cristim]

Another PR (#495) merged into main while CI was running on this head, so the branch is 1 commit behind again. CI is green and the verdict stands. @IAmJSD, one more main merge, and I will merge as soon as CI is green. I've asked the other merge drivers to hold.

[cristim]

Merge driver status: blocked on the author. The branch now conflicts with main in CHANGELOG.md, which an Update branch merge cannot resolve. @IAmJSD, please merge main and resolve the CHANGELOG entry. The CLEAN verdict at ee57bfd6 carries over to a clean main merge, and I will merge once CI is green. Separately, this PR links no issue and carries no triage labels. Please add Closes #N if one applies.