fix(aws/recommendations): protect RateLimiter against concurrent access (closes #271)

providers/aws/recommendations/client.go

@@ -22,19 +22,22 @@ import (
 // payer org we have seen. Exceeding the cap returns a diagnostic error (issue #692).
 const maxRecommendationPages = 20
 
-// CostExplorerAPI defines the interface for Cost Explorer operations
+// CostExplorerAPI defines the interface for Cost Explorer operations.
 type CostExplorerAPI interface {
 	GetReservationPurchaseRecommendation(ctx context.Context, params *costexplorer.GetReservationPurchaseRecommendationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationPurchaseRecommendationOutput, error)
 	GetSavingsPlansPurchaseRecommendation(ctx context.Context, params *costexplorer.GetSavingsPlansPurchaseRecommendationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetSavingsPlansPurchaseRecommendationOutput, error)
 	GetReservationUtilization(ctx context.Context, params *costexplorer.GetReservationUtilizationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationUtilizationOutput, error)
 	GetReservationCoverage(ctx context.Context, params *costexplorer.GetReservationCoverageInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationCoverageOutput, error)
 }
 
-// Client wraps the AWS Cost Explorer client for RI recommendations
+// Client wraps the AWS Cost Explorer client for RI recommendations.
 type Client struct {
 	costExplorerClient CostExplorerAPI
 	region             string
-	rateLimiter        *RateLimiter
+
+	// newRateLimiter is called once per API call (not shared across goroutines).
+	// Tests can replace it with a factory returning a faster limiter.
+	newRateLimiter func() *RateLimiter
 
 	// ec2API is the EC2 client used to build the DescribeInstanceTypes paginator.
 	// Populated by NewClient from aws.Config; nil when created via NewClientWithAPI.
@@ -45,24 +48,24 @@ type Client struct {
 	// for hermetic tests. When nil, instanceTypeLookup returns (0,0).
 	instanceTypePagerFactory func() InstanceTypePager
 
-	// skuCatalog caches the per-instance-type vCPU/memory catalogue, fetched
+	// skuCatalog caches the per-instance-type vCPU/memory catalog, fetched
 	// lazily once per Client lifetime via sync.Once (one DescribeInstanceTypes
 	// fan-out per scheduler tick).
 	skuCatalog skuCatalog
 }
 
-// NewClient creates a new recommendations client
-func NewClient(cfg aws.Config) *Client {
+// NewClient creates a new recommendations client.
+func NewClient(cfg *aws.Config) *Client {
 	// Force Cost Explorer to use us-east-1 with explicit endpoint
 	ceConfig := cfg.Copy()
 	ceConfig.Region = "us-east-1"
 	ceConfig.BaseEndpoint = aws.String("https://ce.us-east-1.amazonaws.com")
 
-	ec2Client := awsec2.NewFromConfig(cfg)
+	ec2Client := awsec2.NewFromConfig(*cfg)
 	return &Client{
 		costExplorerClient: costexplorer.NewFromConfig(ceConfig),
 		region:             cfg.Region,
-		rateLimiter:        NewRateLimiter(),
+		newRateLimiter:     NewRateLimiter,
 		ec2API:             ec2Client,
 		// Factory wraps the EC2 client so the paginator is created lazily
 		// on the first EC2 recommendation parse (not at construction time).
@@ -72,29 +75,29 @@ func NewClient(cfg aws.Config) *Client {
 	}
 }
 
-// NewClientWithAPI creates a new recommendations client with a custom Cost Explorer API (for testing)
+// NewClientWithAPI creates a new recommendations client with a custom Cost Explorer API (for testing).
 func NewClientWithAPI(api CostExplorerAPI, region string) *Client {
 	return &Client{
 		costExplorerClient: api,
 		region:             region,
-		rateLimiter:        NewRateLimiter(),
+		newRateLimiter:     NewRateLimiter,
 		// ec2API left nil: instanceTypeLookup falls back to VCPU=0/MemoryGB=0
 		// unless the caller sets instanceTypePagerFactory.
 	}
 }
 
 // SetInstanceTypePagerFactory injects a pager factory for the instance-type
-// SKU catalogue. Must be called before the first GetRecommendations call.
+// SKU catalog. Must be called before the first GetRecommendations call.
 // Intended for tests that need to verify the one-fetch-per-lifetime invariant
 // without hitting AWS.
 func (c *Client) SetInstanceTypePagerFactory(f func() InstanceTypePager) {
 	c.instanceTypePagerFactory = f
 }
 
 // instanceTypeLookup returns the cached SKU entry for instanceType.
-// On the first call the catalogue is built by calling the pager factory.
-// ok=false when no factory is configured, the catalogue fetch failed, or
-// the instance type was not in the catalogue — the caller falls back to
+// On the first call the catalog is built by calling the pager factory.
+// ok=false when no factory is configured, the catalog fetch failed, or
+// the instance type was not in the catalog — the caller falls back to
 // VCPU=0/MemoryGB=0 (graceful-degradation contract from Azure PR #810).
 func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (instanceTypeSKUEntry, bool) {
 	if c.instanceTypePagerFactory == nil {
@@ -103,15 +106,15 @@ func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (i
 	return c.skuCatalog.lookup(ctx, instanceType, c.instanceTypePagerFactory)
 }
 
-// GetRecommendations fetches Reserved Instance recommendations for any service
+// GetRecommendations fetches Reserved Instance recommendations for any service.
 func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) {
 	// Handle Savings Plans separately — they use a different Cost Explorer API
 	// (GetSavingsPlansPurchaseRecommendation, not GetReservationPurchaseRecommendation).
 	// Match any SP slug — the legacy umbrella plus the four per-plan-type slugs —
 	// via the IsSavingsPlan family predicate so the dispatch keeps working as
 	// callers migrate.
 	if common.IsSavingsPlan(params.Service) {
-		return c.getSavingsPlansRecommendations(ctx, params)
+		return c.getSavingsPlansRecommendations(ctx, &params)
 	}
 
 	input := &costexplorer.GetReservationPurchaseRecommendationInput{
@@ -177,12 +180,12 @@ func (c *Client) fetchRIPageWithRetry(
 	ctx context.Context,
 	input *costexplorer.GetReservationPurchaseRecommendationInput,
 ) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) {
-	c.rateLimiter.Reset()
+	rl := c.newRateLimiter()
 	var result *costexplorer.GetReservationPurchaseRecommendationOutput
 	var err error
 
 	for {
-		if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil {
+		if waitErr := rl.Wait(ctx); waitErr != nil {
 			return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr)
 		}
 
@@ -191,13 +194,13 @@ func (c *Client) fetchRIPageWithRetry(
 		}
 		result, err = c.costExplorerClient.GetReservationPurchaseRecommendation(ctx, input)
 		concurrency.Release(ctx)
-		if !c.rateLimiter.ShouldRetry(err) {
+		if !rl.ShouldRetry(err) {
 			break
 		}
 	}
 
 	if err != nil {
-		return nil, fmt.Errorf("failed to get RI recommendations after %d retries: %w", c.rateLimiter.GetRetryCount(), err)
+		return nil, fmt.Errorf("failed to get RI recommendations after %d retries: %w", rl.GetRetryCount(), err)
 	}
 
 	return result, nil
@@ -297,7 +300,7 @@ func (c *Client) GetRecommendationsForService(ctx context.Context, service commo
 // the canonical order EC2 → RDS → ElastiCache → OpenSearch → Redshift after
 // all goroutines finish so order-sensitive consumers stay stable.
 //
-// Behaviour change vs the previous sequential loop: per-service errors are
+// Behavior change vs the previous sequential loop: per-service errors are
 // now logged at WARN via mergeServiceResults — the previous loop swallowed
 // them silently with a bare `continue`, leaving operators no signal when a
 // single service was misbehaving. Mirrors the Azure parallelisation in
@@ -353,7 +356,12 @@ func (c *Client) GetAllRecommendations(ctx context.Context) ([]common.Recommenda
 	// Wait, propagate ctx cancellation so callers can distinguish "all six
 	// services completed (with possibly per-service errors)" from "the
 	// parent ctx was canceled mid-fan-out".
-	_ = g.Wait()
+	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())
+	}
 	if err := ctx.Err(); err != nil {
 		return nil, err
 	}

providers/aws/recommendations/client_test.go

@@ -3,6 +3,7 @@ package recommendations
 import (
 	"context"
 	"fmt"
+	"sync"
 	"testing"
 	"time"
 
@@ -15,8 +16,9 @@ import (
 	"github.com/LeanerCloud/CUDly/pkg/common"
 )
 
-// Mock CostExplorerAPI for testing
+// Mock CostExplorerAPI for testing.
 type mockCostExplorerAPI struct {
+	mu                sync.Mutex
 	riRecommendations *costexplorer.GetReservationPurchaseRecommendationOutput
 	spRecommendations *costexplorer.GetSavingsPlansPurchaseRecommendationOutput
 	riError           error
@@ -28,20 +30,28 @@ type mockCostExplorerAPI struct {
 }
 
 func (m *mockCostExplorerAPI) GetReservationPurchaseRecommendation(ctx context.Context, params *costexplorer.GetReservationPurchaseRecommendationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) {
+	m.mu.Lock()
 	m.callCount++
 	m.riCalls = append(m.riCalls, params)
-	if m.riError != nil {
-		return nil, m.riError
+	riErr := m.riError
+	riRecs := m.riRecommendations
+	m.mu.Unlock()
+	if riErr != nil {
+		return nil, riErr
 	}
-	return m.riRecommendations, nil
+	return riRecs, nil
 }
 
 func (m *mockCostExplorerAPI) GetSavingsPlansPurchaseRecommendation(ctx context.Context, params *costexplorer.GetSavingsPlansPurchaseRecommendationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetSavingsPlansPurchaseRecommendationOutput, error) {
+	m.mu.Lock()
 	m.callCount++
-	if m.spError != nil {
-		return nil, m.spError
+	spErr := m.spError
+	spRecs := m.spRecommendations
+	m.mu.Unlock()
+	if spErr != nil {
+		return nil, spErr
 	}
-	return m.spRecommendations, nil
+	return spRecs, nil
 }
 
 func (m *mockCostExplorerAPI) GetReservationUtilization(ctx context.Context, params *costexplorer.GetReservationUtilizationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationUtilizationOutput, error) {
@@ -57,11 +67,11 @@ func TestNewClient(t *testing.T) {
 		Region: "us-west-2",
 	}
 
-	client := NewClient(cfg)
+	client := NewClient(&cfg)
 
 	assert.NotNil(t, client)
 	assert.NotNil(t, client.costExplorerClient)
-	assert.NotNil(t, client.rateLimiter)
+	assert.NotNil(t, client.newRateLimiter)
 	assert.Equal(t, "us-west-2", client.region)
 }
 
@@ -74,7 +84,7 @@ func TestNewClientWithAPI(t *testing.T) {
 	assert.NotNil(t, client)
 	assert.Equal(t, mockAPI, client.costExplorerClient)
 	assert.Equal(t, region, client.region)
-	assert.NotNil(t, client.rateLimiter)
+	assert.NotNil(t, client.newRateLimiter)
 }
 
 func TestGetRecommendations_EC2_Success(t *testing.T) {
@@ -262,9 +272,11 @@ func TestGetRecommendations_Error(t *testing.T) {
 		riError: newThrottleError(),
 	}
 
-	// Use custom rate limiter to speed up test
+	// Use custom rate limiter factory to speed up test
 	client := NewClientWithAPI(mockAPI, "us-east-1")
-	client.rateLimiter = NewRateLimiterWithOptions(1*time.Millisecond, 10*time.Millisecond, 2)
+	client.newRateLimiter = func() *RateLimiter {
+		return NewRateLimiterWithOptions(1*time.Millisecond, 10*time.Millisecond, 2)
+	}
 
 	params := common.RecommendationParams{
 		Service:        common.ServiceEC2,
@@ -361,7 +373,7 @@ func TestGetRecommendationsForService(t *testing.T) {
 // `PaymentOption` on each request and returns recs for that single
 // (term, payment) cell — so to let the user choose between every
 // variant in the UI we MUST issue one request per combo. The previous
-// behaviour hardcoded ("3yr", "partial-upfront") and the user-visible
+// behavior hardcoded ("3yr", "partial-upfront") and the user-visible
 // symptoms were "AWS recs only ever show Term = 3 Years" plus "no
 // all-upfront / no-upfront variants ever appear". We assert directly
 // against the captured input slice that all 6 (term, payment) combos
@@ -506,7 +518,9 @@ func TestGetRecommendations_ContextCancellation(t *testing.T) {
 	}
 
 	client := NewClientWithAPI(mockAPI, "us-east-1")
-	client.rateLimiter = NewRateLimiterWithOptions(100*time.Millisecond, 1*time.Second, 5)
+	client.newRateLimiter = func() *RateLimiter {
+		return NewRateLimiterWithOptions(100*time.Millisecond, 1*time.Second, 5)
+	}
 
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel() // Cancel immediately
@@ -522,7 +536,7 @@ func TestGetRecommendations_ContextCancellation(t *testing.T) {
 
 	// With the pagination loop added (issue #692), ctx.Err() is checked at
 	// the top of the first page iteration before the rate-limiter runs. A
-	// pre-cancelled context therefore returns context.Canceled directly, which
+	// pre-canceled context therefore returns context.Canceled directly, which
 	// is the correct behavior per feedback_ctx_cancel_terminal.md.
 	assert.Error(t, err)
 	assert.Nil(t, recs)
@@ -531,7 +545,7 @@ func TestGetRecommendations_ContextCancellation(t *testing.T) {
 
 // TestGetAllRecommendations_PropagatesContextCancellation pins the contract
 // that GetAllRecommendations propagates ctx.Err() to its caller after the
-// errgroup Wait() — the parent context being cancelled or its deadline
+// errgroup Wait() — the parent context being canceled or its deadline
 // exceeding must surface as an error rather than being swallowed by the
 // per-service error-isolation goroutines (which all return nil to the
 // errgroup so a single per-service failure does not cancel siblings).
@@ -551,8 +565,8 @@ func TestGetAllRecommendations_PropagatesContextCancellation(t *testing.T) {
 
 	// Cancel the context BEFORE the call so we don't depend on race-y
 	// timing inside the SDK clients. The Cost Explorer calls inside the
-	// goroutines observe the cancelled gctx (derived from ctx via
-	// errgroup.WithContext) and either short-circuit or return cancelled
+	// goroutines observe the canceled gctx (derived from ctx via
+	// errgroup.WithContext) and either short-circuit or return canceled
 	// errors; either way, our post-Wait ctx.Err() check returns
 	// context.Canceled.
 	ctx, cancel := context.WithCancel(context.Background())

providers/aws/recommendations/coverage.go

@@ -321,13 +321,13 @@ func serviceRegionFilter(service, region string) *types.Expression {
 // Mirrors fetchUtilizationPage so the two paths fail and back off the
 // same way.
 func (c *Client) fetchCoveragePage(ctx context.Context, input *costexplorer.GetReservationCoverageInput) (*costexplorer.GetReservationCoverageOutput, error) {
-	c.rateLimiter.Reset()
+	rl := c.newRateLimiter()
 	for {
-		if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil {
+		if waitErr := rl.Wait(ctx); waitErr != nil {
 			return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr)
 		}
 		result, err := c.costExplorerClient.GetReservationCoverage(ctx, input)
-		if !c.rateLimiter.ShouldRetry(err) {
+		if !rl.ShouldRetry(err) {
 			if err != nil {
 				return nil, fmt.Errorf("failed to get reservation coverage: %w", err)
 			}