fix(providers): fail loud when all Azure/GCP rec services fail

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.

providers/azure/recommendations.go

@@ -97,32 +97,41 @@ 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)
 		})
 	}
 
 	// 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)
 		})
 	}
 
 	// 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)
 		})
 	}
 
 	// 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

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)
+}

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
 }