From 354adbd52913b0aaae8261a3a57d5514a7cc9f03 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 6 May 2026 13:19:39 +0200 Subject: [PATCH 1/3] fix(db/aws): flush stale recs cache missing on_demand_cost (migration 000049) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #312 added on_demand_cost population for AWS SP rows but shipped no cache-invalidation migration. Pre-#312 AWS rows in recommendations.payload lack the on_demand_cost key, so the frontend falls back to the broken reconstruction formula (which double-counts amortization), rendering implausibly high Effective % values (e.g. ~86% instead of ~22%). Migration 000049 follows #256's pattern: set monthly_cost to null on AWS rows that lack on_demand_cost, so the frontend renders "—" until the next scheduler tick repopulates with correct values. The WHERE clause is scoped strictly to AWS rows missing the field (idempotent on re-run). Closes #321 --- ...e_aws_recs_missing_on_demand_cost.down.sql | 5 ++++ ...ate_aws_recs_missing_on_demand_cost.up.sql | 30 +++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.down.sql create mode 100644 internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.up.sql diff --git a/internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.down.sql b/internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.down.sql new file mode 100644 index 00000000..92ee8e93 --- /dev/null +++ b/internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.down.sql @@ -0,0 +1,5 @@ +-- No rollback for the payload rewrite: the original AWS rows lacking +-- on_demand_cost were themselves stale (pre-PR-#312 data). Reverting +-- monthly_cost from null back to those stale values would re-introduce +-- the very bug this migration fixes. The correct values will be written +-- on the next scheduled collection. diff --git a/internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.up.sql b/internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.up.sql new file mode 100644 index 00000000..c8852664 --- /dev/null +++ b/internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.up.sql @@ -0,0 +1,30 @@ +-- Rewrite stale pre-PR-#312 AWS recommendation payloads that lack +-- on_demand_cost in their JSONB payload. +-- +-- PR #312 added on_demand_cost population for AWS Savings Plans rows +-- (via CurrentAverageHourlyOnDemandSpend × 730). PR #277 added it for +-- Azure rows. Neither shipped a cache-invalidation migration for the +-- existing AWS rows already in the DB. +-- +-- Pre-#312 AWS rows have no on_demand_cost key, so the frontend falls +-- back to reconstructing the on-demand denominator from +-- monthly_cost + savings + amortized — a formula that double-counts +-- amortization vs. how AWS computes its savings amounts, producing +-- implausibly high Effective % values. +-- +-- Setting monthly_cost to null here causes the frontend to render "—" +-- instead, which is accurate ("data not yet collected") until the next +-- scheduler tick re-collects with correct on_demand_cost values. +-- +-- NOTE: We scope this strictly to AWS rows that lack on_demand_cost. +-- Azure rows were handled by migration 000046. GCP rows are not affected. +-- +-- NOTE: This UPDATE is idempotent — once a row has on_demand_cost +-- (written by the next scheduler tick), the NOT (payload ? 'on_demand_cost') +-- condition no longer matches it, so re-running this migration is safe. +-- +-- See GitHub issue #321 for the full root-cause investigation. +UPDATE recommendations + SET payload = jsonb_set(payload, '{monthly_cost}', 'null'::jsonb) + WHERE payload->>'provider' = 'aws' + AND NOT (payload ? 'on_demand_cost'); From d6861314c931a5b1b0dd620bafcfe1a69e3406fe Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 6 May 2026 13:20:04 +0200 Subject: [PATCH 2/3] fix(recommendations/aws): warn-log when on-demand cost inputs are nil MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add structured warn-logs in the RI and SP parsers when the AWS CE API fields that populate OnDemandCost are absent from the response: - parseAWSCostDetails (parser_ri.go): logs when EstimatedMonthlyOnDemandCost is nil, which causes OnDemandCost=0 → stored as nil by the scheduler's nonZeroPtr helper → frontend reconstruction fallback. - parseSavingsPlanDetail (parser_sp.go): logs when CurrentAverageHourlyOnDemandSpend is nil, same downstream effect. Both logs follow the respective file's existing logging style (fmt.Printf for parser_ri.go, log.Printf for parser_sp.go). The logs make it observable when the fallback path is taken, aiding diagnosis for issue #321 and future regression detection. --- providers/aws/recommendations/parser_ri.go | 6 ++++++ providers/aws/recommendations/parser_sp.go | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/providers/aws/recommendations/parser_ri.go b/providers/aws/recommendations/parser_ri.go index 4d75f998..5d3f5e46 100644 --- a/providers/aws/recommendations/parser_ri.go +++ b/providers/aws/recommendations/parser_ri.go @@ -127,6 +127,12 @@ func (c *Client) parseAWSCostDetails(rec *common.Recommendation, details *types. if onDemand, err := strconv.ParseFloat(*details.EstimatedMonthlyOnDemandCost, 64); err == nil { rec.OnDemandCost = onDemand } + } else { + // EstimatedMonthlyOnDemandCost absent from AWS CE response — OnDemandCost + // will be 0 and the scheduler's nonZeroPtr will store nil, causing the + // frontend to fall back to the reconstruction formula. Log so operators + // can detect when the API field is missing. See #321. + fmt.Printf("Warning: EstimatedMonthlyOnDemandCost is nil for RI recommendation (service=%s, account=%s) — Effective %% will use reconstruction fallback\n", rec.Service, rec.Account) } // RecurringStandardMonthlyCost is the recurring charge per month for this RI. // It is distinct from CommitmentCost (upfront) and EstimatedMonthlySavingsAmount. diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index 6523caf2..283e969b 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -147,6 +147,13 @@ func (c *Client) parseSavingsPlanDetail( // where monthly_cost reflects only the no-upfront recurring charge, not // the full on-demand baseline). See #303. onDemandCost := parseOptionalFloat("CurrentAverageHourlyOnDemandSpend", detail.CurrentAverageHourlyOnDemandSpend) * hoursPerMonth + if detail.CurrentAverageHourlyOnDemandSpend == nil { + // CurrentAverageHourlyOnDemandSpend absent from AWS CE response — onDemandCost + // will be 0 and the scheduler's nonZeroPtr will store nil, causing the + // frontend to fall back to the reconstruction formula. Log so operators + // can detect when the API field is missing. See #321. + log.Printf("WARNING: CurrentAverageHourlyOnDemandSpend is nil for SP recommendation (planType=%s, account=%s) — Effective %% will use reconstruction fallback", planType, aws.ToString(detail.AccountId)) + } planTypeStr := string(planType) switch planType { From 351bb9116b9ab96828093a4b8e6d147ee48203dc Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Wed, 6 May 2026 13:30:19 +0000 Subject: [PATCH 3/3] fix: apply CodeRabbit auto-fixes Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit --- providers/aws/recommendations/parser_ri.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/providers/aws/recommendations/parser_ri.go b/providers/aws/recommendations/parser_ri.go index 5d3f5e46..fe0ffbf7 100644 --- a/providers/aws/recommendations/parser_ri.go +++ b/providers/aws/recommendations/parser_ri.go @@ -2,6 +2,7 @@ package recommendations import ( "fmt" + "log" "math" "strconv" "time" @@ -132,7 +133,7 @@ func (c *Client) parseAWSCostDetails(rec *common.Recommendation, details *types. // will be 0 and the scheduler's nonZeroPtr will store nil, causing the // frontend to fall back to the reconstruction formula. Log so operators // can detect when the API field is missing. See #321. - fmt.Printf("Warning: EstimatedMonthlyOnDemandCost is nil for RI recommendation (service=%s, account=%s) — Effective %% will use reconstruction fallback\n", rec.Service, rec.Account) + log.Printf("WARNING: EstimatedMonthlyOnDemandCost is nil for RI recommendation (service=%s, account=%s) — Effective %% will use reconstruction fallback", rec.Service, rec.Account) } // RecurringStandardMonthlyCost is the recurring charge per month for this RI. // It is distinct from CommitmentCost (upfront) and EstimatedMonthlySavingsAmount.