[cristim]

Summary

• All three providers (AWS, Azure, GCP) report savings as already-net monthly savings -- the on-demand/recurring delta AFTER the amortized upfront cost is factored in.
• The old effectiveSavingsPct and effectiveMonthlySavings subtracted amortized_upfront_per_month from r.savings again, double-counting the upfront and driving high-upfront 3yr RI percentages deeply negative (e.g. -53%) even when the purchase genuinely saves money.
• Fix: numerator is r.savings directly. The denominator reconstruction (monthly_cost + savings + amortized) is unchanged -- the on-demand baseline still equals what you would pay without the reservation.

Closes #1103

Test plan

• effectiveMonthlySavings: returns r.savings directly; updated 4 stale assertions that encoded the old gross-savings assumption.
• effectiveSavingsPct: numerator changed from savings - amortized to savings; updated 5 stale test assertions plus the 2 integration rendering tests (all-upfront row now shows 50.0% not 0.0%; negative-effective test uses savings=-90 directly).
• New regression test: 3yr all-upfront AWS RI (savings=30, upfront=2988, on_demand=100) goes from -53% (pre-fix) to +30% (post-fix).
• All 2436 tests pass (1 pre-existing skip unrelated to this PR).

Labels (mirrored from issue #1103)

priority/p2 severity/medium urgency/this-sprint impact/many effort/s type/bug triaged

---

Generated by Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2e4b9d0b-d219-4452-94f9-26b8a37e828c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/1103-effective-savings-pct

---

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

[cristim]

@coderabbitai review

---

Generated by Claude Code

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details and diff
gh pr view 1110 --repo LeanerCloud/CUDly
echo "---FILES CHANGED---"
gh pr diff 1110 --repo LeanerCloud/CUDly

Length of output: 21304

---

---

Review of PR #1110: drop double-upfront-subtract from effectiveSavingsPct

Summary: The fix is correct, minimal, and well-tested. Here's my analysis:

Core logic change

effectiveMonthlySavings (recommendations.ts lines 1265–1295)

• Old: r.savings - r.upfront_cost / monthsInTerm — subtracted amortized upfront from already-net savings → double-counted.
• New: return r.savings — returns the provider-supplied net figure directly. ✓

effectiveSavingsPct

• Numerator: r.savings - amortized → r.savings. ✓
• Denominator reconstruction is unchanged and remains mathematically correct: if savings = gross − amortized, then onDemand = monthly_cost + gross = monthly_cost + savings + amortized. The algebra holds. ✓
• amortized is still computed (needed only for the denominator fallback, not the numerator anymore). ✓
• term=0 early return via !r.term prevents division by zero — unaffected. ✓

Test coverage

Scenario	Old result	New (correct) result
savings=50, upfront=600, term=1, on_demand=100	0.0% (all-upfront row)	50.0%
savings=30, upfront=2988, term=3, on_demand=100 (regression #1103)	-53%	+30%
AWS partial-upfront savings=45, upfront=120, on_demand=150	23.33%	30%
Azure D11_v2 with on_demand_cost=122.64	21.88%	23.65%
No-upfront (upfront_cost=0)	unchanged 20%	20% ✓

The negative-effective test is correctly reworked: instead of engineering a negative result artificially through savings - amortized with large upfront, it now uses savings=-90 directly — which correctly tests the case where a provider-reported savings figure is genuinely negative.

Minor observations (no action needed)

1. effectiveMonthlySavings is now a trivial identity function. Keeping it as a named export maintains API stability and the updated JSDoc documents why it's a pass-through — appropriate.
2. The old JSDoc said savings excluded upfront amortization ("EXCLUDING upfront amortization, matches AWS Cost Explorer's EstimatedMonthlySavingsAmount"). The regression evidence in #1103 and the new test (-53% for a genuinely profitable purchase) confirms the old comment was wrong about provider semantics.

The fix is clean. Approving.

[approve]

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 277
File: frontend/src/__tests__/recommendations.test.ts:2472-2476
Timestamp: 2026-05-05T07:46:01.902Z
Learning: In the CUDly frontend (`frontend/src/recommendations.ts`), `effectiveSavingsPct` intentionally has NO plausibility guard (no null return or warning for reconstructed percentages above per-term ceilings). The maintainer explicitly rejected that approach in PR `#277`. The fix for inflated percentages (issue `#274`) is purely a data-path fix: plumbing the provider's canonical `on_demand_cost` through to the frontend so the denominator is not reconstructed from potentially misleading `monthly_cost + savings + amortized`. Tests that pin the reconstructed >80% behavior are intentional documentation of the pre-fix path, not bugs.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:45:38.069Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/cloudsql/client.go`, `getSQLPricing` had the same per-hour/term-total unit mismatch as memorystore and cloudstorage: `commitmentPrice` (per-hour SKU rate) was passed directly to savings math that expected a term total, producing ~99.99% savings, and `HourlyRate` was set to `per-hour / hoursInTerm` (near-zero). Fixed in PR `#1047` commit 20590c6b4 (issue `#1078` folded in): `commitmentPriceTerm := commitmentPrice * hoursInTerm`; `HourlyRate = commitmentPrice` (raw per-hour). `convertGCPRecommendation` also hardcoded `rec.Term = "1yr"`, ignoring `params.Term`; fixed with same defaulting pattern as memorystore/cloudstorage. Regression tests: `TestGetSQLPricing_CommitmentPriceIsTermTotal` and `TestCloudSQLConvertGCPRecommendation_PropagatesTermFromParams`. PaymentOption was already correct (`if paymentOption == "" { paymentOption = "monthly" }`).

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
Learning: In LeanerCloud/CUDly GCP service converters (`convertGCPRecommendation` in memorystore/client.go and cloudstorage/client.go), `rec.Term` must be derived from `params.Term` with a `"1yr"` default — not hardcoded to `"1yr"`. Without this, 3-year callers always emit 1-year commitments even though the downstream `termYears` derivation from `rec.Term` is correct. Fixed in PR `#1047` commit c6280c390 (F2 for memorystore, F4 for cloudstorage). Regression tests: `TestConvertGCPRecommendation_PropagatesTermFromParams` and `TestCloudStorageConvertGCPRecommendation_PropagatesTermFromParams`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `convertGCPRecommendation` must force `paymentOption = "monthly"` unconditionally (logging a WARN when a non-monthly value such as "upfront", "all-upfront", or "partial-upfront" is supplied) because GCP CUDs are monthly-only and any non-monthly value passed through would be a silent misconfiguration. Fixed in PR `#1047` commit c6280c390 (F1). Regression test: `TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:13:37.445Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `convertGCPRecommendation` no longer hardcodes `Term = "1yr"` (fixed in PR `#1047` commit 8f9a787, H-3 audit finding). It propagates `params.Term` with a `"1yr"` default, validates via `termPlan`, and returns nil on unknown term. This is stricter than memorystore/cloudstorage/cloudsql (which default and continue): computeengine returns nil on unknown term because it is on the purchase path. Regression tests: `TestConvertGCPRecommendation_PropagatesParamsTerm`, `TestConvertGCPRecommendation_RejectsUnknownTerm`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:34:23.754Z
Learning: In LeanerCloud/CUDly Azure pricing extractors (compute, cache, database, cosmosdb, search, managedredis), Azure's Retail Prices API returns "1 Year" (singular) for 1-year reservation terms and "3 Years" (plural) for 3-year terms. All 6 pricing extractors use `azureTermString(termYears int) string` helper (added in PR `#1045` commit 5e938d9) to produce the correct singular/plural form for term matching. Prior to this fix, "%d Years" format produced "1 Years" which never matched Azure API responses, causing all 1-year reservation pricing lookups to silently fail. Regression tests: `TestExtractVMPricing_SingularOneYear`, `TestExtractVMPricing_PluralThreeYears`, `TestExtractRedisPricing_SingularOneYear`, `TestExtractRedisPricing_PluralThreeYears`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:13:37.445Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, the `memMBPerVCPU = 4096` const was removed in PR `#1047` commit 8f9a787. MEMORY MB is now extracted directly from the Recommender payload via `memoryMBFromOperationGroups` (sibling of `vcpuCountFromOperationGroups`). `buildInsertRequest` and `GroupCommitments` call `memoryMBFromDetails(rec)` which returns an error (no silent fallback) when `Details.MemoryGB` is absent. This ensures high-memory families (e.g. N2-highmem) get the correct MB from the payload rather than the GENERAL_PURPOSE approximation. Regression test: `TestBuildInsertRequest_RefusesMissingMemory`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly `providers/gcp/recommendations.go`, `collectRegion` fans out to all four GCP service clients (computeengine, cloudsql, memorystore, cloudstorage) concurrently (fixed in PR `#1047` commit 95a390f, H-2 finding). `regionResult` gains `cache` and `storage` slices; merge is updated accordingly. The decision was to wire (not trim) memorystore and cloudstorage because both have complete `GetRecommendations` implementations and their no-op `PurchaseCommitment` paths are orthogonal to surfacing advisory recommendations. Regression tests: `TestRegionResult_HasCacheAndStorageFields` and `TestShouldIncludeService_Cache_Storage`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.792Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T19:32:26.182Z
Learning: In LeanerCloud/CUDly `providers/aws/services/rds/client.go`, `findOfferingID` validates AZConfig with a switch that has a `default: error` case covering non-empty invalid values (e.g. "typo-az"), not just an empty-string check. A bad non-empty AZConfig previously fell through to `multiAZ==false` in `paginateRDSOfferings` (same mis-buy as the old default). Fixed in PR `#1085` commit 681fd9e81. Regression test added.