[cristim]

Summary

GetCommitmentRates (providers/aws/ladder/commitment_rates.go) fetched only the EC2 Instance SP rate. On ErrNoSPCommitmentRate it returned an empty map. That was harmless with one layer, but any layer added later would be dropped whenever the EC2 rate is missing. This is the part that every option for #349 needs, whichever way the open Compute SP decision goes.

• GetCommitmentRates now fetches through fetchLayerRates(ctx, src, layers, base), one GetSPCommitmentRate call per layer:
  • ErrNoSPCommitmentRate: only that layer is absent (held), and the loop continues;
  • any other error: nil map and the error, wrapped with the layer name;
  • a rate that is not finite or is <= 0: an error.
    Nothing defaults to 1:1 or 0.
• The layer list has one entry today, EC2 Instance SP with the ladder region, so behaviour is unchanged. Compute SP stays absent until the owner decides (fix(ladder): on-demand baseline dollars are bought 1:1 as commitment dollars (over-buys by 1/(1-discount)) once the executor ships #349). The comment above the list states its account-wide EC2+Lambda+Fargate blend and the bias that blend has against a regional EC2-only gap.

Tests

• The fake rate source is keyed by plan type and records its calls.
• The EC2 cases: present, missing, hard error, and NaN/Inf/0/negative rates. The params are pinned: plan type, account, region, term, payment and lookback.
• TestFetchLayerRates_LayersAreIndependent uses two synthetic layers:
  • first missing, second present: returns only the second;
  • first present, second a hard error: nil map and the error;
  • both missing: returns {}.
• Fail-before: restoring the old empty-map return fails the independence subtests first_missing_second_present and both_missing.
• commitment_rates_sdk_test.go is unchanged and passes.
• go build, go vet, go test ./ladder/ and golangci-lint (0 issues) pass, and so does pre-commit, all rebased on main 8344b3d.

Part of #349

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 35 minutes.

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

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-go
• Branch: refactor/ladder-per-layer-rate-fetch
• File: .coderabbit.yaml
• Commit: c700726
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 820b398d-29fe-4dde-b103-b8e0e8433230

Commits

Reviewing files that changed from the base of the PR and between 07b7b48 and c700726.

Files selected for processing: 2

• providers/aws/ladder/commitment_rates.go
• providers/aws/ladder/commitment_rates_test.go

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

APPROVED 32ff7fd by claude-qa-ui-4

Cold review. I didn't write this change.

Checked against the stated contract (providers/aws/ladder/commitment_rates.go):

• Each layer is fetched independently: fetchLayerRates (:53-71) copies base per layer and sets PlanType and Region from the layer.
• ErrNoSPCommitmentRate leaves out only that layer: it hits continue (:59-61), and the other layers still run.
• Any other error fails loudly: it returns a nil map, with the layer named (GetCommitmentRates: layer %s: %w, :62-64).
• Non-finite and ≤0 rates are errors (:65-67): they're never mapped to 1:1 or 0.
• The layer list is EC2 Instance SP only (:43-45), with the regional account and the same term, payment and lookback as before. So behaviour is unchanged except that a ≤0 or non-finite EC2 rate is now an error instead of being passed through. Compute SP stays absent, with the reason commented (:41-42).
• Callers: the only implementation is the pkg/ladder/capability.go:12 interface method, and the nil-map-plus-error contract matches it.

Probes in a scratch copy (each must fail a test):

• restore the old empty-map return on ErrNoSPCommitmentRate: fails TestFetchLayerRates_LayersAreIndependent/first_missing_second_present (the fail-before the body cites);
• break instead of continue: fails the same test;
• treat ErrNoSPCommitmentRate as an error: fails the same test;
• neutralise the unusable-rate guard, with math kept in use so it still builds: fails TestGetCommitmentRates_RejectsUnusableRate for zero, negative, nan and inf;
• replace an unusable rate with 1: fails TestGetCommitmentRates_RejectsUnusableRate/zero.

Runs (scoped and throttled, in providers/aws, under heavy-review.lock): GOMAXPROCS=4 go test -p 2 -count=1 ./ladder/ passes, and go vet ./ladder/ passes.

[cristim]

APPROVE @ 32ff7fd by claude-nibblets

Cold review of 32ff7fd, the shared part of #349 (ladder sizing, money path), done in my own detached worktree. The branch is 0 behind main. The PR body was treated as a claim.

Against what correct means

• Each layer's rate is fetched independently. fetchLayerRates (providers/aws/ladder/commitment_rates.go) copies base per layer and sets PlanType and Region from the layer. SPCommitmentRateParams (providers/aws/recommendations/sp_commitment_rate.go:24-38) holds only scalars, so the per-layer copy can't leak state between layers.
• ErrNoSPCommitmentRate drops only that layer. It is continue, so the layer is absent and the others are still fetched.
• Any other error fails loudly. It returns a nil map and an error naming the layer (GetCommitmentRates: layer <layer>: %w), wrapping the cause, so errors.Is still matches.
• Unusable rates are errors. NaN, ±Inf and values of zero or below are refused with a nil map, never treated as 1:1 or 0. The source already refuses NaN and r <= 0 (sp_commitment_rate.go, GetSPCommitmentRate), so this is the ladder's own check and doesn't depend on the source.
• Behaviour is unchanged today. The production list has only LayerEC2InstanceSP with the ladder's region. A missing rate still gives an empty non-nil map, as the old map[...]{} return did, and a valid rate still gives {EC2InstanceSP: rate}. Compute SP stays absent, as the code comment says, pending the owner's decision.

Verification at this SHA

In providers/aws, with GOWORK=off GOMAXPROCS=4, under the shared review lock: go vet ./ladder/ is clean and go test -p 2 -count=1 ./ladder/ passes.

Mutation probes (each reverted): 9 of 9 killed.

Mutation	Result
The old empty-map return restored on a missing rate (the claimed fail-before)	killed (TestFetchLayerRates_LayersAreIndependent/first_missing_second_present, /both_missing)
The unusable-rate check removed	killed (TestGetCommitmentRates_RejectsUnusableRate)
Zero accepted (< 0)	killed (RejectsUnusableRate/zero)
Inf accepted	killed (RejectsUnusableRate/inf)
A hard error swallowed as absent	killed (first_present_second_hard, HardErrorNamesLayer, AbsenceAndErrors)
A partial map returned with a hard error	killed (AbsenceAndErrors/hard, first_present_second_hard)
The layer name dropped from the error	killed (HardErrorNamesLayer)
Region not taken from the layer	killed (LayersAreIndependent, all three subtests that check the region)
Compute SP added to the production layer list	killed (AbsenceAndErrors/absent, ForwardsSelection/0). The unchanged-behaviour claim is pinned

CI at this SHA: Lint for every module, pre-commit, Integration Tests, Snyk and Lint Workflows pass. Unit Tests and Security Scanning were still running when I posted this. CodeRabbit is NEUTRAL.

Not blocking

• No upper bound. A rate is commitment dollars per covered on-demand dollar, and nothing in the ladder or the source refuses a rate above 1. A Savings Plan that costs more than on-demand isn't a usable sizing input. Since this PR already treats the rate as a money-path input with a validity check, (0, 1] would match the range #686's observed rates enforce. A follow-up would do.

Not merging.

[cristim]

APPROVED c700726 by claude-qa-ui-4

Merge-only delta from my APPROVE at 32ff7fd.

• c7007262 has parents 32ff7fdb and 07b7b48d (go main, fix(aws): refuse purchase counts that don't fit int32 before any AWS call #684).
• The PR's diff against main is identical. git diff 07b7b48d c7007262 | git patch-id --stable equals the same for 32ff7fdb against its merge base (36efb9738744), and it touches only providers/aws/ladder/commitment_rates.go and commitment_rates_test.go.
• The main side of the merge (fix(aws): refuse purchase counts that don't fit int32 before any AWS call #684) changes nothing under providers/aws/ladder.

The earlier verification (5/5 probes killed, ladder tests and vet green) applies unchanged.

[cristim]

APPROVE @ c700726 by claude-nibblets

Merge-only delta review of 32ff7fdb...c7007262, the update-branch onto main. I approved 32ff7fdb earlier. This is a comment review because GitHub does not let the PR author's account approve its own PR.

• The merge brings in only main's #684. c7007262 has two parents: 32ff7fdb (the approved head) and 07b7b48d (#684, "refuse purchase counts that don't fit int32 before any AWS call"), which is the current main tip. #684 is the only commit between them.
• No conflict resolution. git show --remerge-diff c7007262 is empty, so git merged it automatically with no hand edits.
• No overlap. main's side of the merge doesn't touch providers/aws/ladder. #684 changes the aws/services purchase paths and offeringprice.
• The PR's own change is identical. git diff 07b7b48d c7007262 (2 files, +123/-19) is byte-identical to 32ff7fdb's diff against its base, apart from index lines. So the code I reviewed and mutation-probed at 32ff7fdb (9 of 9 probes killed) is what merges.

CI at this SHA: Lint for every module, pre-commit, Lint Workflows and Snyk pass. Unit Tests, Integration Tests and Security Scanning were still running when I posted this. CodeRabbit is NEUTRAL.

Not merging.

[cristim]

Merged at head c700726 as 790f369. Gate evidence:

• Cold approvals at 32ff7fd from claude-qa-ui-4 and claude-nibblets, then merge-only deltas from both at c700726. c700726 is update-branch's merge of main 07b7b48 (fix(aws): refuse purchase counts that don't fit int32 before any AWS call #684); the PR diff is byte-identical.
• CI green at the head. 0 unresolved threads. Merge state CLEAN, and main was still 07b7b48 at merge time.
• The Compute SP layer stays absent pending the owner decision on fix(ladder): on-demand baseline dollars are bought 1:1 as commitment dollars (over-buys by 1/(1-discount)) once the executor ships #349.