fix(scheduler): propagate ambient fallback GetRecommendations error

internal/scheduler/scheduler.go

@@ -879,10 +879,13 @@ func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider,
 			PaymentOption:  globalCfg.DefaultPayment,
 			LookbackPeriod: fmt.Sprintf("%dd", lookbackDays),
 		}
-		var recErr error
-		recs, recErr = recClient.GetRecommendations(ctx, params)
-		if recErr != nil {
-			logging.Warnf("fetchAndConvert: %s GetRecommendations fallback failed: %v", providerName, recErr)
+		recs, err = recClient.GetRecommendations(ctx, params)
+		if err != nil {
+			// Fail loud: a misconfigured DefaultPayment/DefaultTerm or a CE
+			// failure on this fallback must surface to the operator instead
+			// of silently presenting as "zero recommendations".
+			return nil, fmt.Errorf("failed to get %s recommendations with default term/payment fallback (term=%s, payment=%s, lookback=%s): %w",
+				providerName, params.Term, params.PaymentOption, params.LookbackPeriod, err)
 		}
 	}
 	result := s.convertRecommendations(recs, providerName)

internal/scheduler/scheduler_test.go

@@ -1642,6 +1642,48 @@ func TestScheduler_CollectAWSRecommendations_FallbackToFiltered(t *testing.T) {
 	assert.Len(t, recs, 1)
 }
 
+// Regression test for COR-05 (#1168): when the primary sweep returns zero
+// recommendations and the default term/payment fallback GetRecommendations
+// call fails, the error must be propagated instead of being silently
+// swallowed (`recs, _ =`) and presenting as "zero recommendations".
+func TestScheduler_CollectAWSRecommendations_FallbackError(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockFactory := new(MockProviderFactory)
+	mockProvider := new(MockProvider)
+	mockRecClient := new(MockRecommendationsClient)
+	t.Cleanup(func() {
+		mockFactory.AssertExpectations(t)
+		mockProvider.AssertExpectations(t)
+		mockRecClient.AssertExpectations(t)
+	})
+
+	globalCfg := &config.GlobalConfig{
+		DefaultTerm:    3,
+		DefaultPayment: "all-upfront",
+	}
+
+	fallbackErr := errors.New("ValidationException: invalid PaymentOption")
+
+	mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil)
+	mockProvider.On("GetRecommendationsClient", ctx).Return(mockRecClient, nil)
+	mockRecClient.On("GetAllRecommendations", ctx).Return([]common.Recommendation{}, nil) // Empty -> triggers fallback
+	mockRecClient.On("GetRecommendations", ctx, mock.AnythingOfType("common.RecommendationParams")).Return(nil, fallbackErr)
+
+	scheduler := &Scheduler{
+		config:          mockStore,
+		providerFactory: mockFactory,
+	}
+
+	recs, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg)
+	require.Error(t, err, "fallback GetRecommendations failure must propagate, not be swallowed")
+	require.ErrorIs(t, err, fallbackErr)
+	assert.Contains(t, err.Error(), "default term/payment fallback", "error must identify the fallback path")
+	assert.Contains(t, err.Error(), "term=3yr")
+	assert.Contains(t, err.Error(), "payment=all-upfront")
+	assert.Nil(t, recs, "no recommendations must be returned when the fallback fails")
+}
+
 // fakeSTSClient is a minimal in-test STSClient implementation used by the
 // ambient host-account tagging tests (issue #604). The fakeAccountID + err
 // fields are set by each test case to drive the GetCallerIdentity response

providers/aws/recommendations/converters.go

