fix(aws/recommendations): protect RateLimiter against concurrent access (closes #271) - #865
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 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 (10)
📝 WalkthroughWalkthroughReplaces the shared ChangesRate limiter factory, SP params pointer, and errgroup panic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Rebased Conflict resolution ( The fix (08-C1) is unchanged and applied at all four goroutine call sites:
Verification post-rebase:
Note: @coderabbitai review |
|
Triggering a fresh review of the rebased branch now. 🧠 Learnings used✅ Action performedReview finished.
|
…ilent defaults (#1085) * fix(aws/savingsplans): error on unknown payment-option/term, no silent default convertPaymentOption and convertTermToSeconds previously logged a warning and silently substituted All Upfront (largest cash outlay) or 1 year when given an unrecognized or empty value, risking a mis-purchase on any typo or new/renamed AWS enum value. Both functions now return an explicit error on unrecognized input so the purchase and offering-lookup paths fail loud instead of buying the wrong product. Mirrors the pattern already used by the RDS convertPaymentOption. Regression tests for H1 (unknown/empty payment option) and H2 (unknown/empty term) are added to TestClient_FindOfferingID_AllPaymentOptions and TestClient_FindOfferingID_TermVariations; the previously-passing "default term" case is updated to assert the error path. Fixes H1 and H2 from docs/code-review/19-hardcoded-fallbacks-aws.md (PR #1039). * fix(aws): fail loud on unknown payment-option/term/engine Fixes H3, H4, M1-M6, L1, L2 from the 19-hardcoded-fallbacks-aws.md audit plus H1/H2 from the now-merged #1039 branch (savingsplans). H3 (converters.go): add convertPaymentOptionE/convertTermInYearsE/ convertLookbackPeriodE that error on unrecognized input; wire into the SP recommendation path (parser_sp.go). The legacy non-erroring wrappers are retained for the RI path in client.go (owned by #865/#1075) with a deprecation comment and a follow-up reference. H4 (elasticache/client.go): convertPaymentOption now returns (string, error) mirroring the RDS sibling that already errors correctly. H1/H2 (savingsplans/client.go): convertPaymentOption and convertTermToSeconds now return (type, error); cherry-picked from the merged #1039 branch where the fix was staged. M4 (parser_services.go, rds/client.go): parseRDSDetails no longer silently defaults AZConfig to "single-az" when CE omits DeploymentOption; findOfferingID rejects empty AZConfig with an explicit error. M5 (parser_services.go): resolveEC2Tenancy now maps only the known CE values ("shared"/nil -> default, "dedicated" -> dedicated); any other value (e.g. "host" for Dedicated Hosts) returns an error rather than silently collapsing to default tenancy. M6 (rds/client.go): normalizeEngineName now returns (string, error) and errors for ambiguous Oracle ("oracle*"), SQL Server ("sqlserver*"/"sql-server*"), and bare Aurora inputs; unambiguous engines (mysql, postgresql, mariadb, aurora-mysql, aurora-postgresql) pass through unchanged. M1/M2/M3 (ec2/client.go): replace raw "convertible" literals with types.OfferingClassTypeConvertible; buildEC2OfferingQuery errors on empty Platform (purchase path); centralize "Linux/UNIX" literal into defaultEC2Platform constant (exchange-path helpers retain their safe fallback; the purchase path does not). Regression tests added for H3, H1/H2, H4, M2, M4, M5, M6, L1, L2; each fails pre-fix and passes post-fix. Closes #1084 * test(aws/rds): clarify ambiguous-engine regression test Simplify TestNormalizeEngineName_AmbiguousErrors: remove the confusing oracle-se2 case (strings.Contains("oracle-se2","oracle") is true, so it errors like all other oracle inputs -- no special case needed) and remove the misleading inline comment. All remaining cases assert that any Oracle, SQL Server, or bare Aurora input errors, which is the invariant under test. * refactor(aws): extract helpers to fix gocyclo violations in rds and savingsplans gocyclo -over 10 fails on two functions added by the fail-loud refactor: - savingsplans.(*Client).findOfferingID (complexity 12): extract resolveSPPlanType and buildSPOfferingsInput helpers - rds.(*Client).paginateRDSOfferings (complexity 11): extract fetchRDSOfferingPage to isolate the per-page API call and scan Also apply gofmt to rds/client.go (trailing whitespace / brace style). Behaviour and error paths are unchanged; all rds and savingsplans tests still pass. * fix(aws/rds): tighten AZ-config and engine-name validation (CR #1085) Three related fail-loud fixes in the AWS RDS purchase path: 1. parseRDSDetails: unknown DeploymentOption now errors instead of silently folding into "single-az". Only "Multi-AZ" and "Single-AZ" are accepted; any other non-nil value (e.g. "Multi-AZ-Readable-Standbys") returns an explicit error so the bad token surfaces rather than driving findOfferingID to the wrong RI class. 2. findOfferingID: AZConfig validation now uses a switch with an explicit default error case rather than only checking for empty string. A non-empty but invalid value (typo, future AWS enum addition) would previously fall through to multiAZ==false in paginateRDSOfferings, silently treating the bad input as single-AZ. 3. normalizeEngineName: Oracle and SQL Server checks changed from strings.Contains to == so that edition-qualified tokens (oracle-se2, oracle-ee, sqlserver-se, sqlserver-web, etc.) pass through as valid RDS ProductDescription values. The previous Contains check contradicted the error message itself, which told callers to supply those exact edition tokens. Regression tests added for all three: each new test was verified to fail against the pre-fix code and pass after.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
providers/aws/recommendations/client.go (1)
359-364: ⚡ Quick winReturn the unexpected wait error instead of panicking.
GetAllRecommendationsalready returns an error; if this invariant ever regresses, Line 363 can crash callers instead of surfacing the fan-out failure through the API.Proposed fix
if waitErr := g.Wait(); waitErr != nil { - // This branch is unreachable: every goroutine above returns nil. - // The panic makes the invariant explicit and visible to the race - // detector rather than silently discarding an unexpected error. - panic("errgroup returned non-nil despite all goroutines returning nil: " + waitErr.Error()) + return nil, fmt.Errorf("recommendations fan-out failed unexpectedly: %w", waitErr) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/aws/recommendations/client.go` around lines 359 - 364, The code panics when g.Wait() returns a non-nil error in the GetAllRecommendations function, but since this function already has an error return type, the panic should be replaced with a proper error return. Remove the panic statement and instead return the waitErr directly through the function's error return value to properly surface the failure through the API rather than crashing the caller.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@providers/aws/recommendations/client.go`:
- Around line 359-364: The code panics when g.Wait() returns a non-nil error in
the GetAllRecommendations function, but since this function already has an error
return type, the panic should be replaced with a proper error return. Remove the
panic statement and instead return the waitErr directly through the function's
error return value to properly surface the failure through the API rather than
crashing the caller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09455575-bad4-4852-940f-bf3c0cb26acf
📒 Files selected for processing (8)
providers/aws/recommendations/client.goproviders/aws/recommendations/client_test.goproviders/aws/recommendations/coverage.goproviders/aws/recommendations/parser_sp.goproviders/aws/recommendations/parser_sp_additional_test.goproviders/aws/recommendations/parser_sp_test.goproviders/aws/recommendations/utilization.goproviders/aws/service_client.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
) RateLimiter was a single *RateLimiter field on Client, shared across the 6 concurrent goroutines that GetAllRecommendations fans out via errgroup. Reset/ShouldRetry/GetRetryCount all mutate retryCount without a lock, triggering data races under go test -race. Per feedback_rate_limiter_per_call.md: each goroutine needs its own independent retry budget (not shared throughput), so per-call instantiation is correct over adding a mutex. Replace rateLimiter *RateLimiter with newRateLimiter func() *RateLimiter. Each fetch*WithRetry / fetch*Page function calls rl := c.newRateLimiter() at entry. Tests inject a factory returning a faster limiter for speed. Also fix mockCostExplorerAPI: callCount and riCalls were mutated by concurrent goroutines without synchronisation. Add sync.Mutex guard.
…-touched files - godot: add missing periods on doc comments in client.go, client_test.go, parser_sp.go - misspell (US locale): catalogue->catalog, behaviour->behavior, amortised->amortized, cancelled->canceled in touched files - errcheck: replace blank _ = g.Wait() with explicit invariant-enforcing panic so the linter sees the error handled - gocritic unlambda: extract normalizeFilters closure to package-level normalizeFilterSet function - gocritic hugeParam: suppress on functions taking RecommendationParams by value (read-only; pointer-cascade is a larger refactor deferred to a follow-up issue) - nolint:gocritic on NewClient (aws.Config by-value per SDK convention)
Remove 6 gocritic hugeParam nolint suppressions from providers/aws/recommendations: - NewClient now accepts *aws.Config instead of aws.Config; callers in service_client.go updated to pass &cfg. - getSavingsPlansRecommendations, fetchSPAllPages, parseSavingsPlansRecommendations, parseSavingsPlanDetail, planTypesForParams all accept *common.RecommendationParams instead of the value type; internal callers updated. Tests in client_test.go, parser_sp_test.go, and parser_sp_additional_test.go updated to pass pointer arguments.
…lization Rebase on main surfaced sp_coverage.go (added on main, not touched by this PR) still calling the removed shared c.rateLimiter field. Convert its fetchSPCoveragePage and fetchSPUtilizationPage to build a fresh RateLimiter per call via c.newRateLimiter(), matching the race-fix already applied to the RI, coverage, utilization and SP-recommendation paths (feedback_rate_limiter_per_call). This removes the last shared mutable RateLimiter access, so go test -race on the recommendations package is clean. Update the now-obsolete concurrency doc comments on GetSPCoverageSummary and GetSPUtilization, and add the two GetSavingsPlans* methods main added to the CostExplorerAPI interface to mockCostExplorerClient so the aws package test suite compiles.
…ladder caller Post-rebase fix: ondemand_series.go was added to main after this PR was cut, still using c.rateLimiter (removed by this PR). Convert to c.newRateLimiter() per-call pattern matching coverage.go/utilization.go. Also update ladder/factory.go to pass &awsCfg to recommendations.NewClient which now takes *aws.Config after the pointer-ize commit in this PR.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Remove a stray blank line introduced in dfa7a01 that left the file non-gofmt-clean and failed the pre-commit gofmt hook. The file now matches origin/main exactly; no behavioural change.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
RateLimiterwas stored as a single*RateLimiterfield onClientand shared across all 6 concurrent goroutines spawned byGetAllRecommendations.Reset,ShouldRetry, andGetRetryCountall mutateretryCountwithout synchronisation, causing data races detected bygo test -race.rateLimiter *RateLimiterfield with anewRateLimiter func() *RateLimiterfactory. Each of the fourfetch*WithRetry/fetch*Pagefunctions now callsrl := c.newRateLimiter()at entry, giving every call site its own independent retry budget (correct: these are not shared throughput limiters).callCountandriCallsmutations inmockCostExplorerAPIwith async.Mutex- they were also racing whenGetAllRecommendationscalled the mock from multiple goroutines.Test plan
go test -race github.com/LeanerCloud/CUDly/providers/aws/recommendations/...passes (280 tests, no data race report)🤖 Generated with claude-flow
Summary by CodeRabbit
Bug Fixes
Refactor