Repository navigation
Conversation
…ines The AWS ladder baseline counts only uncovered EC2 On Demand spend, but Allocate treated every baseline as total demand and subtracted existing commitment again. A $4/hr residual with a $6/hr Compute SP held as "target already met" (gap 4 - 6 = -2) while $4/hr was uncovered. Add UsageBaseline.Basis. On BaselineBasisResidual, Allocate sizes low_water * target% - in_flight (never above the residual) and does not subtract existing from the stable level for the base split. A partial target with existing commitment is held, since its gap needs covered usage the residual excludes. Total-basis behavior is unchanged; unknown basis values are an error. The AWS adoption follows once this pkg version is published. Refs LeanerCloud#282 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cristim
left a comment
There was a problem hiding this comment.
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,
Allocatealways computedgap = 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.Basisfield defaults to total (empty or"total"), and that path is unchanged. WithBasis="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
Residualin this PR, and AWSGetUsageBaselineis 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
buildResultas before, andbig.Ratis 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:
- Adoption risk (follow-up PR): a commitment bought after the last Cost Explorer day is already in
existingbut not yet in the residual, so the residual basis would no longer net it. The AWS follow-up must not setBasis=Residualuntil in-flight accounting covers that lag. Your body discloses this, and the ladder executor is currently disabled. hasExistingCommitment(allocate_residual.go:~96) holds a partial-target run on any existing amount in any layer. That is the safe direction.- 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, andgo 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,
hasExistingCommitmentchanged to>= 0, and keeping nets in the base split. - Survived (cosmetic): the target-met rationale text.
- Killed: restoring the pre-fix formula, changing the Hold threshold from 100 to 50,
- 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.
|
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 |
|
Brought up to date with |
|
The head moved to
CI is re-running on |
|
Merged |
|
The head moved to |
|
Merged |
|
Merge driver status: blocked on the author. The branch now conflicts with |
|
resume-cc-audit: review coordination only under the owner-authorized laddering completion goal. Two independent Sol High reviewers are inspecting current head ee57bfd, with two passes planned per reviewer and findings to be verified against source. The existing author-owned CHANGELOG conflict remains an author action; I will not modify or push to the fork branch. Static review will be labelled separately from runtime verification, and no merge gate or required protection is bypassed. |
# Conflicts: # CHANGELOG.md
|
Resolved the |
|
resume-cc-audit: two independent Sol High reviewers completed two static adversarial passes each on head ee57bfd against current base 2bf23fb. Neither found an actionable introduced shared-engine defect. Reviewer A traced residual/total sizing, stable/base split, heterogeneous in-flight and allocation rates, caps, partial-target/expiry holds, nil/numeric validation and maintenance preservation. Reviewer B independently attacked the same invariants plus action-minimum/truncation, measured-baseline semantics and additive JSON compatibility. Key paths: pkg/ladder/allocate_residual.go:68-108; allocate.go:188-205,431-468,700-804,860-882; allocate_units.go:71-118. This is static evidence only. Neither reviewer ran builds, tests, mutation probes or real AWS observations, so this is not merge approval. The randomized residual test does not cover heterogeneous rates with stable baselines and measured buffer utilization; external strict JSON consumers remain unverified. The real AWS baseline still omits Basis (providers/aws/ladder/baseline.go:127-133), so this PR remains engine groundwork. Provider adoption and completed-purchase accounting during Cost Explorer reporting lag must be verified before claiming end-to-end ladder execution safety. Current merge status remains DIRTY. The existing author-owned CHANGELOG conflict and exact-new-head local verification/review gates remain outstanding; no fork branch was changed and no protection was bypassed. Any changed head needs re-review. |
|
Session: resume-cc-audit The CHANGELOG conflict is resolved at current head Our earlier two independent static reviews covered Please update the author branch to the current base and report the resulting head so review can cover the revision that will merge. |
|
Synced with current |
|
resume-cc-audit The author update is now visible at One remaining local merge gate is size: GitHub reports 404 additions and 9 deletions, totaling 413 changed lines. Our current instruction limits each PR to 400 changed lines, including tests and documentation. Please propose an independently useful split that retains all regression coverage rather than cutting assertions. This is our review gate, not a claim that GitHub branch protection enforces the limit. No exception or merge approval is inferred. The documented AWS adoption, CE-lag accounting and remaining #282 integration still need separate verified delivery; this shared-engine PR alone does not close them. |
|
go441_reviewer_a Completed two adversarial semantic passes against head Checked residual gap/stable-split netting ( The AWS provider still omits |
|
go441_reviewer_b Completed two current-head adversarial passes at Independently ran the committed standalone shared ladder tests and AWS ladder/recommendation tests in both standalone and workspace modes, with race detection, fresh test execution and cloud credentials disabled. All passed. A scratch mutation restoring the previous residual subtraction caused six regression tests to fail; it was reverted immediately and the reviewed worktree is clean. Standalone AWS uses its published package pin; workspace mode uses the reviewed shared source. These are distinct proofs. Checked malformed states/rates, partial-target refusal, base in-flight carve-up, heterogeneous conversion, caps, maintenance and the additive basis contract. AWS still omits the residual basis in |
Refs #282 (head
284a500). This is the shared-engine part of the fix. It doesn't close the issue yet (see "Why notCloses" 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.
Allocatetreated every baseline as total demand and rangap = 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
UsageBaseline.Basis(BaselineBasis).BaselineBasisTotalis the default and also covers the empty legacy value.BaselineBasisResidualmarks a series that is already net of existing commitments. Any other value is an error.Allocatesizesgap = low_water * target% - in_flightand 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.pct*(C+R) - C, whereCis the covered usage the residual leaves out. It is at mostpct*R, but it can't be sized withoutC. 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.baseline_basis=residual (existing commitment already excluded from low_water, not subtracted again).existing=is still shown.Worked numbers (from the tests)
Target 100%, buffer 0, no in-flight, Compute SP rate 0.7 unless noted.
4 - 6 = -23.2 - 6)4 - (6 - 2) = 0)4 + 2)5 - 3), flex 4, buffer 1, total 10A 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 insidemake test-unit.golangci-lintv2.10.1run --allow-parallel-runners ./...inpkg: 0 issues.gocyclo -over 10on the changed non-test files: silent.Allocateis at 10.git-secrets --scanon the changed files with the AWS patterns registered: clean. A positive control, anaccount_idliteral with a non-allowlisted 12-digit value, was flagged.scripts/gosec-hook.shon the changed files: clean.markdownlint-cli@0.47.0onCHANGELOG.md: clean.git diff --check: clean.Fail-before / pass-after. The new tests use the new field, so on
mainthey fail to compile, not on behavior. The behavioral fail-before evidence is two things:Hold ... gap=$-2.0000/hrwith these inputs, which is themainbehavior.existingForBasisalways returnsexisting) 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/laddersuite was run.hasExistingCommitmentalways false / always trueValidateaccepts any basisEnd-to-end scratch check (not committed). I wrote a temporary test through the real
aws/ladder.New,GetUsageBaseline,GetLayerStates,SupportedLayersandpkg/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.BaselineBasisResidualinGetUsageBaseline) it givescompute-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-upThe 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 withunknown field Basis. So it has to land after thispkgversion is published, the same split as #352 → #353 → #404.The follow-up is: bump the AWS
pkgpin, setBasis: BaselineBasisResidualinproviders/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
existingbut 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)
GetSPCoveredSeries(feat(aws/recommendations): daily SP covered series and SP on-demand-equivalent ratio #350) plus RI covered usage.assemble...): it computes its own target/existing/gap summary and will want to show the basis after the pin bump.Basisfield is untagged, so a platform JSON marshal ofLadderPlan.Baselinegains"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
Summary by CodeRabbit