Repository navigation
fix(aws/recommendations): complete NaN/Inf money guards across parser_ri, ondemand_series, RI-utilization (follow-up to #1461) - #1473
Conversation
… too Addresses the Fable adversarial-review finding on this PR: the SP-path non-finite guard did not fully close the bug class. The RI sibling parser (parseCostInformation, parseAWSCostDetails) parsed the same CE money fields (EstimatedMonthlySavingsAmount/Percentage, UpfrontCost, EstimatedMonthlyOnDemandCost, RecurringStandardMonthlyCost) with bare strconv.ParseFloat and no guard, so a "NaN"/"Inf" value still flowed into CommitmentCost/OnDemandCost/savings/RecurringMonthlyCost. The doc comments also claimed the SP path "mirrors the RI path", which was false after the first commit. Changes: - Route both RI money parsers through the shared parseOptionalFloat, restoring genuine SP/RI parity (comments now hold). - parseOptionalFloat also rejects negative values (every field it parses is a non-negative financial metric; mirrors the repo's stricter parseSPFloat). - utilization.go parseFloat degrades NaN/Inf to 0 with a warning, so a single non-finite hours value can't poison every downstream utilization aggregate. Tests: - New TestRICostParsers_RejectNonFiniteAndNegative (verified it fails when the RI routing is reverted). - Extended TestParseOptionalFloat_RejectsNonFinite with negative cases + a zero-is-valid sanity check. - Updated TestParseRecommendationDetail_MalformedCostFields error-string assertions to the now-unified "failed to parse <Field> ..." wording.
…package Second round of the Fable adversarial review on this PR: it found the same non-finite bug class still open at three more parse sites in the recommendations package that the first two commits missed. - ondemand_series.go accumulateDailyResults: a "NaN"/"Inf" CE daily amount parsed with a nil error and slipped past the downstream all-zero fail-loud check (NaN != 0), poisoning the ladder baseline the engine sizes purchases from. Now fails loud on non-finite. NO negative guard here -- CE unblended cost is legitimately negative on credit/refund days. - parser_ri.go parseRIUtilizationSignals: NaN/Inf were stored into AverageInstancesUsedPerHour / RecommendedUtilization; the --target-coverage guards are all `<= 0` and NaN <= 0 is false, so a stored NaN was treated as a live signal and produced NaN purchase counts. Now routed through parseOptionalFloatOrWarn (warn + 0). - parser_ri.go parseRecommendedQuantity: fmt.Sscanf %f also accepts NaN/Inf; a non-finite quantity would corrupt the purchase count. Fail loud. Tests: - ondemand_series_test.go: TestGetOnDemandSeries_NonFiniteAmountFails. - parser_ri_test.go: non-finite-degrades-to-zero case in the utilization-signals table. - parser_sp_test.go: TestUtilizationParseFloat_DegradesNonFinite for the utilization.go parseFloat guard added in the previous commit.
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…tity (Fable follow-ups)
Addresses the Fable open-PR review findings on this PR:
- ladder/adapters.go mapActiveSP (MED, live money path): SavingsPlan.Commitment
parsed with bare strconv.ParseFloat and no non-finite guard, despite the doc
claiming it "fails loud on a non-numeric Commitment (money path)". A NaN/Inf
commitment flowed through sumSPHourlyCost / sumExpiringSPHourlyCost into the
ladder layer-state totals the engine sizes purchases from. Now rejects
NaN/Inf/negative (a commitment is a non-negative money rate).
- parser_ri.go parseRecommendedQuantity (LOW): rejected non-finite but still
accepted a negative purchase count ("-3" via Sscanf/Atoi). Now rejects negatives.
- parser_ri.go parseRIUtilizationSignals (LOW): the refactor dropped
service/account context from the parse-failure warning; the field label now
carries "service=.. account=.." so an operator can identify the corrupt row.
Regression tests: TestSPLister_NonFiniteOrNegativeCommitmentFails (verified fails
when the guard is reverted), plus NaN/Inf/negative cases in the quantity table.
|
Addressed the Fable open-PR review: guarded @coderabbitai review |
|
✅ Action performedReview finished.
|
1 participant
Follow-up to #1461 — completes the NaN/Inf/negative money guards that were stranded
#1461 merged with only its first commit (the SP-path
parseOptionalFloatNaN/Inf guard). The two follow-up commits that closed the rest of the same
bug class — added on the PR branch after an adversarial (Fable) review — never
reached
main. This PR brings them tomain. Verified on currentorigin/main:ondemand_series.gohas no non-finite guard, so these are live bugs.strconv.ParseFloat(andfmt.Sscanf %f) accept"NaN"/"Inf"with a nilerror, so a corrupt CE float slips through every
err == nil/!= 0/<= 0check downstream.
Sites fixed (all in
providers/aws/recommendations)parser_ri.gomoney parsers (parseCostInformation,parseAWSCostDetails):routed through the shared guarded
parseOptionalFloat, so the RI path rejectsNaN/Inf/negative exactly like the SP path (restores real SP/RI parity — the
doc comments claiming it already asserted a parity that didn't hold).
parseOptionalFloat: also rejects negative values (every field it parsesis a non-negative financial metric; mirrors the repo's stricter
parseSPFloat).ondemand_series.goaccumulateDailyResults: a"NaN"/"Inf"daily CEamount slipped past the downstream all-zero fail-loud check (
NaN != 0) andpoisoned the ladder baseline the engine sizes purchases from. Now fails
loud. No negative guard here — CE unblended cost is legitimately negative on
credit/refund days.
parser_ri.goparseRIUtilizationSignals: NaN/Inf were stored as a live--target-coveragesignal (NaN <= 0is false), producing NaN purchasecounts. Now routed through
parseOptionalFloatOrWarn(warn + 0).parser_ri.goparseRecommendedQuantity:Sscanf %falso accepts NaN/Inf;a non-finite quantity would corrupt the purchase count. Fail loud.
utilization.goparseFloat: degrades NaN/Inf to 0 (a single non-finitehours value would otherwise poison every utilization aggregate).
Verification
pass-on-fix (e.g. removing the
ondemand_seriesguard failsTestGetOnDemandSeries_NonFiniteAmountFails).go build ./...,go vet, fullrecommendationssuite (400+ tests) green.gocyclo -over 10clean (the refactor reduced complexity). Local golangci isv2.11.4 (CI pins v2.10.1); the diff adds no new findings — all pre-existing
package lint debt is on lines this PR did not touch.