diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 67230a806..2bc6fd040 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -1015,6 +1015,32 @@ func TestFanOutPerAccount_PartialSuccess(t *testing.T) { "LastErr should reflect the failed account's error") } +// TestFanOutPerAccount_FailedCollectionNotInSucceededAccountIDs is the +// scheduler-side COR-03 regression test: an account whose collection fn +// errors (the shape Azure mergeServiceResults / GCP mergeRegionResults now +// produce when every service call failed) must NOT land in +// SucceededAccountIDs. UpsertRecommendations evicts stale rows only for +// accounts listed there, so this is what preserves the previously collected +// rows of an account hit by a transient full-provider failure. +func TestFanOutPerAccount_FailedCollectionNotInSucceededAccountIDs(t *testing.T) { + accounts := []config.CloudAccount{ + {ID: "acct-ok", Name: "acct-ok", ExternalID: "e-ok"}, + {ID: "acct-all-services-failed", Name: "acct-all-services-failed", ExternalID: "e-bad"}, + } + fn := func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, error) { + if acct.ID == "acct-all-services-failed" { + return nil, errors.New("all 6 Azure recommendation services failed: 429 too many requests") + } + return []config.RecommendationRecord{{ID: acct.ID, Provider: "azure"}}, nil + } + + _, outcome := fanOutPerAccount(context.Background(), "Azure", accounts, fn) + assert.Equal(t, []string{"acct-ok"}, outcome.SucceededAccountIDs, + "a failed collection must not be eligible for stale-row eviction") + assert.Equal(t, 1, outcome.FailedCount) + assert.Contains(t, outcome.LastErr, "all 6 Azure recommendation services failed") +} + // TestFanOutPerAccount_ZeroAccounts: empty input → empty outcome, // no errors. The caller's "FailedCount == len(accounts) > 0" guard // correctly skips the all-failed error path. @@ -1447,8 +1473,19 @@ func TestScheduler_CollectAWSRecommendations_GetRecsError(t *testing.T) { assert.Nil(t, recs) } -// Test successful Azure recommendations. -func TestScheduler_CollectAzureRecommendations_Success(t *testing.T) { +// TestScheduler_CollectAzureRecommendations_AllAccountsFailLoud pins the +// COR-03 contract end-to-end on the Azure path: when the only registered +// Azure account has unusable managed_identity credentials in the test +// environment (no IMDS, no federated token), every per-service call inside +// mergeServiceResults errors, the all-attempted-failed guard fires, the +// per-account call returns an error, fanOutPerAccount marks the account as +// failed, and collectAzureRecommendations propagates an "all accounts +// failed" error to the caller instead of returning (empty, nil). +// +// Pre-fix this test asserted require.NoError and validated the silent-skip +// behavior the COR-03 fix removes. Keeping that assertion would have +// regressed the very property this PR is trying to enforce. +func TestScheduler_CollectAzureRecommendations_AllAccountsFailLoud(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -1469,17 +1506,17 @@ func TestScheduler_CollectAzureRecommendations_Success(t *testing.T) { } mockStore.On("ListCloudAccounts", mock.Anything, mock.Anything).Return(azureAccounts, nil) - // The managed_identity path will try DefaultAzureCredential which will - // fail in tests, so we expect an error log but no crash. scheduler := &Scheduler{ config: mockStore, } - recs, _, err := scheduler.collectAzureRecommendations(ctx, globalCfg) - require.NoError(t, err) - // In test environment without Azure credentials, 0 recommendations is expected - // (the error is logged and skipped). The test validates the per-account loop runs. - _ = recs + recs, succeeded, err := scheduler.collectAzureRecommendations(ctx, globalCfg) + require.Error(t, err, "collectAzureRecommendations must fail loud when the only account has unusable credentials (COR-03)") + assert.Contains(t, err.Error(), "Azure: all 1 accounts failed", + "the per-account fan-out must surface the all-accounts-failed shape so the scheduler keeps stale rows and records last_collection_error") + assert.Nil(t, recs) + assert.Empty(t, succeeded, + "the failed account must not land in SucceededAccountIDs (stale-row eviction guard)") } // Test GCP recommendations with no accounts — should skip gracefully. diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 8fe3719cd..4bc68b03b 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -97,8 +97,17 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p }) } + // Record which services the params filter lets through so the merge can + // distinguish "skipped by filter" from "attempted and succeeded with zero + // recommendations" when applying the all-attempted-failed guard. + includeCompute := shouldIncludeService(params, common.ServiceCompute) + includeDB := shouldIncludeService(params, common.ServiceRelationalDB) + includeCache := shouldIncludeService(params, common.ServiceCache) + includeCosmos := shouldIncludeService(params, common.ServiceNoSQL) + includeSP := shouldIncludeService(params, common.ServiceSavingsPlans) + // Compute (VM) recommendations — subscription-wide. - if shouldIncludeService(params, common.ServiceCompute) { + if includeCompute { goService(&computeErr, func() { computeClient := compute.NewClient(r.cred, r.subscriptionID, "") computeRecs, computeErr = computeClient.GetRecommendations(gctx, params) @@ -106,7 +115,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p } // Database (SQL) recommendations — subscription-wide. - if shouldIncludeService(params, common.ServiceRelationalDB) { + if includeDB { goService(&dbErr, func() { dbClient := database.NewClient(r.cred, r.subscriptionID, "") dbRecs, dbErr = dbClient.GetRecommendations(gctx, params) @@ -114,7 +123,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p } // Cache (Redis) recommendations — subscription-wide. - if shouldIncludeService(params, common.ServiceCache) { + if includeCache { goService(&cacheErr, func() { cacheClient := cache.NewClient(r.cred, r.subscriptionID, "") cacheRecs, cacheErr = cacheClient.GetRecommendations(gctx, params) @@ -122,7 +131,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p } // CosmosDB (NoSQL) recommendations — subscription-wide. - if shouldIncludeService(params, common.ServiceNoSQL) { + if includeCosmos { goService(&cosmosErr, func() { cosmosClient := cosmosdb.NewClient(r.cred, r.subscriptionID, "") cosmosRecs, cosmosErr = cosmosClient.GetRecommendations(gctx, params) @@ -132,9 +141,9 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // Savings Plans — Azure has no stable public API for SP purchase // recommendations (Benefits Recommendations API is still in preview). // The call returns an empty slice so the service appears in the fan-out - // and will start returning data once the API stabilises without requiring + // and will start returning data once the API stabilizes without requiring // a scheduler change. - if shouldIncludeService(params, common.ServiceSavingsPlans) { + if includeSP { goService(&spErr, func() { spClient := savingsplans.NewClient(r.cred, r.subscriptionID, "") spRecs, spErr = spClient.GetRecommendations(gctx, params) @@ -160,12 +169,28 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p return nil, err } - return mergeServiceResults(serviceResult{"compute", computeRecs, computeErr}, - serviceResult{"database", dbRecs, dbErr}, - serviceResult{"cache", cacheRecs, cacheErr}, - serviceResult{"cosmosdb", cosmosRecs, cosmosErr}, - serviceResult{"savingsplans", spRecs, spErr}, - serviceResult{"advisor", advisorRecs, advisorErr}), nil + return mergeServiceResults(serviceResult{"compute", computeRecs, computeErr, includeCompute}, + serviceResult{"database", dbRecs, dbErr, includeDB}, + serviceResult{"cache", cacheRecs, cacheErr, includeCache}, + serviceResult{"cosmosdb", cosmosRecs, cosmosErr, includeCosmos}, + // The savingsplans client is a stub that unconditionally returns + // ([], nil) until the Benefits Recommendations API stabilizes (see + // services/savingsplans Client.GetRecommendations). Counting its + // built-in success as an attempted service would keep the + // all-attempted-failed guard from ever firing on a total provider + // failure (expired credential, subscription-wide throttle), so it is + // excluded from the guard until it makes real API calls. + serviceResult{"savingsplans", spRecs, spErr, false}, + // The Advisor client is excluded from the all-attempted-failed guard + // for the same reason: getAdvisorRecommendations swallows pagination + // errors (the auth failure from an expired credential surfaces as a + // 401 during pager.NextPage, not during client construction) and + // always returns (recs, nil). Counting its unconditional success as + // an attempted service would keep the guard from firing on a total + // credential failure -- the same hazard the savingsplans exclusion + // prevents. When getAdvisorRecommendations is changed to propagate + // hard errors, flip this flag back to true. + serviceResult{"advisor", advisorRecs, advisorErr, false}) } // serviceResult bundles a per-service collection outcome for the deterministic @@ -176,6 +201,12 @@ type serviceResult struct { name string recs []common.Recommendation err error + // attempted records whether the service call was actually launched + // (i.e. not skipped by the params service filter). Skipped services + // carry nil recs and nil err, which is indistinguishable from a + // successful zero-recommendation call, so the all-attempted-failed + // guard below needs this flag to avoid counting skips as successes. + attempted bool } // mergeServiceResults logs per-service errors (matches the previous sequential @@ -184,10 +215,31 @@ type serviceResult struct { // the canonical compute → database → cache → cosmosdb → savingsplans → advisor // order so that order-sensitive consumers remain stable. The advisor entry's // error is logged via logging.Errorf to match the pre-parallelisation severity. -func mergeServiceResults(results ...serviceResult) []common.Recommendation { +// +// Partial failure is tolerated: as long as at least one attempted service +// succeeded, the successful services' recommendations are returned with a nil +// error. But when EVERY attempted service errored (e.g. an expired federated +// credential or a subscription-wide throttle), the merge returns a wrapped +// error instead of an empty-but-nil-error result, porting the AWS 08-H4 guard +// from providers/aws/recommendations/client.go. Returning (recs, nil) on a +// total failure makes a broken run indistinguishable from "no savings +// available": the scheduler would count the account as succeeded, evict its +// previously collected rows, and clear last_collection_error (COR-03). +func mergeServiceResults(results ...serviceResult) ([]common.Recommendation, error) { total := 0 + attempted := 0 + failures := 0 + var lastErr error for _, r := range results { total += len(r.recs) + if !r.attempted { + continue + } + attempted++ + if r.err != nil { + failures++ + lastErr = r.err + } } out := make([]common.Recommendation, 0, total) for _, r := range results { @@ -201,7 +253,10 @@ func mergeServiceResults(results ...serviceResult) []common.Recommendation { } out = append(out, r.recs...) } - return out + if failures > 0 && failures == attempted { + return nil, fmt.Errorf("all %d Azure recommendation services failed: %w", failures, lastErr) + } + return out, nil } // GetRecommendationsForService retrieves Azure reservation recommendations for a specific service diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index efdca18dc..557e31f13 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -2,6 +2,7 @@ package azure import ( "context" + "errors" "strings" "testing" @@ -435,15 +436,16 @@ func TestMergeServiceResults_OrderIsStable(t *testing.T) { advisorRec := mkRec(common.ServiceCompute, "advisor") // Advisor produces Compute recs // Replicate the exact call order from GetRecommendations. - result := mergeServiceResults( - serviceResult{"compute", []common.Recommendation{computeRec}, nil}, - serviceResult{"database", []common.Recommendation{dbRec}, nil}, - serviceResult{"cache", []common.Recommendation{cacheRec}, nil}, - serviceResult{"cosmosdb", []common.Recommendation{cosmosRec}, nil}, - serviceResult{"savingsplans", []common.Recommendation{spRec}, nil}, - serviceResult{"advisor", []common.Recommendation{advisorRec}, nil}, + result, err := mergeServiceResults( + serviceResult{"compute", []common.Recommendation{computeRec}, nil, true}, + serviceResult{"database", []common.Recommendation{dbRec}, nil, true}, + serviceResult{"cache", []common.Recommendation{cacheRec}, nil, true}, + serviceResult{"cosmosdb", []common.Recommendation{cosmosRec}, nil, true}, + serviceResult{"savingsplans", []common.Recommendation{spRec}, nil, true}, + serviceResult{"advisor", []common.Recommendation{advisorRec}, nil, true}, ) + require.NoError(t, err, "all-success merge must not error") require.Len(t, result, 6, "all six services must be represented") assert.Equal(t, "compute", result[0].ResourceType, "first must be compute") assert.Equal(t, "database", result[1].ResourceType, "second must be database") @@ -452,3 +454,106 @@ func TestMergeServiceResults_OrderIsStable(t *testing.T) { assert.Equal(t, "savingsplans", result[4].ResourceType, "fifth must be savingsplans") assert.Equal(t, "advisor", result[5].ResourceType, "sixth must be advisor (compute-type)") } + +// TestMergeServiceResults_AllAttemptedFailed is the COR-03 regression test: +// when EVERY attempted service errors (the shape produced by an expired +// federated credential or a subscription-wide throttle), the merge must return +// a non-nil error instead of (empty, nil). Pre-fix, mergeServiceResults +// returned only []common.Recommendation, so a total failure surfaced as an +// empty successful collection: the scheduler counted the account in +// SucceededAccountIDs, UpsertRecommendations evicted all previously collected +// rows for it, and last_collection_error was cleared. +// +// savingsplans and advisor are both attempted=false because GetRecommendations +// excludes them from the guard: savingsplans is a stub that makes no API call +// and always returns ([], nil); advisor's getAdvisorRecommendations swallows +// pagination errors and therefore also always returns (recs, nil). Counting +// either as an attempted service would keep the guard from firing on a real +// total credential failure. +func TestMergeServiceResults_AllAttemptedFailed(t *testing.T) { + authErr := errors.New("DefaultAzureCredential: federated token expired") + + recs, err := mergeServiceResults( + serviceResult{"compute", nil, authErr, true}, + serviceResult{"database", nil, authErr, true}, + serviceResult{"cache", nil, authErr, true}, + serviceResult{"cosmosdb", nil, authErr, true}, + serviceResult{"savingsplans", nil, nil, false}, + serviceResult{"advisor", nil, nil, false}, + ) + + require.Error(t, err, "all-attempted-failed merge must fail loud, not return (empty, nil)") + assert.ErrorIs(t, err, authErr, "the underlying service error must be wrapped") + assert.Contains(t, err.Error(), "all 4 Azure recommendation services failed") + assert.Nil(t, recs) +} + +// TestMergeServiceResults_SkippedServicesDoNotMaskTotalFailure asserts that +// services skipped by the params filter (attempted == false, nil err) are not +// counted as successes: when every ATTEMPTED service failed, the merge must +// still error even though skipped entries carry a nil err. +// savingsplans and advisor are always attempted=false in production (see +// TestMergeServiceResults_AllAttemptedFailed), so the scenario here models +// a service-filter run where only compute is requested and it fails. +func TestMergeServiceResults_SkippedServicesDoNotMaskTotalFailure(t *testing.T) { + throttleErr := errors.New("429 too many requests") + + _, err := mergeServiceResults( + serviceResult{"compute", nil, throttleErr, true}, + serviceResult{"database", nil, nil, false}, + serviceResult{"cache", nil, nil, false}, + serviceResult{"cosmosdb", nil, nil, false}, + serviceResult{"savingsplans", nil, nil, false}, + serviceResult{"advisor", nil, nil, false}, + ) + + require.Error(t, err, "skipped services must not count as successes in the all-failed guard") + assert.ErrorIs(t, err, throttleErr) +} + +// TestMergeServiceResults_PartialFailureStillSucceeds pins the tolerated +// partial-failure behavior: one service succeeding is enough for the merge +// to return its recommendations with a nil error. +func TestMergeServiceResults_PartialFailureStillSucceeds(t *testing.T) { + svcErr := errors.New("reservation API unavailable") + computeRec := common.Recommendation{Service: common.ServiceCompute, Provider: common.ProviderAzure} + + recs, err := mergeServiceResults( + serviceResult{"compute", []common.Recommendation{computeRec}, nil, true}, + serviceResult{"database", nil, svcErr, true}, + serviceResult{"cache", nil, svcErr, true}, + serviceResult{"cosmosdb", nil, svcErr, true}, + serviceResult{"savingsplans", nil, nil, false}, + serviceResult{"advisor", nil, nil, false}, + ) + + require.NoError(t, err, "partial failure must still return the successful services' recs") + require.Len(t, recs, 1) + assert.Equal(t, common.ServiceCompute, recs[0].Service) +} + +// TestMergeServiceResults_StubsDoNotMaskTotalFailure replicates the exact +// production shape of a total Azure provider failure (e.g. an expired +// federated credential): the four real services all error, while both +// non-API-making entries -- the savingsplans stub (always ([], nil)) and +// advisor (getAdvisorRecommendations swallows pagination auth errors and +// always returns (recs, nil)) -- succeed. GetRecommendations passes +// attempted=false for both; the merge must still fail loud rather than +// counting either stub's unconditional success and returning (empty, nil). +func TestMergeServiceResults_StubsDoNotMaskTotalFailure(t *testing.T) { + authErr := errors.New("DefaultAzureCredential: federated token expired") + + recs, err := mergeServiceResults( + serviceResult{"compute", nil, authErr, true}, + serviceResult{"database", nil, authErr, true}, + serviceResult{"cache", nil, authErr, true}, + serviceResult{"cosmosdb", nil, authErr, true}, + serviceResult{"savingsplans", []common.Recommendation{}, nil, false}, + serviceResult{"advisor", []common.Recommendation{}, nil, false}, + ) + + require.Error(t, err, "stubs that swallow errors must not mask a total provider failure") + assert.ErrorIs(t, err, authErr) + assert.Contains(t, err.Error(), "all 4 Azure recommendation services failed") + assert.Nil(t, recs) +} diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index 8d98cf6b3..4b1b28a66 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -126,6 +126,12 @@ func (c *Client) GetRegion() string { // recommendations (the Benefits Recommendations API is still in preview). // This mirrors the AWS Savings Plans client which also returns empty here and // delegates to Cost Explorer centrally. +// +// NOTE: because this stub succeeds unconditionally, it is excluded from the +// all-attempted-failed guard in the parent RecommendationsClientAdapter +// (providers/azure/recommendations.go, COR-03). When this method starts +// making real API calls, flip that call site's attempted flag back to the +// params-filter value so its failures count toward the guard. func (c *Client) GetRecommendations(_ context.Context, _ common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/gcp/recommendations.go b/providers/gcp/recommendations.go index 2a8c1cecc..1d19ca2c0 100644 --- a/providers/gcp/recommendations.go +++ b/providers/gcp/recommendations.go @@ -59,6 +59,13 @@ type regionResult struct { sql []common.Recommendation cache []common.Recommendation storage []common.Recommendation + // attempted counts the service calls launched for this region (after the + // params service filter), failed counts how many of those errored, and + // lastErr keeps one representative error. mergeRegionResults aggregates + // these across regions for the all-attempted-failed guard (COR-03). + attempted int + failed int + lastErr error } // RecommendationsClientAdapter aggregates GCP CUD and commitment recommendations across all services @@ -152,15 +159,42 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p } sort.Strings(sortedRegions) - allRecommendations := make([]common.Recommendation, 0) + return mergeRegionResults(sortedRegions, results) +} + +// mergeRegionResults appends compute, sql, cache, storage per region in the +// (sorted) order given so output is deterministic, and ports the AWS 08-H4 +// all-failed guard from providers/aws/recommendations/client.go: when every +// attempted (region, service) call errored (e.g. an expired credential, a +// project-wide throttle, or an RBAC gap), it returns a wrapped error instead +// of an empty-but-nil-error result. Returning (recs, nil) on a total failure +// makes a broken run indistinguishable from "no savings available": the +// scheduler would count the account as succeeded, evict its previously +// collected rows, and clear last_collection_error (COR-03). Partial failure +// is tolerated: if at least one call succeeded, the successful results are +// returned with a nil error (failures were already logged at WARN in +// collectRegion). +func mergeRegionResults(sortedRegions []string, results map[string]regionResult) ([]common.Recommendation, error) { + attempted := 0 + failed := 0 + var lastErr error + merged := make([]common.Recommendation, 0) for _, region := range sortedRegions { res := results[region] - allRecommendations = append(allRecommendations, res.compute...) - allRecommendations = append(allRecommendations, res.sql...) - allRecommendations = append(allRecommendations, res.cache...) - allRecommendations = append(allRecommendations, res.storage...) + attempted += res.attempted + failed += res.failed + if res.lastErr != nil { + lastErr = res.lastErr + } + merged = append(merged, res.compute...) + merged = append(merged, res.sql...) + merged = append(merged, res.cache...) + merged = append(merged, res.storage...) } - return allRecommendations, nil + if failed > 0 && failed == attempted { + return nil, fmt.Errorf("all %d GCP recommendation service calls failed across %d regions: %w", failed, len(sortedRegions), lastErr) + } + return merged, nil } // collectComputeRecs fetches Compute Engine CUD recommendations for one region. @@ -220,9 +254,13 @@ func (r *RecommendationsClientAdapter) collectStorageRecs(ctx context.Context, p // collectRegion fetches recommendations for all four GCP services // (Compute Engine, Cloud SQL, Memorystore, Cloud Storage) for a single region // concurrently. Per-service errors are logged at WARN with the region+service -// tag and never propagate -- the previous silent-skip-on-err shape is preserved -// (so a misconfigured project doesn't error out the whole recommendations -// refresh) but errors are now observable in logs. The per-service fetch logic +// tag and do not fail the region on their own -- the previous +// silent-skip-on-err shape is preserved for partial failures (so a +// misconfigured project doesn't error out the whole recommendations refresh). +// Each region does report its attempted/failed call counts and a +// representative error in the returned regionResult so mergeRegionResults can +// fail loud when EVERY attempted call across all regions errored (COR-03). +// The per-service fetch logic // (semaphore, client construction, GetRecommendations call) is delegated to // dedicated helpers (collectComputeRecs, collectSQLRecs, collectCacheRecs, // collectStorageRecs) to keep this function's cyclomatic complexity under the @@ -239,25 +277,30 @@ func (r *RecommendationsClientAdapter) collectRegion(ctx context.Context, params g, gctx := errgroup.WithContext(ctx) + attempted := 0 if shouldIncludeService(params, common.ServiceCompute) { + attempted++ g.Go(func() error { computeRecs, computeErr = r.collectComputeRecs(gctx, params, region) return nil }) } if shouldIncludeService(params, common.ServiceRelationalDB) { + attempted++ g.Go(func() error { sqlRecs, sqlErr = r.collectSQLRecs(gctx, params, region) return nil }) } if shouldIncludeService(params, common.ServiceCache) { + attempted++ g.Go(func() error { cacheRecs, cacheErr = r.collectCacheRecs(gctx, params, region) return nil }) } if shouldIncludeService(params, common.ServiceStorage) { + attempted++ g.Go(func() error { storageRecs, storageErr = r.collectStorageRecs(gctx, params, region) return nil @@ -265,20 +308,33 @@ func (r *RecommendationsClientAdapter) collectRegion(ctx context.Context, params } _ = g.Wait() + failed := 0 + var lastErr error if computeErr != nil { logging.Warnf("GCP %s compute recommendations: %v", region, computeErr) + failed++ + lastErr = computeErr } if sqlErr != nil { logging.Warnf("GCP %s cloudsql recommendations: %v", region, sqlErr) + failed++ + lastErr = sqlErr } if cacheErr != nil { logging.Warnf("GCP %s memorystore recommendations: %v", region, cacheErr) + failed++ + lastErr = cacheErr } if storageErr != nil { logging.Warnf("GCP %s cloudstorage recommendations: %v", region, storageErr) + failed++ + lastErr = storageErr } - return regionResult{compute: computeRecs, sql: sqlRecs, cache: cacheRecs, storage: storageRecs} + return regionResult{ + compute: computeRecs, sql: sqlRecs, cache: cacheRecs, storage: storageRecs, + attempted: attempted, failed: failed, lastErr: lastErr, + } } // GetRecommendationsForService retrieves GCP commitment recommendations for a specific service diff --git a/providers/gcp/recommendations_test.go b/providers/gcp/recommendations_test.go index d0fe486b6..94f9c3eb6 100644 --- a/providers/gcp/recommendations_test.go +++ b/providers/gcp/recommendations_test.go @@ -2,6 +2,7 @@ package gcp import ( "context" + "errors" "os" "testing" @@ -266,3 +267,61 @@ func TestGCPRegionConcurrency(t *testing.T) { assert.Equal(t, defaultGCPRegionConcurrency, gcpRegionConcurrency()) }) } + +// TestMergeRegionResults_AllAttemptedFailed is the COR-03 regression test: +// when EVERY attempted (region, service) call errors (the shape produced by a +// project-wide throttle or an RBAC gap that survives getRegions), the merge +// must return a non-nil error instead of (empty, nil). Pre-fix, the per-region +// errors were logged in collectRegion and dropped, so a total failure surfaced +// as an empty successful collection: the scheduler counted the account in +// SucceededAccountIDs, UpsertRecommendations evicted all previously collected +// rows for it, and last_collection_error was cleared. +func TestMergeRegionResults_AllAttemptedFailed(t *testing.T) { + rbacErr := errors.New("googleapi: Error 403: missing recommender.commitmentRecommendations.list") + + results := map[string]regionResult{ + "europe-west1": {attempted: 4, failed: 4, lastErr: rbacErr}, + "us-central1": {attempted: 4, failed: 4, lastErr: rbacErr}, + } + + recs, err := mergeRegionResults([]string{"europe-west1", "us-central1"}, results) + + require.Error(t, err, "all-attempted-failed merge must fail loud, not return (empty, nil)") + assert.ErrorIs(t, err, rbacErr, "the underlying service error must be wrapped") + assert.Contains(t, err.Error(), "all 8 GCP recommendation service calls failed across 2 regions") + assert.Nil(t, recs) +} + +// TestMergeRegionResults_PartialFailureStillSucceeds pins the tolerated +// partial-failure behavior: a single successful (region, service) call is +// enough for the merge to return its recommendations with a nil error. +func TestMergeRegionResults_PartialFailureStillSucceeds(t *testing.T) { + throttleErr := errors.New("googleapi: Error 429: quota exceeded") + computeRec := common.Recommendation{Service: common.ServiceCompute, Provider: common.ProviderGCP, Region: "us-central1"} + + results := map[string]regionResult{ + "europe-west1": {attempted: 4, failed: 4, lastErr: throttleErr}, + "us-central1": { + compute: []common.Recommendation{computeRec}, + attempted: 4, + failed: 3, + lastErr: throttleErr, + }, + } + + recs, err := mergeRegionResults([]string{"europe-west1", "us-central1"}, results) + + require.NoError(t, err, "partial failure must still return the successful calls' recs") + require.Len(t, recs, 1) + assert.Equal(t, "us-central1", recs[0].Region) +} + +// TestMergeRegionResults_NoAttemptsIsNotAFailure asserts the guard does not +// fire when nothing was attempted (zero regions, or every service filtered +// out by params): that case keeps the previous empty-success behavior. +func TestMergeRegionResults_NoAttemptsIsNotAFailure(t *testing.T) { + recs, err := mergeRegionResults(nil, map[string]regionResult{}) + + require.NoError(t, err) + assert.Empty(t, recs) +}