fix(aws/recs): RI coverage keeps only the last CE bucket but divides by the full lookback window

[cristim]

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Where

• providers/aws/recommendations/coverage.go:275-287 (fetchCoveragePaged, the loop over result.CoveragesByTime) with the record closures at :226 and :250
• providers/aws/recommendations/coverage.go:186 (windowHours = lookbackDays * 24) and :359-361 (poolCoverageFromGroup computing AvgInstancesPerHour = TotalRunningHours / windowHours)
• Correct sibling implementation: providers/aws/recommendations/sp_coverage.go:217-266 (spCoverageAccumulator.add / .summarize), which pins Granularity: DAILY and accumulates across every period before dividing
• Downstream consumers: providers/aws/recommendations/coverage.go:409-456 (ApplyCoverageMapToRecommendations) and providers/aws/ladder/layer_states.go:100 (computeEC2CoveragePct)

What

fetchCoveragePaged walks result.CoveragesByTime, which holds one entry per Cost Explorer time bucket, and for each bucket calls record(...), which does an unconditional out[key] = cov. Whatever bucket CE happens to return last silently replaces every earlier bucket, so the map ends up holding a single bucket's running hours.

AvgInstancesPerHour is then computed as TotalRunningHours / windowHours, where windowHours is the full lookback (lookbackDays * 24). So one bucket's hours are divided by the whole window.

The Savings Plans path does this correctly. The RI path is the outlier, and it is structurally unable to control the bucketing: it sets GroupBy and therefore cannot also set Granularity, so the number of buckets CE returns is not under its control.

Failure scenario

GetRICoverageMap(ctx, 30, ["us-east-1"]) run on the 2nd of a month. CE returns two buckets: the previous month, and the 2-day partial current month. The 2-day bucket wins.

A pool genuinely running 40 instances/hr reports TotalRunningHours of roughly 1920 (40 x 48h), divided by windowHours = 720, giving AvgInstancesPerHour = 2.67 instead of 40, roughly a 15x understatement. Pct is likewise taken from a 2-day sample rather than the requested 30 days.

ApplyCoverageMapToRecommendations then rescales every recommendation's AverageInstancesUsedPerHour down by that factor (recs[i].AverageInstancesUsedPerHour *= cov.AvgInstancesPerHour / recSum), so a --target-coverage run buys roughly 15x fewer RIs than the pool actually needs. The under-buy is silent: no error, no warning, and the numbers look internally consistent. The same distortion feeds the ladder's EC2 coverage input via computeEC2CoveragePct.

The magnitude depends on where in the month the run happens, so the same account sized on the 2nd and on the 28th produces materially different purchase plans from identical usage.

Fix direction

Accumulate TotalRunningHours and hour-weighted coverage across all CoveragesByTime entries per pool key before dividing, mirroring spCoverageAccumulator.add / .summarize, instead of last-write-wins. Add a test that feeds two buckets of unequal length for the same pool key and asserts the summed hours (this fails pre-fix, where only the last bucket survives).

Related

• LeanerCloud/cloud-commitments-cli#1482 is a different bug in the same data's consumer (computeEC2CoveragePct blending non-EC2 pool keys into reported EC2 coverage). Verified as distinct: LeanerCloud/cloud-commitments-cli#1482 is about which pool keys are included, this is about the producer discarding all but one time bucket. Fixing either one leaves the other live.
• ARCH-09: Coverage sizing logic trapped in package main; web coverage knob applies only to displayed savings cloud-commitments-cli#1198 (coverage sizing logic trapped in package main) is adjacent context for the --target-coverage path.

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.

A07-019 (medium)

The sibling this issue points at as correct has the same divisor bug. spCoverageAccumulator.add increments a.days only for buckets that carried a Coverage block (providers/aws/recommendations/sp_coverage.go:222-240), but summarize at :246-248 divides the accumulated dollars by the caller's lookbackDays*24 unconditionally (:355). Cost Explorer lags 24-48h and omits buckets with no eligible activity, so a 30-day request that returns 10 populated days reports CoveredUSDPerHour and EligibleUSDPerHour at a third of the true rate. Days is returned so a caller could detect it, but spCoverageAdapter.GetSPCoverageSummary (providers/aws/ladder/adapters.go:257) keeps only CoveragePct and no caller reads Days. CoveragePct itself is a ratio and is unaffected, and a repo-wide grep finds no production reader for the three rate fields, so nothing is displayed wrong today. Worth fixing in the same pass so the accumulator pattern this issue adopts is correct at the source. Finding A07-019.

[cristim]

The RI multi-bucket overwrite is reproduced and fixed separately: the public SDK/local HTTP scenario returned 2.67 instances/hour for a 40-instance pool before the fix, including downstream recommendation enrichment.

For appended A07-019, independent review found that replacing the Savings Plans divisor with populated days would change the requested-window average incorrectly for sparse usage. A 30-day fixture with 240USD reported across 10 active days returns 1/3USD/hour whether the other 20 days are omitted or explicitly zero. Dividing by Days instead returns 1USD/hour for the omitted form but 1/3 for explicit zeros. Coverage remains 50%, and the ladder adapter currently consumes only that percentage.

The full-window divisor is retained. A separate regression/documentation change will preserve this invariant and clarify that rates normalize reported totals over the requested window; Days is not a completeness guarantee.

AWS documents billing-data lag, but the GetSavingsPlansCoverage response has no completeness marker or missing-day reason. The same fixture payload could represent sparse valid usage or an incomplete tail; this does not demonstrate a live incomplete-data defect. Any completeness-aware API needs an independently justified signal or explicit conservative policy rather than inferring duration from Days. No live AWS calls were made. Issue51 remains open until the separate contract scope and completeness disposition are recorded.

Sources: GetSavingsPlansCoverage response, Cost Explorer freshness.

[cristim]

RI fix #167 is merged as 6ce98fbeb892a2215033a2d6ac75f28b742665f5. The merged tree exactly matches independently reviewed commit 7ce5060d50c59376d756f0c4aeafacbf9d01c7fd.

Fresh verification on the actual merge passed: focused recommendations and ladder coverage race tests, plus the public AWS SDK/local HTTP scenario through recommendation enrichment. The two unequal buckets now yield 40 instances/hour and 71.6667% coverage; the enriched recommendation retains 40 rather than the original 2.67. This is local fixture evidence, not live AWS execution.

The separate Savings Plans contract regression and documentation are in #168 at independently reviewed commit 19dba433cc6db5b63978b2b14dfe5b2c81fcc96d. Issue #51 remains open while that PR and the recorded reporting-completeness disposition are completed. The full requested-window SP divisor is retained; omitted dates alone do not establish a live incomplete-data defect.