fix(azure): pair the recommended SKU with the count in its own units

[cristim]

Closes #1540

The defect

Azure's consumption API carries two quantities on a reservation recommendation and they are not interchangeable:

field	counts
RecommendedQuantity	the actual SKU (SKUProperties[SKUName])
RecommendedQuantityNormalized	NormalizedSize

ResourceType and Count were resolved independently, so the Legacy/EA ladder took its SKU from NormalizedSize while the count came from RecommendedQuantity. Azure recommending 4 × Standard_D8s_v3, normalized to 16 × Standard_D2s_v3, became Standard_D2s_v3 × 4 — a quarter of the recommended capacity.

RecommendedQuantityNormalized appeared nowhere in the repo before this change. The field was never read, so no path produced the correct pair.

It reaches a purchase, not a display

Carried through the compute converter into buildReservationBody, these are the bytes sent to calculatePrice and purchase:

{"sku":{"name":"Standard_D2s_v3"},
 "properties":{"quantity":4,"term":"P1Y","reservedResourceType":"VirtualMachines", ...}}

Azure has no reason to reject it: Standard_D2s_v3 is a real SKU and 4 is a valid quantity. The purchase succeeds and the customer is committed for the term. The only quantity guard anywhere on the path is rec.Count <= 0; nothing validated that the SKU and the count were in the same units.

And the money figures ride along unchanged — same run:

common.Recommendation: ResourceType="Standard_D2s_v3" Count=4
                       OnDemandCost=1000 CommitmentCost=600 EstimatedSavings=400

EstimatedSavings is Azure's number for the full recommended quantity — the converter says so itself (NetSavings is the savings from buying the full recommended quantity). So $400/mo was reported against a purchase delivering roughly a quarter of it.

Direction

Under-buy — fails safe on commitment risk, unsafe on reported savings.

Nobody is locked into capacity they cannot use; that is the worse direction and it does not happen here. But the savings figure shown to whoever approves the purchase is wrong. A decision-quality defect rather than a spend defect — which is why "wrong purchases" would overstate it and "display only" would understate it.

Strictly, the direction is decided by Azure's data rather than by the code: under-buy iff NormalizedSize ≤ the actual SKU. The armconsumption@v1.1.0 doc comments are terse ("The recommended Quantity Normalized") and never state units, so that half rests on Azure's documented normalize-to-base-size behaviour. In-repo corroboration: all 34 Legacy fixture call sites treat NormalizedSize as the base size — Standard_D2s_v3, GeneralPurpose_Gen5_2, 100RU, DW500c, Premium_P1.

Scope — wider than the issue states

The issue cites one example consumer. It is seven services, all through the one shared recommendations.Extract:

compute, cache, cosmosdb, database, managedredis, search, synapse.

Count also feeds provider-agnostic logic in cmd/helpers.go, which the issue does not mention: applySizing/ApplyCoverage scale by newCount / rec.Count, ApplyTargetCoverage anchors on it, --max-instances clamps on it, and the duplicate checker compares existingCount >= rec.Count and subtracts. None of that is gated on provider, so the wrong count reached sizing math and duplicate suppression as well as the purchase body.

The fix

ResourceType and Count are now resolved together, so no branch can emit a mixed pair.

• The actual SKU is preferred, matching the Modern ladder, so EA and MCA subscriptions with identical usage yield the same ResourceType. Its partner RecommendedQuantity is also the *float64 field, avoiding a *float32 rounding question on a purchase path.
• When only NormalizedSize is available, its normalized count is the only valid partner.
• When that partner is absent, the count is left unset rather than borrowed from RecommendedQuantity — an unpaired count is precisely the defect above. Zero is refused by the Count <= 0 guard every service applies before purchasing, and a warning is logged. (Note there is no empty-ResourceType guard anywhere in providers/azure, so blanking the type instead would have put an empty sku.name on the wire.)
• A payload with no resource type from either source keeps its previous behaviour — there is no normalized size for the count to disagree with, so the pairing fix leaves that degenerate case alone.

Behaviour change worth calling out explicitly

This changes what gets purchased for EA customers. The purchased SKU moves from the normalized size to the actual size for every Legacy recommendation carrying both fields — which is the common real-world payload shape. That is the point of the fix, but it is a real change and should not pass unnoticed.

Modern was extended too — a correction to my own earlier assessment

I reported during investigation that Modern/MCA was unaffected. That is true of the rung real MCA payloads take (SKUName present), and this PR leaves that rung byte-identical — pinned by TestExtract_Modern_ResourceTypePrefersSKUNameOverNormalizedSize.

But while editing I found that with SKUName absent, Modern's NormalizedSize rung mixed the same two fields exactly as Legacy did. Fixing Legacy alone would have left one branch of the same function still able to emit mismatched units on a money path, so it is paired here rather than deferred. Flagging it as a deliberate, named extension rather than something slipped in.

Tests — both directions, plus a downstream consumer

A test asserting only that the bad pairing is gone passes against a converter that produces nothing, so every case asserts the SKU and the quantity that must accompany it.

• TestExtract_LegacySKUAndQuantityStayInMatchingUnits — three payload shapes; each valid pair must still be produced.
• TestExtract_LegacyNormalizedSizeWithoutNormalizedQuantityLeavesCountUnset — the one state with no valid pairing.
• TestExtract_Modern_NormalizedSizeRungIsCountedNormalized and the SKUName-rung test above.
• TestPurchaseBody_SKUAndQuantityStayInMatchingUnits (services/compute) — the downstream half. The pairing is fixed in the shared converter, but the defect only mattered because it reached the wire; this parses the actual request body and asserts sku.name and quantity together. A converter-only test would have looked right while the bytes Azure receives stayed wrong, which is how the defect survived.

Mutation-tested

Five mutations, each reverting one property of the fix. Every one is killed by a named test:

mutation	killed by
M1 — original bug restored (NormalizedSize + un-normalized count)	3 converter tests and both downstream cases
M2 — normalized rung counted by RecommendedQuantity	2 converter tests
M3 — unpaired count borrowed from RecommendedQuantity	the unset-count test
M4 — Modern normalized rung counted by RecommendedQuantity	the Modern rung test
M5 — legacy pairing returns nothing at all	3 converter tests

M5 is the trap this design exists to avoid, and M1 shows the downstream test reporting the exact wrong wire values:

expected: "Standard_D8s_v3"   actual: "Standard_D2s_v3"   (sku.name is what Azure reserves)
expected: 16                  actual: 4                   (quantity must be expressed in units of sku.name)

Fixtures

The Legacy and Modern builders now set both quantities, and WithQuantity / WithModernQuantity set both: a fixture naming a NormalizedSize while setting only RecommendedQuantity describes a payload with no valid pairing. WithNormalizedQuantity / WithModernNormalizedQuantity make them differ deliberately, and WithoutNormalizedQuantity models the unpairable payload. All 34 pre-existing Legacy fixture call sites are unchanged and still pass.

Gates

gate	result
gofmt -l .	clean (0 files)
providers/azure: go build ./... + go vet ./... + go test -race -count=1 ./...	exit 0 — 12 ok, 0 FAIL
gocyclo -over 10 -ignore "_test\.go" (root and providers/azure)	exit 0, empty
golangci-lint v2.10.1 on providers/azure, comm set diff vs base	592 both sides; zero new, zero removed

The set diff earned itself: the first pass came back 592 → 597, and the diff named all five as mine — one behaviour misspell, two modelling misspells, and two unnamedResult from the new two-value returns. All fixed; the re-run is identical to base on both the untouched files (540 = 540, exact line-preserving comm) and the touched ones (by file+message, since my edits shift line numbers).

Both golangci runs initially exited 3 with zero findings — Error: parallel golangci-lint is running. That reads exactly like a clean run and was retried rather than trusted.

main is currently red on TestGrantCeiling_ConstraintContainment (internal/auth), from #1737 and #1758 colliding — confirmed failing at this branch's base commit, unrelated to this diff and untouched by it. This PR's CI will inherit it until that fix lands.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 5 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c9fa2ea4-3cfb-48b3-bfac-5c271acf142e