@@ -31,10 +31,11 @@ func getServiceStringForCostExplorer(service common.ServiceType) string {
 
 // convertPaymentOption converts payment option string to AWS type.
 //
-// Deprecated: this function silently defaults to NoUpfront for unrecognized
-// values and is retained only for the legacy RI recommendation-fetch path in
-// client.go (owned by #865/#1075). New callers must use convertPaymentOptionE
-// and propagate the error. See open-questions/fix-aws-converters.md OQ-1.
+// Deprecated: this function silently returns the empty ("") PaymentOption for
+// unrecognized values, which Cost Explorer rejects with a validation error. It
+// is retained only for the legacy RI recommendation-fetch path in client.go
+// (owned by #865/#1075). New callers must use convertPaymentOptionE and
+// propagate the error. See open-questions/fix-aws-converters.md OQ-1.
 func convertPaymentOption(option string) types.PaymentOption {
 	v, _ := convertPaymentOptionE(option)
 	return v
@@ -72,10 +73,11 @@ func convertTermInYearsE(term string) (types.TermInYears, error) {
 
 // convertTermInYears converts term string to AWS type.
 //
-// Deprecated: this function silently defaults to OneYear for unrecognized
-// values and is retained only for the legacy RI recommendation-fetch path in
-// client.go (owned by #865/#1075). New callers must use convertTermInYearsE
-// and propagate the error. See open-questions/fix-aws-converters.md OQ-1.
+// Deprecated: this function silently returns the empty ("") TermInYears for
+// unrecognized values, which Cost Explorer rejects with a validation error. It
+// is retained only for the legacy RI recommendation-fetch path in client.go
+// (owned by #865/#1075). New callers must use convertTermInYearsE and
+// propagate the error. See open-questions/fix-aws-converters.md OQ-1.
 func convertTermInYears(term string) types.TermInYears {
 	v, _ := convertTermInYearsE(term)
 	return v
@@ -99,10 +101,11 @@ func convertLookbackPeriodE(period string) (types.LookbackPeriodInDays, error) {
 
 // convertLookbackPeriod converts lookback period string to AWS type.
 //
-// Deprecated: this function silently defaults to SevenDays for unrecognized
-// values and is retained only for the legacy RI recommendation-fetch path in
-// client.go (owned by #865/#1075). New callers must use convertLookbackPeriodE
-// and propagate the error. See open-questions/fix-aws-converters.md OQ-1.
+// Deprecated: this function silently returns the empty ("") LookbackPeriodInDays
+// for unrecognized values, which Cost Explorer rejects with a validation error.
+// It is retained only for the legacy RI recommendation-fetch path in client.go
+// (owned by #865/#1075). New callers must use convertLookbackPeriodE and
+// propagate the error. See open-questions/fix-aws-converters.md OQ-1.
 func convertLookbackPeriod(period string) types.LookbackPeriodInDays {
 	v, _ := convertLookbackPeriodE(period)
 	return v

providers/aws/recommendations/converters_test.go

@@ -86,10 +86,12 @@ func TestGetServiceStringForCostExplorer(t *testing.T) {
 }
 
 func TestConvertPaymentOption(t *testing.T) {
-	// convertPaymentOption is the legacy wrapper that silently defaults to NoUpfront
-	// for unknown values (used by client.go RI path, owned by #865/#1075).
-	// This test documents that silent-default behaviour; new callers should use
-	// convertPaymentOptionE which returns an error on unrecognised values.
+	// convertPaymentOption is the legacy wrapper used by client.go (RI path,
+	// owned by #865/#1075); it silently returns the empty ("") PaymentOption
+	// for unrecognized values, which Cost Explorer then rejects with a
+	// validation error. This test covers the valid cases only; the fail-loud
+	// path is tested by TestConvertPaymentOptionE_FailLoud. New callers must
+	// use convertPaymentOptionE and propagate the error.
 	tests := []struct {
 		name     string
 		option   string
@@ -122,10 +124,11 @@ func TestConvertPaymentOption(t *testing.T) {
 
 // TestConvertPaymentOptionE_FailLoud is the regression test for H3:
 // convertPaymentOptionE must return an error on any unrecognised payment option
-// instead of silently substituting NoUpfront (the old behaviour of the
-// convertPaymentOption default branch). Callers on the SP recommendation path
-// use this erroring variant so a typo or new/renamed option is caught
-// before the wrong recs are queried.
+// instead of silently substituting the empty ("") PaymentOption (the old
+// behaviour of the convertPaymentOption default branch, which surfaced to CE
+// as a confusing validation error). Callers on the SP recommendation path use
+// this erroring variant so a typo or new/renamed option is caught before the
+// wrong recs are queried.
 func TestConvertPaymentOptionE_FailLoud(t *testing.T) {
 	tests := []struct {
 		name        string
@@ -136,7 +139,7 @@ func TestConvertPaymentOptionE_FailLoud(t *testing.T) {
 		{"All upfront", "all-upfront", types.PaymentOptionAllUpfront, false},
 		{"Partial upfront", "partial-upfront", types.PaymentOptionPartialUpfront, false},
 		{"No upfront", "no-upfront", types.PaymentOptionNoUpfront, false},
-		// These must error, not default to NoUpfront (H3 regression guard):
+		// These must error, not return the empty PaymentOption (H3 regression guard):
 		{"Unknown option errors", "unknown", "", true},
 		{"Empty string errors", "", "", true},
 		{"Mixed case errors", "All-Upfront", "", true},
@@ -158,7 +161,7 @@ func TestConvertPaymentOptionE_FailLoud(t *testing.T) {
 
 // TestConvertTermInYearsE_FailLoud is the regression test for L1:
 // convertTermInYearsE must error on unrecognised terms rather than silently
-// defaulting to OneYear.
+// returning the empty ("") TermInYears (which CE then rejects).
 func TestConvertTermInYearsE_FailLoud(t *testing.T) {
 	tests := []struct {
 		name        string
@@ -170,7 +173,7 @@ func TestConvertTermInYearsE_FailLoud(t *testing.T) {
 		{"1 numeric", "1", types.TermInYearsOneYear, false},
 		{"3yr", "3yr", types.TermInYearsThreeYears, false},
 		{"3 numeric", "3", types.TermInYearsThreeYears, false},
-		// These must error, not default to OneYear (L1 regression guard):
+		// These must error, not return the empty TermInYears (L1 regression guard):
 		{"Unknown term errors", "unknown", "", true},
 		{"Empty string errors", "", "", true},
 		{"2yr errors", "2yr", "", true},
@@ -192,7 +195,7 @@ func TestConvertTermInYearsE_FailLoud(t *testing.T) {
 
 // TestConvertLookbackPeriodE_FailLoud is the regression test for L2:
 // convertLookbackPeriodE must error on unrecognised periods rather than
-// silently defaulting to SevenDays.
+// silently returning the empty ("") LookbackPeriodInDays (which CE then rejects).
 func TestConvertLookbackPeriodE_FailLoud(t *testing.T) {
 	tests := []struct {
 		name        string
@@ -206,7 +209,7 @@ func TestConvertLookbackPeriodE_FailLoud(t *testing.T) {
 		{"30 numeric", "30", types.LookbackPeriodInDaysThirtyDays, false},
 		{"60d", "60d", types.LookbackPeriodInDaysSixtyDays, false},
 		{"60 numeric", "60", types.LookbackPeriodInDaysSixtyDays, false},
-		// These must error, not default to SevenDays (L2 regression guard):
+		// These must error, not return the empty LookbackPeriodInDays (L2 regression guard):
 		{"Unknown period errors", "unknown", "", true},
 		{"Empty string errors", "", "", true},
 		{"90d errors", "90d", "", true},
@@ -228,8 +231,10 @@ func TestConvertLookbackPeriodE_FailLoud(t *testing.T) {
 
 func TestConvertTermInYears(t *testing.T) {
 	// convertTermInYears is the legacy wrapper used by client.go (RI path);
-	// it silently returns OneYear for unrecognised values. This test covers the
-	// valid cases only; the fail-loud path is tested by TestConvertTermInYearsE_FailLoud.
+	// it silently returns the empty ("") TermInYears for unrecognised values,
+	// which Cost Explorer then rejects with a validation error. This test
+	// covers the valid cases only; the fail-loud path is tested by
+	// TestConvertTermInYearsE_FailLoud.
 	tests := []struct {
 		name     string
 		term     string
@@ -267,8 +272,10 @@ func TestConvertTermInYears(t *testing.T) {
 
 func TestConvertLookbackPeriod(t *testing.T) {
 	// convertLookbackPeriod is the legacy wrapper used by client.go (RI path);
-	// it silently returns SevenDays for unrecognised values. This test covers valid
-	// cases only; the fail-loud path is tested by TestConvertLookbackPeriodE_FailLoud.
+	// it silently returns the empty ("") LookbackPeriodInDays for unrecognised
+	// values, which Cost Explorer then rejects with a validation error. This
+	// test covers valid cases only; the fail-loud path is tested by
+	// TestConvertLookbackPeriodE_FailLoud.
 	tests := []struct {
 		name     string
 		period   string