From c969c5fafa10fcd7f444aceb89c208392c7c6000 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 21:00:18 +0200 Subject: [PATCH] fix(cmd): fail the run when recommendation fetching fails fetchRecommendationsForRegion logged provider errors and returned nil, and fetchAllRecs logged region-discovery failures and continued, so an expired session or denied Cost Explorer call read as "Nothing to purchase" with exit 0. Both now return the error and the run exits 1. Empty answers and incomplete-but-usable answers keep their current behaviour. Refs #2190 Co-Authored-By: Claude Sonnet 5.5 --- cmd/cancellation_prepurchase_test.go | 32 +++++++++++------ cmd/multi_service.go | 10 +++++- cmd/multi_service_helpers.go | 41 +++++++++++++++------- cmd/recommendation_completeness_test.go | 46 +++++++++++-------------- cmd/savings_plan_purchase_test.go | 3 +- cmd/savings_plan_region_test.go | 3 +- cmd/sp_account_scope_test.go | 3 +- 7 files changed, 87 insertions(+), 51 deletions(-) diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index b2d858b04..9aa5bdb37 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -88,8 +88,9 @@ func TestFetchRecommendationsForRegion_InterruptIsNotReportedAsProviderFailure(t client := &MockRecommendationsClient{} client.On("GetRecommendations", mock.Anything, mock.Anything).Return(nil, ctx.Err()) - got := fetchRecommendationsForRegion(ctx, client, common.ServiceRDS, "us-east-1", Config{TermYears: 1}) + got, err := fetchRecommendationsForRegion(ctx, client, common.ServiceRDS, "us-east-1", Config{TermYears: 1}) + assert.ErrorIs(t, err, context.Canceled) assert.Nil(t, got) assert.NotContains(t, out.String(), "Failed to fetch recommendations") } @@ -111,20 +112,31 @@ func TestFetchAllRecs_InterruptStopsFanOutAndDiscardsPartialData(t *testing.T) { client.AssertNumberOfCalls(t, "GetRecommendations", 1) // later regions are never queried } -func TestFetchAllRecs_ActiveContextProviderErrorStaysRecoverable(t *testing.T) { - out := captureAppLog(t) +func TestFetchAllRecs_ProviderErrorFailsTheRun(t *testing.T) { + captureAppLog(t) cfg := Config{Regions: []string{"us-east-1", "us-west-2"}, Coverage: 100, TermYears: 1, PaymentOption: "partial-upfront"} client := &MockRecommendationsClient{} client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { return p.Region == "us-east-1" })). - Return(nil, errors.New("throttling")).Once() - rec := common.Recommendation{Service: common.ServiceRDS, Region: "us-west-2", ResourceType: "db.t3.small", Count: 3, EstimatedSavings: 10, SavingsPercentage: 10} - client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { return p.Region == "us-west-2" })). - Return([]common.Recommendation{rec}, nil).Once() + Return(nil, errors.New("expired SSO token")).Once() + + recs, _, err := fetchAllRecs(context.Background(), aws.Config{}, client, NewAccountAliasCache(aws.Config{}), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) + + require.Error(t, err, "a fetch failure must not read as 'no recommendations'") + assert.Contains(t, err.Error(), "expired SSO token") + assert.Empty(t, recs) + client.AssertNumberOfCalls(t, "GetRecommendations", 1) +} + +func TestFetchAllRecs_EmptyResultIsNotAnError(t *testing.T) { + captureAppLog(t) + cfg := Config{Regions: []string{"us-east-1"}, Coverage: 100, TermYears: 1, PaymentOption: "partial-upfront"} + client := &MockRecommendationsClient{} + client.On("GetRecommendations", mock.Anything, mock.Anything).Return([]common.Recommendation{}, nil) - recs, _, _ := fetchAllRecs(context.Background(), aws.Config{}, client, NewAccountAliasCache(aws.Config{}), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) + recs, _, err := fetchAllRecs(context.Background(), aws.Config{}, client, NewAccountAliasCache(aws.Config{}), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) - assert.Len(t, recs, 1, "a provider error with an active context only loses that region") - assert.Contains(t, out.String(), "Failed to fetch recommendations") + require.NoError(t, err) + assert.Empty(t, recs) } func TestCheckDuplicates_InterruptedCheckIsTerminalInDryAndRealRuns(t *testing.T) { diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 2085869b6..1636cf2aa 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "log" "sync/atomic" @@ -188,7 +189,7 @@ func runToolMultiService(ctx context.Context, cfg Config) { // Phase 1: collect all recommendations without purchasing. AppLogger.Printf("\nšŸ“„ Fetching recommendations from all services...\n") allRecs, drops, err := fetchAllRecs(ctx, awsCfg, recClient, accountCache, servicesToProcess, engineData, cfg, coverageMap) - exitOnExclusionError(err) + exitOnFetchError(err) exitIfInterrupted(ctx, "fetching recommendations") // Phase 2: score, enforce the run-wide instance cap, and display. @@ -1051,6 +1052,13 @@ func hasDatabaseRecs(recs []common.Recommendation) bool { // exitOnExclusionError aborts the run before any purchase when the // extended-support exclusion cannot be applied (issue #2147). +func exitOnFetchError(err error) { + if errors.Is(err, errRecommendationFetch) { + log.Fatalf("Cannot complete the recommendation fetch: %v", err) + } + exitOnExclusionError(err) +} + func exitOnExclusionError(err error) { if err != nil { log.Fatalf("Cannot apply extended-support exclusion: %v", err) diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 831ed41e5..87cd19f40 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -508,7 +508,13 @@ func processRegionRecommendations( AppLogger.Printf("\n šŸ“ [%d/%d] Region: %s\n", regionIndex, totalRegions, region) // Fetch recommendations - recs := fetchRecommendationsForRegion(ctx, recClient, service, region, cfg) + recs, fetchErr := fetchRecommendationsForRegion(ctx, recClient, service, region, cfg) + if fetchErr != nil { + // Legacy test-only path: the CLI run goes through fetchAndFilterRegionRecs, + // which returns this error to the caller. + AppLogger.Printf(" āŒ Failed to fetch recommendations: %v\n", fetchErr) + return result + } if len(recs) == 0 { AppLogger.Printf(" ā„¹ļø No recommendations found\n") return result @@ -582,14 +588,21 @@ func processRegionRecommendations( return result } +// errRecommendationFetch marks a failure to obtain recommendations from the +// provider, as opposed to a successful but empty answer. +var errRecommendationFetch = errors.New("recommendation fetch failed") + // fetchRecommendationsForRegion fetches recommendations from AWS for a specific region. +// A provider failure is returned, not logged away: an empty slice with a nil +// error means the provider answered and had nothing to recommend. An +// incomplete-but-usable answer (partial sweep) is returned with a warning. func fetchRecommendationsForRegion( ctx context.Context, recClient provider.RecommendationsClient, service common.ServiceType, region string, cfg Config, -) []common.Recommendation { +) ([]common.Recommendation, error) { termStr := "1yr" if cfg.TermYears == 3 { termStr = "3yr" @@ -622,7 +635,7 @@ func fetchRecommendationsForRegion( var incomplete *recommendations.IncompleteRecommendationsError if errors.As(err, &incomplete) { AppLogger.Printf(" āš ļø %v\n", incomplete) - return recs + return recs, nil } if partial := azureprovider.AsPartialSubscriptionFailure(err); partial != nil { // Keep the subscriptions that did answer, but say plainly that the @@ -634,18 +647,17 @@ func fetchRecommendationsForRegion( for _, f := range partial.Failed { AppLogger.Printf(" subscription %s: %v\n", f.SubscriptionID, f.Err) } - return recs + return recs, nil } - if ctx.Err() != nil { + if ctxErr := ctx.Err(); ctxErr != nil { // Interrupted, not a provider failure: the caller stops on ctx.Err(). - return nil + return nil, ctxErr } if err != nil { - AppLogger.Printf(" āŒ Failed to fetch recommendations: %v\n", err) - return nil + return nil, fmt.Errorf("%w: %s in %s: %w", errRecommendationFetch, getServiceDisplayName(service), region, err) } - return recs + return recs, nil } // stampSavingsPlanRegions copies the region Cost Explorer reports for an EC2 @@ -854,7 +866,10 @@ func fetchAndFilterRegionRecs( ) ([]common.Recommendation, error) { AppLogger.Printf("\n šŸ“ [%d/%d] Region: %s\n", regionIndex, totalRegions, region) - recs := fetchRecommendationsForRegion(ctx, recClient, service, region, cfg) + recs, err := fetchRecommendationsForRegion(ctx, recClient, service, region, cfg) + if err != nil { + return nil, err + } if len(recs) == 0 { AppLogger.Printf(" ā„¹ļø No recommendations found\n") return nil, nil @@ -930,14 +945,16 @@ func fetchAllRecs( if ctx.Err() != nil { return nil, drops, nil } - log.Printf("āŒ Failed to determine regions for %s: %v", getServiceDisplayName(service), err) - continue + return nil, nil, fmt.Errorf("%w: determining regions for %s: %w", errRecommendationFetch, getServiceDisplayName(service), err) } for i, region := range regions { if ctx.Err() != nil { return nil, drops, nil // partial data from an interrupted fetch is not a result } recs, err := fetchAndFilterRegionRecs(ctx, awsCfg, recClient, accountCache, service, region, i+1, len(regions), engineData, cfg, coverageMap, drops) + if ctx.Err() != nil { + return nil, drops, nil // interrupted mid-fetch: not a result, not a failure + } if err != nil { return nil, nil, err } diff --git a/cmd/recommendation_completeness_test.go b/cmd/recommendation_completeness_test.go index cfee43e17..9368d2761 100644 --- a/cmd/recommendation_completeness_test.go +++ b/cmd/recommendation_completeness_test.go @@ -67,9 +67,20 @@ func runCompletenessScenario(t *testing.T, service, regionMode, details string) } var stdout, stderr bytes.Buffer child.Stdout, child.Stderr = &stdout, &stderr - require.NoError(t, child.Run(), "stdout: %s\nstderr: %s", &stdout, &stderr) - fixture.assertRequests(t) + runErr := child.Run() logs := stdout.String() + stderr.String() + if details == "api-error" || details == "failed-type" { + // A provider failure is a failed run, not "nothing to buy". + var exitErr *exec.ExitError + require.ErrorAs(t, runErr, &exitErr, logs) + require.Equal(t, 1, exitErr.ExitCode(), logs) + require.Contains(t, logs, "Cannot complete the recommendation fetch") + require.Contains(t, logs, "fixture denied recommendations") + require.NoFileExists(t, output) + return + } + require.NoError(t, runErr, "stdout: %s\nstderr: %s", &stdout, &stderr) + fixture.assertRequests(t) for _, failure := range []string{"Could not check", "Failed to query", "request send failed", "certificate", "TLS handshake"} { require.NotContains(t, logs, failure, "unexpected ancillary error") } @@ -125,14 +136,8 @@ func assertSPCompleteness(t *testing.T, output, service, details, logs string) { warnings, failedDetails = len(types), 1 case "late-page": warnings, failedScopes = len(types), 1 - case "failed-type": - if service == "savingsplans" { - survivors = types[:3] - } else { - survivors = nil - } } - if details == "empty" || details == "all-invalid" || details == "api-error" { + if details == "empty" || details == "all-invalid" { survivors = nil } require.Equal(t, warnings, strings.Count(logs, "incomplete AWS recommendations:"), logs) @@ -143,15 +148,8 @@ func assertSPCompleteness(t *testing.T, output, service, details, logs string) { require.Contains(t, logs, "not-a-number") } } - failures := 0 - switch details { - case "api-error": - failures = len(types) - case "failed-type": - failures = 1 - } - require.Equal(t, failures, strings.Count(logs, "Failed to fetch recommendations:"), logs) - if failures > 0 || details == "late-page" { + require.NotContains(t, logs, "Failed to fetch recommendations:") + if details == "late-page" { require.Contains(t, logs, "fixture denied recommendations") } require.NotContains(t, logs, "Region discovery incomplete") @@ -203,12 +201,8 @@ func assertCompletenessDiagnostics(t *testing.T, regionMode, details, stdout, st } else { require.NotContains(t, stdout, "incomplete AWS recommendations") } - if details == "api-error" { - require.Contains(t, stdout+stderr, "fixture denied recommendations") - } else { - require.NotContains(t, stdout+stderr, "Failed to fetch recommendations", "partial collection treated as fatal") - require.NotContains(t, stdout+stderr, "failed to discover regions", "partial collection treated as fatal") - } + require.NotContains(t, stdout+stderr, "Failed to fetch recommendations", "partial collection treated as fatal") + require.NotContains(t, stdout+stderr, "failed to discover regions", "partial collection treated as fatal") } func assertCompletenessCSV(t *testing.T, output, details, logs string) { @@ -262,14 +256,16 @@ func TestRecommendationCompletenessHelpers(t *testing.T) { client.On("GetRecommendationsForService", ctx, common.ServiceRDS).Return(tc.recs, tc.err) client.On("GetRecommendations", ctx, mock.Anything).Return(tc.recs, tc.err) regions, err := discoverRegionsForService(ctx, client, common.ServiceRDS) - got := fetchRecommendationsForRegion(ctx, client, common.ServiceRDS, "us-east-1", Config{TermYears: 1}) + got, fetchErr := fetchRecommendationsForRegion(ctx, client, common.ServiceRDS, "us-east-1", Config{TermYears: 1}) if !tc.keep { require.ErrorIs(t, err, tc.err) + require.ErrorIs(t, fetchErr, tc.err) require.Nil(t, regions) require.Nil(t, got) require.NotContains(t, output.String(), "incomplete AWS recommendations") } else { require.NoError(t, err) + require.NoError(t, fetchErr) require.Equal(t, tc.recs, got) require.Len(t, regions, len(tc.recs)) require.Equal(t, 2, strings.Count(output.String(), "2 failed details, 1 failed scopes")) diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index d2b804c8e..fcb28beea 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -419,7 +419,8 @@ func TestFetchRecommendationsForRegion_SavingsPlanDoesNotFilterOnEndpointRegion( Run(func(args mock.Arguments) { got = args.Get(1).(*common.RecommendationParams) }). Return([]common.Recommendation{}, nil) - fetchRecommendationsForRegion(context.Background(), client, tc.service, spAPIRegion, Config{TermYears: 1}) + _, err := fetchRecommendationsForRegion(context.Background(), client, tc.service, spAPIRegion, Config{TermYears: 1}) + require.NoError(t, err) require.NotNil(t, got, tc.service) assert.Equal(t, tc.wantRegion, got.Region, tc.service) diff --git a/cmd/savings_plan_region_test.go b/cmd/savings_plan_region_test.go index d13725aff..a9f42b2dd 100644 --- a/cmd/savings_plan_region_test.go +++ b/cmd/savings_plan_region_test.go @@ -32,7 +32,8 @@ func TestFetchRecommendationsForRegion_SavingsPlanRowsCarryDetailsRegion(t *test client.On("GetRecommendations", mock.Anything, mock.Anything). Return([]common.Recommendation{ec2InstanceSPRow(), compute}, nil) - got := fetchRecommendationsForRegion(context.Background(), client, common.ServiceSavingsPlansEC2Instance, spAPIRegion, Config{TermYears: 1}) + got, err := fetchRecommendationsForRegion(context.Background(), client, common.ServiceSavingsPlansEC2Instance, spAPIRegion, Config{TermYears: 1}) + require.NoError(t, err) require.Len(t, got, 2) assert.Equal(t, "eu-west-2", got[0].Region) diff --git a/cmd/sp_account_scope_test.go b/cmd/sp_account_scope_test.go index 667b514af..9c309bf23 100644 --- a/cmd/sp_account_scope_test.go +++ b/cmd/sp_account_scope_test.go @@ -33,8 +33,9 @@ func TestFetchRecommendationsForRegion_PassesSPAccountScope(t *testing.T) { Run(func(args mock.Arguments) { got = args.Get(1).(*common.RecommendationParams) }). Return([]common.Recommendation{}, nil) - fetchRecommendationsForRegion(context.Background(), client, common.ServiceSavingsPlansCompute, spAPIRegion, + _, err := fetchRecommendationsForRegion(context.Background(), client, common.ServiceSavingsPlansCompute, spAPIRegion, Config{TermYears: 1, SPAccountScope: "payer"}) + require.NoError(t, err) require.NotNil(t, got) assert.Equal(t, "payer", got.SPAccountScope)