📥 Commits

Reviewing files that changed from the base of the PR and between 29beea0 and cd80e33.

📒 Files selected for processing (4)

• providers/azure/internal/recommendations/converter.go
• providers/azure/internal/recommendations/converter_test.go
• providers/azure/mocks/recommendation_fixtures.go
• providers/azure/services/compute/client_test.go

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Merging. Closes #1540.

Merging on independent adversarial review — CodeRabbit has no verdict at this head.

Legacy (EA) recommendations paired the normalized SKU with the un-normalized quantity. It reaches a purchase, not just a display.

The direction finding is the part worth preserving, and it survived attack: under-buy — fails safe on commitment risk, unsafe on reported savings. Nobody buys a commitment they should not; the savings figure shown to whoever approves the purchase is wrong. That is a decision-quality defect. Calling it "wrong purchases" would overstate it and "display only" would understate it, and the third framing is the accurate one.

The reviewer attacked that claim directly rather than accepting it — traced both resolvers by hand and stress-tested the float32/float64 conversion (RecommendedQuantityNormalized is *float32, RecommendedQuantity is *float64; widening is exact and int() truncates toward zero, so no path can inflate a count past the true recommended value). Direction claim confirmed.

Scope was wider than the issue was filed with: seven consuming services, plus provider-agnostic CLI sizing and duplicate suppression in cmd/helpers.go. All seven were traced statically — each threads f.ResourceType/f.Count from the shared recommendations.Extract straight through to its purchase bodys sku.name/quantity, each gated by a Count <= 0 refusal, and none re-derives the pair independently. cmd/helpers.go consumes rec.Count via ratios and floor division without re-deriving it, so it inherits the fix with no separate change.

The Modern/MCA negative was re-examined, not written off. The SKUName rung — what real MCA payloads take — pairs with RecommendedQuantity in both old and new code, byte-identical. But the NormalizedSize rung (SKUName absent) did carry the same mismatch and is fixed here. Not a half-fix.

Mutations re-run independently rather than trusted from the PRs table:

• M1 (restore the original bug) — killed by 6 converter tests plus 3 downstream compute tests, i.e. more coverage than the PR claimed, not less
• M5 (legacy pairing returns nothing) — killed loudly across 10 tests in 2 packages
• M4 (Moderns NormalizedSize rung counted by the wrong field) — killed by its dedicated test

All restored cleanly afterward, verified by diff against the PR commit.

One verified, non-blocking finding, recorded for a fast-follow. The deliberate reordering to check SKUProperties before NormalizedSize means resourceTypeFromSKUPropertiess "first non-empty value, whatever it is named" last-resort loop is now reached more often. Demonstrated: SKUProperties=[{Cores:4}] with a valid NormalizedSize produces ResourceType="4" — garbage from the Cores value — discarding a good pairing that was available.

The reviewer then asked the question that makes this reportable rather than alarming: does it weaken the direction claim? It does not. Under old code that same input hit the original #1540 bug — a silent under-buy where the purchase succeeds. Under new code it produces a SKU name Azure rejects outright: fails loud, no money moves. Safer-or-equal, with a different failure mode — availability/completeness, not money-safety. Suggested fix is to let the last-resort loop win only when NormalizedSize is also unavailable.

Note on the CI history here: this PR spent two cycles failing on TestGrantCeiling_ConstraintContainment, a test fixed on main by #1772. Close/reopen re-ran the workflows but did not reliably force GitHub to recompute refs/pull/N/merge, so they kept re-testing the old base — indistinguishable from the fix not working. Rebasing onto 29beea014 resolved it deterministically by putting the fixed test in this PRs own tree. The rebase was verified as the same commit object as the locally-gated one (cd80e338b7ca1b06b144cd96a1f04f7a17e4f706), so the pre-rebase gate results carry unchanged.

Gates at cd80e338b: 20/20 checks pass, zero unresolved threads. providers/azure full -race suite green across 13 packages, gocyclo 0 findings, and golangci-lint compared as a comm set diff normalized by file+message rather than by totals — 0 lines only-in-base, 0 only-in-PR, genuinely identical sets.