fix(cli): --max-instances truncation does not re-derive ProjectedCoverage/ProjectedUtilization or NumberOfNodes · Issue #1845 · LeanerCloud/cloud-commitments-cli · GitHub
Skip to content

fix(cli): --max-instances truncation does not re-derive ProjectedCoverage/ProjectedUtilization or NumberOfNodes #1845

Description

@cristim

Surfaced during the adversarial review of the #1830 fix. Pre-existing, and left out of that PR because the fields are projections and node counts rather than money, so rescaling them does not belong in common.ScaleRecommendationCosts.

What

#1830 established that every quantity derived from Recommendation.Count must be re-derived when a sizing path changes Count. Two count-linear fields are still not re-derived when --max-instances truncates a row:

1. ProjectedCoverage / ProjectedUtilization

applyTargetCoverageRI computes ProjectedCoverage = ExistingCoveragePct + nTarget/avg*100 and a matching ProjectedUtilization, both linear in the quantity purchased. ApplyInstanceLimit truncates Count afterwards and recomputes neither, so a run combining --target-coverage with --max-instances emits a row claiming the coverage it was sized for rather than the coverage the truncated purchase will actually reach.

Concretely, --target-coverage 80 --max-instances 10 against a row sized to 100 instances writes Count=10 next to ProjectedCoverage=80.0. The CSV carries both columns, so the report is internally inconsistent in the same way #1830 described: a capped run overstating what it achieves, just denominated in percent rather than dollars.

2. DataWarehouseDetails.NumberOfNodes

Set to rec.Count at parse time in providers/aws/recommendations/parser_services.go and never re-derived by any sizing path (ApplyCoverage and ApplyTargetCoverage leave it stale too, so this predates the cap). After any Count change the mirror disagrees with the field it mirrors.

Why it matters

Neither field currently causes a wrong purchase. The Redshift purchase path reads rec.Count, not NumberOfNodes, and the projections are advisory. This is a reporting correctness issue, not a spend issue, which is why it is filed separately from #1830 rather than folded into it.

It still matters: --target-coverage exists so an operator can deliberately under-buy to a chosen coverage figure, and the projection column is the only feedback that the chosen figure was hit. A projection that silently ignores the cap tells the operator they reached 80% coverage on a run that bought a tenth of the instances.

Fix direction

For the projections, recompute in ApplyInstanceLimit for truncated rows from the fields already on the rec (AverageInstancesUsedPerHour, ExistingCoveragePct), mirroring the formulas in applyTargetCoverageRI / applyTargetCoverageSP and keeping the existing clamps to 100. Note the two formulas differ between RI and SP, so the recompute has to branch the same way the sizing does. Extracting the projection math into one helper shared by the sizing path and the cap is preferable to a third copy.

Where no signal exists (AverageInstancesUsedPerHour == 0), leave the projections rather than fabricating one, consistent with the project rule that absent is not zero.

For NumberOfNodes, decide whether the field should exist at all given it duplicates Count; removing a redundant mirror is likely better than teaching every sizing path to maintain it.

Context

common.ScaleRecommendationCosts documents both of these as deliberately out of scope, so the docstring should be updated to point at this issue once it is fixed.

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A10-001 (high)

A fourth count-reducing path, and this one drops the money fields too. adjustRecommendationForExcludedVersions (cmd/multi_service_engine_versions.go:479-488) sets rec.Count = newCount when instances sit on an extended-support engine version and leaves EstimatedSavings, CommitmentCost and UpfrontPayment at their pre-truncation values. The three sibling paths this issue's premise rests on all route through common.ScaleRecommendationCosts (cmd/helpers.go:189, cmd/helpers_count_override.go:56, pkg/recfilter/sizing.go:303); this one does not. A 10-instance RDS rec with 3 excluded versions buys 7 while the confirmation prompt, the CSV UpfrontPayment/EstimatedSavings columns and the TOTAL row all describe the 10-instance purchase, overstating spend and benefit by 30%. The adjusted rec reaches the purchase set via cmd/multi_service_filters.go:73. Finding A10-001.

A10-025 (medium)

A third count-mutating site for the invariant this issue states. adjustRecommendationForExcludedVersions reduces Count on the extended-support path, and its tests assert only result.Count -- cmd/multi_service_engine_versions_test.go:180, :185, :238, :255, :282 and cmd/multi_service_coverage_test.go:416 -- while no fixture sets EstimatedSavings or CommitmentCost at all. So whether the money fields are rescaled there is not merely unasserted, it is untestable with the current fixtures. The sibling flags already have exactly the coverage this path lacks (cmd/helpers_instance_limit_rescale_test.go, cmd/helpers_count_override_rescale_test.go), so the fix is a rescale test in that shape: a rec with non-zero EstimatedSavings/CommitmentCost, two of ten instances excluded, asserting both land at 0.8x. (audit finding A10-025)

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions