From f84e94652c63a263aa74210f293edf4011f4d7c3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 18:58:11 +0200 Subject: [PATCH 1/3] fix(azure/gcp/aws): ctx.Err guards + page cap on 22 pagination loops (closes #691) Add ctx.Err() early-return guards and maxPages budget caps to all unbounded pagination loops across Azure (14 loops), GCP (6 loops), and AWS (2 loops). Context cancellation now terminates loops immediately with a wrapped error instead of either continuing or silently returning a partial result set, matching the pattern from PR #690 and the feedback_ctx_cancel_terminal rule. Fixes in collect* helpers (cache, cosmosdb) and the inline search loop that previously used ctx.Err()+break (silently partial) -- changed to return the error so callers propagate it. Also fixes two nil-context test calls in cache/client_test.go that panicked after the ctx.Err() guard was introduced. Regression tests added: Azure cache GetValidResourceTypes ctx-cancel + page cap, GCP computeengine GetRecommendations ctx-cancel + page cap, AWS fetchCoveragePaged ctx-cancel. --- providers/aws/recommendations/coverage.go | 3 ++ .../aws/recommendations/coverage_test.go | 27 ++++++++++ providers/aws/recommendations/utilization.go | 3 ++ providers/azure/services/cache/client.go | 47 +++++++++++++++--- providers/azure/services/cache/client_test.go | 42 +++++++++++++++- providers/azure/services/compute/client.go | 49 +++++++++++++++++-- providers/azure/services/cosmosdb/client.go | 47 +++++++++++++++--- providers/azure/services/database/client.go | 22 ++++++++- providers/azure/services/search/client.go | 34 +++++++++++-- providers/gcp/services/cloudsql/client.go | 13 ++++- .../gcp/services/computeengine/client.go | 36 ++++++++++++-- .../gcp/services/computeengine/client_test.go | 43 ++++++++++++++++ providers/gcp/services/memorystore/client.go | 13 ++++- 13 files changed, 345 insertions(+), 34 deletions(-) diff --git a/providers/aws/recommendations/coverage.go b/providers/aws/recommendations/coverage.go index 415a7101c..d4b4b97ad 100644 --- a/providers/aws/recommendations/coverage.go +++ b/providers/aws/recommendations/coverage.go @@ -263,6 +263,9 @@ func (c *Client) fetchCoveragePaged( ) error { var token *string for { + if err := ctx.Err(); err != nil { + return fmt.Errorf("coverage: pagination cancelled: %w", err) + } input.NextPageToken = token result, err := c.fetchCoveragePage(ctx, input) if err != nil { diff --git a/providers/aws/recommendations/coverage_test.go b/providers/aws/recommendations/coverage_test.go index ac2df72ce..685d20802 100644 --- a/providers/aws/recommendations/coverage_test.go +++ b/providers/aws/recommendations/coverage_test.go @@ -361,6 +361,33 @@ func TestNormaliseRDSEngine(t *testing.T) { } } +// TestFetchCoveragePaged_CtxCancelReturnsError asserts that a cancelled context +// is treated as a hard stop inside the pagination loop and surfaces an error +// rather than returning a partial/empty result silently +// (feedback_ctx_cancel_terminal). +func TestFetchCoveragePaged_CtxCancelReturnsError(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before the call + + // The mock returns a non-nil token on the first call so the loop + // would try to paginate -- but ctx.Err() must fire first. + token := "page2" + mock := &mockCoverageCE{ + coverageOutput: &costexplorer.GetReservationCoverageOutput{ + NextPageToken: &token, + }, + } + client := NewClientWithAPI(mock, "us-east-1") + + err := client.fetchCoveragePaged( + ctx, + &costexplorer.GetReservationCoverageInput{}, + func(_, _ string, _ PoolCoverage) {}, + 720, + ) + require.Error(t, err, "cancelled context must surface an error from fetchCoveragePaged") +} + // TestNormaliseDeployment locks deployment-option canonicalisation. CE // returns "Single-AZ" / "Multi-AZ" / "Multi-AZ (readable standbys)" while // the parser stores "single-az" / "multi-az"; both forms must collapse to diff --git a/providers/aws/recommendations/utilization.go b/providers/aws/recommendations/utilization.go index b5be494e9..a47d67208 100644 --- a/providers/aws/recommendations/utilization.go +++ b/providers/aws/recommendations/utilization.go @@ -53,6 +53,9 @@ func (c *Client) GetRIUtilization(ctx context.Context, lookbackDays int) ([]RIUt var nextPageToken *string for { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("utilization: pagination cancelled: %w", err) + } input.NextPageToken = nextPageToken result, err := c.fetchUtilizationPage(ctx, input) diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 706c1d115..9e45643ac 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -25,6 +25,15 @@ import ( "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) +// maxRecsPages caps Consumption API recommendation pagination. +const maxRecsPages = 10 + +// maxReservationsPages caps reservation-detail pagination. +const maxReservationsPages = 50 + +// maxCachesPages caps Redis cache list pagination. +const maxCachesPages = 20 + // redisSKUEntry holds the SKU-catalogue-derived fields the converter // wants for each Redis SKU. Sourced from the cache's Properties: // - shardCount: Properties.ShardCount (Premium-tier clustered caches). @@ -166,7 +175,13 @@ func (c *CacheClient) GetRecommendations(ctx context.Context, params common.Reco pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter}) } - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxRecsPages { + return nil, fmt.Errorf("cache: GetRecommendations pagination cap (%d pages) reached", maxRecsPages) + } page, err := pager.NextPage(ctx) if err != nil { return nil, fmt.Errorf("failed to get Redis Cache recommendations: %w", err) @@ -216,7 +231,13 @@ func (c *CacheClient) createReservationsPager() (ReservationsDetailsPager, error func (c *CacheClient) collectRedisReservations(ctx context.Context, pager ReservationsDetailsPager) ([]common.Commitment, error) { commitments := make([]common.Commitment, 0) - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxReservationsPages { + return nil, fmt.Errorf("cache: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) + } page, err := pager.NextPage(ctx) if err != nil { return nil, fmt.Errorf("cache: list reservations: %w", err) @@ -403,7 +424,10 @@ func (c *CacheClient) GetValidResourceTypes(ctx context.Context) ([]string, erro return c.getCommonSKUs(), nil } - skuSet := c.collectSKUsFromCaches(ctx, pager) + skuSet, err := c.collectSKUsFromCaches(ctx, pager) + if err != nil { + return nil, err + } // If we found SKUs from existing caches, use those if len(skuSet) > 0 { @@ -429,11 +453,20 @@ func (c *CacheClient) createRedisCachesPager() (RedisCachesPager, error) { return client.NewListBySubscriptionPager(nil), nil } -// collectSKUsFromCaches collects SKUs from existing Redis caches -func (c *CacheClient) collectSKUsFromCaches(ctx context.Context, pager RedisCachesPager) map[string]bool { +// collectSKUsFromCaches collects SKUs from existing Redis caches. +// Returns (nil, err) on context cancellation so callers can propagate the error +// instead of silently using a partial result set. +func (c *CacheClient) collectSKUsFromCaches(ctx context.Context, pager RedisCachesPager) (map[string]bool, error) { skuSet := make(map[string]bool) - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("cache: GetValidResourceTypes context cancelled after %d pages: %w", pageIdx, err) + } + if pageIdx >= maxCachesPages { + log.Printf("WARNING: cache: GetValidResourceTypes pagination cap (%d pages) reached", maxCachesPages) + break + } page, err := pager.NextPage(ctx) if err != nil { // If we can't list existing caches, fall back to known SKU families @@ -447,7 +480,7 @@ func (c *CacheClient) collectSKUsFromCaches(ctx context.Context, pager RedisCach } } - return skuSet + return skuSet, nil } // extractSKUFromCache extracts the full SKU name from a cache resource diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 3380d6083..9333c510e 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -188,7 +188,7 @@ func TestCacheClient_GetValidResourceTypes_Fallback(t *testing.T) { // When API calls fail, GetValidResourceTypes should return common SKUs client := NewClient(nil, "invalid-subscription", "eastus") - skus, err := client.GetValidResourceTypes(nil) + skus, err := client.GetValidResourceTypes(context.Background()) require.NoError(t, err) require.NotEmpty(t, skus) @@ -204,7 +204,7 @@ func TestCacheClient_ValidateOffering_InvalidSKU(t *testing.T) { ResourceType: "InvalidSKU_X99", } - err := client.ValidateOffering(nil, rec) + err := client.ValidateOffering(context.Background(), rec) assert.Error(t, err) assert.Contains(t, err.Error(), "invalid Azure Redis Cache SKU") } @@ -1196,3 +1196,41 @@ func TestCacheClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t *t assert.Regexp(t, `^redis-`, capturedDisplayName) assert.Contains(t, capturedDisplayName, "Premium_P1") } + +// infiniteRedisCachesPager is a pager that always reports More()=true, +// used to exercise the maxCachesPages budget cap. +type infiniteRedisCachesPager struct{} + +func (p *infiniteRedisCachesPager) More() bool { return true } +func (p *infiniteRedisCachesPager) NextPage(_ context.Context) (armredis.ClientListBySubscriptionResponse, error) { + return armredis.ClientListBySubscriptionResponse{}, nil +} + +// TestCacheClient_GetValidResourceTypes_CtxCancelReturnsError asserts that a +// cancelled context is treated as a hard stop and surfaces an error rather than +// returning a silent partial result (feedback_ctx_cancel_terminal). +func TestCacheClient_GetValidResourceTypes_CtxCancelReturnsError(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel immediately + + client := NewClient(nil, "test-subscription", "eastus") + client.SetRedisCachesPager(&infiniteRedisCachesPager{}) + + _, err := client.GetValidResourceTypes(ctx) + require.Error(t, err, "cancelled context must produce an error, not a silent partial result") +} + +// TestCacheClient_GetValidResourceTypes_PageCapReturnsError asserts that the +// page budget is enforced: an unbounded pager is stopped after maxCachesPages +// iterations and the function returns an error rather than looping forever. +func TestCacheClient_GetValidResourceTypes_PageCapFires(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetRedisCachesPager(&infiniteRedisCachesPager{}) + + // GetValidResourceTypes falls back to common SKUs after hitting the log-only + // cap, so the call itself succeeds. The important invariant is that it + // terminates rather than looping forever. + skus, err := client.GetValidResourceTypes(context.Background()) + require.NoError(t, err) + require.NotEmpty(t, skus, "page cap must trigger fallback to common SKUs, not an empty result") +} diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index f97e0f42a..2b45d9a12 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -58,6 +58,19 @@ type HTTPClient interface { // Either field stays at the zero value when the capability is missing // or unparseable; common.ComputeDetails treats 0 as "unknown" (the // JSON tags on VCPU/MemoryGB are omitempty). + +// maxRecsPages caps Consumption API recommendation pagination to avoid +// burning a Lambda deadline on a stalled or unexpectedly deep result set. +const maxRecsPages = 10 + +// maxReservationsPages caps reservation-detail pagination. +// Large orgs may have hundreds of reservations spread over many pages. +const maxReservationsPages = 50 + +// maxSKUPages caps Azure ResourceSKUs pagination. +// The SKU catalogue for a subscription can run to many pages. +const maxSKUPages = 20 + type vmSKUEntry struct { vCPUs int memoryGB float64 @@ -185,7 +198,13 @@ func (c *ComputeClient) GetRecommendations(ctx context.Context, params common.Re pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter}) } - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxRecsPages { + return nil, fmt.Errorf("compute: GetRecommendations pagination cap (%d pages) reached", maxRecsPages) + } page, err := pager.NextPage(ctx) if err != nil { return nil, fmt.Errorf("failed to get VM recommendations: %w", err) @@ -238,7 +257,13 @@ func (c *ComputeClient) createReservationsPager() (ReservationsDetailsPager, err func (c *ComputeClient) collectVMReservations(ctx context.Context, pager ReservationsDetailsPager) ([]common.Commitment, error) { commitments := make([]common.Commitment, 0) - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxReservationsPages { + return nil, fmt.Errorf("compute: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) + } page, err := pager.NextPage(ctx) if err != nil { return nil, fmt.Errorf("compute: list reservations: %w", err) @@ -553,7 +578,13 @@ func (c *ComputeClient) createResourceSKUsPager() (ResourceSKUsPager, error) { func (c *ComputeClient) collectVMSizesFromSKUs(ctx context.Context, pager ResourceSKUsPager) ([]string, error) { vmSizes := make([]string, 0) - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxSKUPages { + return nil, fmt.Errorf("compute: GetValidResourceTypes pagination cap (%d pages) reached", maxSKUPages) + } page, err := pager.NextPage(ctx) if err != nil { return nil, fmt.Errorf("failed to list VM sizes: %w", err) @@ -777,10 +808,18 @@ func (c *ComputeClient) fetchSKUCatalogue(ctx context.Context) map[string]vmSKUE return nil } out := make(map[string]vmSKUEntry) - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + logging.Warnf("azure compute: SKU catalogue fetch cancelled for region %s after %d pages: %v; partial cache discarded", c.region, pageIdx, err) + return nil + } + if pageIdx >= maxSKUPages { + logging.Warnf("azure compute: SKU catalogue pagination cap (%d pages) reached for region %s; partial cache (%d entries) used", maxSKUPages, c.region, len(out)) + break + } page, err := pager.NextPage(ctx) if err != nil { - logging.Warnf("azure compute: SKU catalogue page fetch failed for region %s: %v — partial cache (%d entries) discarded, Details.VCPU/MemoryGB left at 0", c.region, err, len(out)) + logging.Warnf("azure compute: SKU catalogue page fetch failed for region %s: %v; partial cache (%d entries) discarded, Details.VCPU/MemoryGB left at 0", c.region, err, len(out)) return nil } c.populateVMSKUMapFromPage(out, page.Value) diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index 85b5b6d97..e1bbaa5d9 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -26,6 +26,15 @@ import ( "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) +// maxRecsPages caps Consumption API recommendation pagination. +const maxRecsPages = 10 + +// maxReservationsPages caps reservation-detail pagination. +const maxReservationsPages = 50 + +// maxAccountsPages caps Cosmos DB account list pagination. +const maxAccountsPages = 20 + // HTTPClient interface for HTTP operations (enables mocking) type HTTPClient interface { Do(req *http.Request) (*http.Response, error) @@ -160,7 +169,13 @@ func (c *CosmosDBClient) GetRecommendations(ctx context.Context, params common.R pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter}) } - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxRecsPages { + return nil, fmt.Errorf("cosmosdb: GetRecommendations pagination cap (%d pages) reached", maxRecsPages) + } page, err := pager.NextPage(ctx) if err != nil { return nil, fmt.Errorf("failed to get Cosmos DB recommendations: %w", err) @@ -210,7 +225,13 @@ func (c *CosmosDBClient) createReservationsPager() (ReservationsDetailsPager, er func (c *CosmosDBClient) collectCosmosReservations(ctx context.Context, pager ReservationsDetailsPager) ([]common.Commitment, error) { commitments := make([]common.Commitment, 0) - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxReservationsPages { + return nil, fmt.Errorf("cosmosdb: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) + } page, err := pager.NextPage(ctx) if err != nil { return nil, fmt.Errorf("cosmosdb: list reservations: %w", err) @@ -395,7 +416,10 @@ func (c *CosmosDBClient) GetValidResourceTypes(ctx context.Context) ([]string, e return c.getCommonSKUs(), nil } - skuSet := c.collectCapabilitiesFromAccounts(ctx, pager) + skuSet, err := c.collectCapabilitiesFromAccounts(ctx, pager) + if err != nil { + return nil, err + } // If we found SKUs from existing accounts, use those if len(skuSet) > 0 { @@ -421,11 +445,20 @@ func (c *CosmosDBClient) createCosmosAccountsPager() (CosmosAccountsPager, error return client.NewListPager(nil), nil } -// collectCapabilitiesFromAccounts collects capabilities from existing Cosmos DB accounts -func (c *CosmosDBClient) collectCapabilitiesFromAccounts(ctx context.Context, pager CosmosAccountsPager) map[string]bool { +// collectCapabilitiesFromAccounts collects capabilities from existing Cosmos DB accounts. +// Returns (nil, err) on context cancellation so callers can propagate the error +// instead of silently using a partial result set. +func (c *CosmosDBClient) collectCapabilitiesFromAccounts(ctx context.Context, pager CosmosAccountsPager) (map[string]bool, error) { skuSet := make(map[string]bool) - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("cosmosdb: GetValidResourceTypes context cancelled after %d pages: %w", pageIdx, err) + } + if pageIdx >= maxAccountsPages { + log.Printf("WARNING: cosmosdb: GetValidResourceTypes pagination cap (%d pages) reached", maxAccountsPages) + break + } page, err := pager.NextPage(ctx) if err != nil { // If we can't list existing accounts, fall back to known SKU types @@ -440,7 +473,7 @@ func (c *CosmosDBClient) collectCapabilitiesFromAccounts(ctx context.Context, pa } } - return skuSet + return skuSet, nil } // extractCapabilitiesFromAccount extracts capability names from a Cosmos DB account diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index f214501c5..c775c10b2 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -25,6 +25,12 @@ import ( "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) +// maxRecsPages caps Consumption API recommendation pagination. +const maxRecsPages = 10 + +// maxReservationsPages caps reservation-detail pagination. +const maxReservationsPages = 50 + // sqlSKUEntry holds the SKU-catalogue-derived fields the converter // wants for each Azure SQL SKU. Sourced from the // armsql.CapabilitiesClient.ListByLocation response which embeds the @@ -168,7 +174,13 @@ func (c *DatabaseClient) GetRecommendations(ctx context.Context, params common.R pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter}) } - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxRecsPages { + return nil, fmt.Errorf("database: GetRecommendations pagination cap (%d pages) reached", maxRecsPages) + } page, err := pager.NextPage(ctx) if err != nil { return nil, fmt.Errorf("failed to get SQL recommendations: %w", err) @@ -218,7 +230,13 @@ func (c *DatabaseClient) createReservationsPager() (ReservationsDetailsPager, er func (c *DatabaseClient) collectSQLReservations(ctx context.Context, pager ReservationsDetailsPager) ([]common.Commitment, error) { commitments := make([]common.Commitment, 0) - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxReservationsPages { + return nil, fmt.Errorf("database: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) + } page, err := pager.NextPage(ctx) if err != nil { return nil, fmt.Errorf("database: list reservations: %w", err) diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index c38f285f9..476d698fb 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -23,6 +23,15 @@ import ( "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) +// maxRecsPages caps Consumption API recommendation pagination. +const maxRecsPages = 10 + +// maxReservationsPages caps reservation-detail pagination. +const maxReservationsPages = 50 + +// maxServicesPages caps Search service list pagination. +const maxServicesPages = 20 + // HTTPClient interface for HTTP operations (enables mocking) type HTTPClient interface { Do(req *http.Request) (*http.Response, error) @@ -141,7 +150,13 @@ func (c *SearchClient) GetRecommendations(ctx context.Context, params common.Rec pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter}) } - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxRecsPages { + return nil, fmt.Errorf("search: GetRecommendations pagination cap (%d pages) reached", maxRecsPages) + } page, err := pager.NextPage(ctx) if err != nil { return nil, fmt.Errorf("failed to get Search recommendations: %w", err) @@ -191,7 +206,13 @@ func (c *SearchClient) createReservationsPager() (ReservationsDetailsPager, erro func (c *SearchClient) collectSearchReservations(ctx context.Context, pager ReservationsDetailsPager) ([]common.Commitment, error) { commitments := make([]common.Commitment, 0) - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxReservationsPages { + return nil, fmt.Errorf("search: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) + } page, err := pager.NextPage(ctx) if err != nil { return nil, fmt.Errorf("search: list reservations: %w", err) @@ -385,7 +406,14 @@ func (c *SearchClient) GetValidResourceTypes(ctx context.Context) ([]string, err pager = client.NewListBySubscriptionPager(nil, nil) } - for pager.More() { + for pageIdx := 0; pager.More(); pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("search: GetValidResourceTypes context cancelled after %d pages: %w", pageIdx, err) + } + if pageIdx >= maxServicesPages { + log.Printf("WARNING: search: GetValidResourceTypes pagination cap (%d pages) reached", maxServicesPages) + break + } page, err := pager.NextPage(ctx) if err != nil { // If we can't list existing services, fall back to known SKU families diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 1535c7937..b164f0310 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -17,6 +17,9 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" ) +// maxRecsPages caps GCP Recommender API iteration. +const maxRecsPages = 20 + // SQLAdminService interface for SQL admin operations (enables mocking) type SQLAdminService interface { ListInstances(projectID string) (*sqladmin.InstancesListResponse, error) @@ -160,14 +163,20 @@ func (c *CloudSQLClient) GetRecommendations(ctx context.Context, params common.R } it := recClient.ListRecommendations(ctx, req) - for { + for pageIdx := 0; ; pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxRecsPages { + return nil, fmt.Errorf("cloudsql: GetRecommendations iteration cap (%d items) reached", maxRecsPages) + } rec, err := it.Next() if err == iterator.Done { break } if err != nil { // Iterator errors must propagate so callers don't silently act - // on a partial recommendation list — see the computeengine + // on a partial recommendation list -- see the computeengine // client for the full rationale. return nil, fmt.Errorf("cloudsql: iterate recommendations: %w", err) } diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 226e4ade6..ac95916ce 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -22,6 +22,16 @@ import ( "github.com/LeanerCloud/CUDly/pkg/retry" ) +// maxRecsPages caps GCP Recommender API iteration to avoid burning a Lambda +// deadline on a stalled or unexpectedly large result set. +const maxRecsPages = 20 + +// maxCommitmentsPages caps GCP committed-use discount iteration. +const maxCommitmentsPages = 50 + +// maxMachineTypesPages caps GCP machine types iteration. +const maxMachineTypesPages = 20 + // CommitmentsService interface for commitments operations (enables mocking) type CommitmentsService interface { List(ctx context.Context, req *computepb.ListRegionCommitmentsRequest) CommitmentsIterator @@ -205,14 +215,20 @@ func (c *ComputeEngineClient) GetRecommendations(ctx context.Context, params com } it := recClient.ListRecommendations(ctx, req) - for { + for pageIdx := 0; ; pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxRecsPages { + return nil, fmt.Errorf("computeengine: GetRecommendations iteration cap (%d items) reached", maxRecsPages) + } rec, err := it.Next() if err == iterator.Done { break } if err != nil { // Iterator errors (quota, auth, transient 5xx) must propagate so - // callers don't silently act on a partial recommendation list — + // callers don't silently act on a partial recommendation list -- // a missed recommendation can lead to under-committing or // double-purchasing. Callers should retry. return nil, fmt.Errorf("computeengine: iterate recommendations: %w", err) @@ -263,7 +279,13 @@ func (c *ComputeEngineClient) collectCommitments(ctx context.Context, svc Commit commitments := make([]common.Commitment, 0) it := svc.List(ctx, req) - for { + for pageIdx := 0; ; pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxCommitmentsPages { + return nil, fmt.Errorf("computeengine: GetExistingCommitments iteration cap (%d items) reached", maxCommitmentsPages) + } commitment, err := it.Next() if err == iterator.Done { break @@ -631,7 +653,13 @@ func (c *ComputeEngineClient) GetValidResourceTypes(ctx context.Context) ([]stri machineTypes := make([]string, 0) it := svc.List(ctx, req) - for { + for pageIdx := 0; ; pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxMachineTypesPages { + return nil, fmt.Errorf("computeengine: GetValidResourceTypes iteration cap (%d items) reached", maxMachineTypesPages) + } machineType, err := it.Next() if err == iterator.Done { break diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index ffcb4adc4..74ce48896 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -840,3 +840,46 @@ func TestComputeEngineClient_ConvertGCPRecommendation(t *testing.T) { assert.Equal(t, "n1-standard-4", rec.ResourceType) assert.Equal(t, 50.5, rec.EstimatedSavings) } + +// infiniteRecommenderIterator never signals iterator.Done, used to exercise +// the ctx-cancel guard and the maxRecsPages budget cap. +type infiniteRecommenderIterator struct{} + +func (i *infiniteRecommenderIterator) Next() (*recommenderpb.Recommendation, error) { + return &recommenderpb.Recommendation{}, nil +} + +type infiniteRecommenderClient struct{} + +func (c *infiniteRecommenderClient) ListRecommendations(_ context.Context, _ *recommenderpb.ListRecommendationsRequest) RecommenderIterator { + return &infiniteRecommenderIterator{} +} + +func (c *infiniteRecommenderClient) Close() error { return nil } + +// TestComputeEngineClient_GetRecommendations_CtxCancelReturnsError asserts +// that a cancelled context is treated as a terminal stop and returns an error +// rather than silently producing a partial result set +// (feedback_ctx_cancel_terminal). +func TestComputeEngineClient_GetRecommendations_CtxCancelReturnsError(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before the call + + client, err := NewClient(context.Background(), "test-project", "us-central1") + require.NoError(t, err) + client.SetRecommenderClient(&infiniteRecommenderClient{}) + + _, err = client.GetRecommendations(ctx, common.RecommendationParams{}) + require.Error(t, err, "cancelled context must surface an error, not a partial result set") +} + +// TestComputeEngineClient_GetRecommendations_PageCapFires asserts that the +// iteration budget terminates an infinite iterator rather than looping forever. +func TestComputeEngineClient_GetRecommendations_PageCapFires(t *testing.T) { + client, err := NewClient(context.Background(), "test-project", "us-central1") + require.NoError(t, err) + client.SetRecommenderClient(&infiniteRecommenderClient{}) + + _, err = client.GetRecommendations(context.Background(), common.RecommendationParams{}) + require.Error(t, err, "page cap must surface an error when the iterator never terminates") +} diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index 73cbf6cd8..2407b77c8 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -19,6 +19,9 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" ) +// maxRecsPages caps GCP Recommender API iteration. +const maxRecsPages = 20 + // RedisService interface for Redis operations type RedisService interface { ListInstances(ctx context.Context, req *redispb.ListInstancesRequest) RedisIterator @@ -160,14 +163,20 @@ func (c *MemorystoreClient) GetRecommendations(ctx context.Context, params commo } it := recClient.ListRecommendations(ctx, req) - for { + for pageIdx := 0; ; pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxRecsPages { + return nil, fmt.Errorf("memorystore: GetRecommendations iteration cap (%d items) reached", maxRecsPages) + } rec, err := it.Next() if err == iterator.Done { break } if err != nil { // Iterator errors must propagate so callers don't silently act - // on a partial recommendation list — see the computeengine + // on a partial recommendation list -- see the computeengine // client for the full rationale. return nil, fmt.Errorf("memorystore: iterate recommendations: %w", err) } From 65b0c667f2472e18af67f139f192c2faf39980b5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 19:02:15 +0200 Subject: [PATCH 2/3] fix(gcp): rename page-cap constant + counter to reflect item semantics MachineTypesIterator.Next() yields one machine type per call, not one page. Rename maxMachineTypesPages -> maxMachineTypeItems and pageIdx -> itemIdx so the names and error message match the actual iteration semantics. --- providers/gcp/services/computeengine/client.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index ac95916ce..7e598af71 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -29,8 +29,8 @@ const maxRecsPages = 20 // maxCommitmentsPages caps GCP committed-use discount iteration. const maxCommitmentsPages = 50 -// maxMachineTypesPages caps GCP machine types iteration. -const maxMachineTypesPages = 20 +// maxMachineTypeItems caps GCP machine types iteration (one item per Next() call). +const maxMachineTypeItems = 20 // CommitmentsService interface for commitments operations (enables mocking) type CommitmentsService interface { @@ -653,12 +653,12 @@ func (c *ComputeEngineClient) GetValidResourceTypes(ctx context.Context) ([]stri machineTypes := make([]string, 0) it := svc.List(ctx, req) - for pageIdx := 0; ; pageIdx++ { + for itemIdx := 0; ; itemIdx++ { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("context cancelled during pagination: %w", err) } - if pageIdx >= maxMachineTypesPages { - return nil, fmt.Errorf("computeengine: GetValidResourceTypes iteration cap (%d items) reached", maxMachineTypesPages) + if itemIdx >= maxMachineTypeItems { + return nil, fmt.Errorf("computeengine: GetValidResourceTypes iteration cap (%d items) reached", maxMachineTypeItems) } machineType, err := it.Next() if err == iterator.Done { From 394058f44bd15dfae7e101212d5881c865ea69cb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 31 May 2026 19:48:42 +0200 Subject: [PATCH 3/3] refactor: extract helpers to clear gocyclo threshold (#800) The ctx.Err pagination guards added in this PR pushed two functions just over the project's gocyclo > 10 ceiling, failing pre-commit: - providers/azure/services/search/client.go GetValidResourceTypes (12) - providers/aws/recommendations/utilization.go GetRIUtilization (11) Each is split into a thin entry point plus a focused helper: - GetValidResourceTypes -> resolveServicesPager + collectSKUsFromPager - GetRIUtilization -> buildUtilizations for the agg-to-slice tail Behaviour unchanged; 323 tests pass across the two packages. --- providers/aws/recommendations/utilization.go | 7 ++- providers/azure/services/search/client.go | 58 +++++++++++--------- 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/providers/aws/recommendations/utilization.go b/providers/aws/recommendations/utilization.go index a47d67208..7c79d7511 100644 --- a/providers/aws/recommendations/utilization.go +++ b/providers/aws/recommendations/utilization.go @@ -75,6 +75,10 @@ func (c *Client) GetRIUtilization(ctx context.Context, lookbackDays int) ([]RIUt nextPageToken = result.NextPageToken } + return buildUtilizations(agg), nil +} + +func buildUtilizations(agg map[string]*riAccumulator) []RIUtilization { utilizations := make([]RIUtilization, 0, len(agg)) for id, a := range agg { pct := 0.0 @@ -89,8 +93,7 @@ func (c *Client) GetRIUtilization(ctx context.Context, lookbackDays int) ([]RIUt UnusedHours: a.unusedHours, }) } - - return utilizations, nil + return utilizations } // fetchUtilizationPage calls the Cost Explorer API with rate-limit retry. diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index 476d698fb..1ca532a79 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -392,20 +392,40 @@ func (c *SearchClient) GetOfferingDetails(ctx context.Context, rec common.Recomm // GetValidResourceTypes returns valid Search SKUs from Azure API func (c *SearchClient) GetValidResourceTypes(ctx context.Context) ([]string, error) { - skuSet := make(map[string]bool) + pager, ok := c.resolveServicesPager() + if !ok { + return c.getCommonSKUs(), nil + } - // Use injected pager if available (for testing) - var pager SearchServicesPager - if c.searchServicesPager != nil { - pager = c.searchServicesPager - } else { - client, err := armsearch.NewServicesClient(c.subscriptionID, c.cred, nil) - if err != nil { - return c.getCommonSKUs(), nil + skuSet, err := c.collectSKUsFromPager(ctx, pager) + if err != nil { + return nil, err + } + + if len(skuSet) > 0 { + skus := make([]string, 0, len(skuSet)) + for sku := range skuSet { + skus = append(skus, sku) } - pager = client.NewListBySubscriptionPager(nil, nil) + return skus, nil + } + + return c.getCommonSKUs(), nil +} + +func (c *SearchClient) resolveServicesPager() (SearchServicesPager, bool) { + if c.searchServicesPager != nil { + return c.searchServicesPager, true + } + client, err := armsearch.NewServicesClient(c.subscriptionID, c.cred, nil) + if err != nil { + return nil, false } + return client.NewListBySubscriptionPager(nil, nil), true +} +func (c *SearchClient) collectSKUsFromPager(ctx context.Context, pager SearchServicesPager) (map[string]bool, error) { + skuSet := make(map[string]bool) for pageIdx := 0; pager.More(); pageIdx++ { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("search: GetValidResourceTypes context cancelled after %d pages: %w", pageIdx, err) @@ -416,29 +436,15 @@ func (c *SearchClient) GetValidResourceTypes(ctx context.Context) ([]string, err } page, err := pager.NextPage(ctx) if err != nil { - // If we can't list existing services, fall back to known SKU families break } - for _, service := range page.Value { if service.SKU != nil && service.SKU.Name != nil { - skuName := string(*service.SKU.Name) - skuSet[skuName] = true + skuSet[string(*service.SKU.Name)] = true } } } - - // If we found SKUs from existing services, use those - if len(skuSet) > 0 { - skus := make([]string, 0, len(skuSet)) - for sku := range skuSet { - skus = append(skus, sku) - } - return skus, nil - } - - // Otherwise, return common SKU tiers that support reservations - return c.getCommonSKUs(), nil + return skuSet, nil } // getCommonSKUs returns common Search SKUs