From 6b7ad637115c28dfce8d05b5a59314f5f3762ad6 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 18:02:59 +0100 Subject: [PATCH] fix(cmd): exit non-zero and count Savings Plan failures after a purchase run A real purchase run returned nil from executeAndReport and runCSVPurchases whenever the audit trail was intact, so the CLI exited 0 even when every purchase failed. The final summary also built its success rate and final message from the Reserved Instance aggregates only, so Savings Plan successes and failures were left out. Both purchase paths now return errPurchasesFailed (joined with any errAuditTrailLost) when a real run has a failed result, and the summary counts Savings Plan results alongside RI results, prints the succeeded/failed counts and warns when any purchase failed. Dry runs keep exiting 0. Closes #2157 Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service.go | 6 +- cmd/multi_service_stats.go | 46 ++++++- cmd/multi_service_stats_test.go | 2 +- cmd/purchase_failure_exit_2157_test.go | 176 +++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 9 deletions(-) create mode 100644 cmd/purchase_failure_exit_2157_test.go diff --git a/cmd/multi_service.go b/cmd/multi_service.go index d43a39bd1..90de40e00 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -266,7 +266,7 @@ func executeAndReport(ctx context.Context, awsCfg aws.Config, passed []common.Re if auditErr != nil { AppLogger.Print(auditStopMessage(len(allResults), len(passed), allResults, auditErr)) } - return auditErr + return errors.Join(auditErr, purchaseFailureError(allResults, isDryRun)) } // confirmPurchaseRun asks for confirmation once against the full @@ -762,7 +762,7 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { // runCSVPurchases groups the confirmed CSV rows by service and region, buys // them, and writes the report and summary for whatever ran. A non-nil error -// is errAuditTrailLost. +// is errAuditTrailLost and/or errPurchasesFailed. func runCSVPurchases(ctx context.Context, awsCfg aws.Config, recs []common.Recommendation, isDryRun bool, cfg Config, runID string) error { // Create account alias cache for lookup accountCache := NewAccountAliasCache(awsCfg) @@ -827,7 +827,7 @@ func runCSVPurchases(ctx context.Context, awsCfg aws.Config, recs []common.Recom if auditErr != nil { AppLogger.Print(auditStopMessage(len(allResults), len(recs), allResults, auditErr)) } - return auditErr + return errors.Join(auditErr, purchaseFailureError(allResults, isDryRun)) } // processCSVRegionPurchases handles a single (service, region) pair within diff --git a/cmd/multi_service_stats.go b/cmd/multi_service_stats.go index 52c6a3811..33bad3330 100644 --- a/cmd/multi_service_stats.go +++ b/cmd/multi_service_stats.go @@ -1,9 +1,35 @@ package main import ( + "errors" + "fmt" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" ) +// errPurchasesFailed marks a real purchase run in which at least one +// purchase failed. The run exits non-zero so a scripted caller can detect a +// partial or total failure (#2157). +var errPurchasesFailed = errors.New("purchases failed") + +// purchaseFailureError returns errPurchasesFailed, with counts, when a real +// run has any failed result. Dry runs buy nothing, so they never fail here. +func purchaseFailureError(results []common.PurchaseResult, isDryRun bool) error { + if isDryRun { + return nil + } + failed := 0 + for i := range results { + if !results[i].Success { + failed++ + } + } + if failed == 0 { + return nil + } + return fmt.Errorf("%w: %d of %d purchase(s) failed", errPurchasesFailed, failed, len(results)) +} + // ServiceProcessingStats holds statistics for each service. type ServiceProcessingStats struct { Service common.ServiceType @@ -73,8 +99,11 @@ func printMultiServiceSummary(allRecommendations []common.Recommendation, allRes printComparisonSection(allRecommendations, riStats, riAggregates.savings) } - printSuccessRate(riAggregates.success, riAggregates.failed) - printFinalMessage(isDryRun, riAggregates.success) + // Savings Plan results count alongside RI results (#2157). + success := riAggregates.success + spStats.SuccessfulPurchases + failed := riAggregates.failed + spStats.FailedPurchases + printSuccessRate(success, failed) + printFinalMessage(isDryRun, success, failed) } // riAggregateStats holds aggregated RI statistics. @@ -165,7 +194,8 @@ func printSuccessRate(success, failed int) { totalResults := success + failed if totalResults > 0 { successRate := (float64(success) / float64(totalResults)) * 100 - AppLogger.Printf("\nOverall success rate: %.1f%%\n", successRate) + AppLogger.Printf("\nPurchases: %d succeeded, %d failed\n", success, failed) + AppLogger.Printf("Overall success rate: %.1f%%\n", successRate) } } @@ -178,10 +208,16 @@ func printSuccessRate(success, failed int) { const archeraSignupURL = common.ArcheraSignupURL // printFinalMessage prints the final message based on mode and results. -func printFinalMessage(isDryRun bool, riSuccess int) { +// success and failed cover both Reserved Instances and Savings Plans. +func printFinalMessage(isDryRun bool, success, failed int) { if isDryRun { AppLogger.Println("\n💡 To actually purchase these commitments, run with --purchase flag") - } else if riSuccess > 0 { + return + } + if failed > 0 { + AppLogger.Printf("\n⚠️ %d of %d purchase(s) failed; see the per-purchase errors above\n", failed, success+failed) + } + if success > 0 { AppLogger.Println("\n🎉 Purchase operations completed!") AppLogger.Println("⏰ Allow up to 15 minutes for RIs to appear in your account") printArcheraPitch() diff --git a/cmd/multi_service_stats_test.go b/cmd/multi_service_stats_test.go index 5ce253782..38f735243 100644 --- a/cmd/multi_service_stats_test.go +++ b/cmd/multi_service_stats_test.go @@ -559,7 +559,7 @@ func TestPrintFinalMessage(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { output := captureAppOutput(t, func() { - printFinalMessage(tt.isDryRun, tt.riSuccess) + printFinalMessage(tt.isDryRun, tt.riSuccess, 0) }) for _, want := range tt.wantOutput { diff --git a/cmd/purchase_failure_exit_2157_test.go b/cmd/purchase_failure_exit_2157_test.go new file mode 100644 index 000000000..ab39cf37b --- /dev/null +++ b/cmd/purchase_failure_exit_2157_test.go @@ -0,0 +1,176 @@ +package main + +import ( + "context" + "io" + "net/http" + "strings" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// spHookFailing answers the Savings Plan calls like spHook, but fails the +// CreateSavingsPlan call for the given offering IDs. Every other call (the +// EC2 RI API included) gets a 400, so an RI purchase through it fails. +func spHookFailing(failOfferings ...string) *hookTransport { + h := &hookTransport{} + h.handle = func(r *http.Request) (int, string) { + switch { + case strings.HasSuffix(r.URL.Path, "/CreateSavingsPlan"): + body, _ := io.ReadAll(r.Body) + for _, id := range failOfferings { + if strings.Contains(string(body), `"`+id+`"`) { + return http.StatusBadRequest, `{"__type":"ValidationException","message":"offering rejected"}` + } + } + return http.StatusOK, `{"savingsPlanId":"sp-111"}` + case strings.HasSuffix(r.URL.Path, "/DescribeSavingsPlans"): + return http.StatusOK, `{"savingsPlans":[]}` + } + return http.StatusBadRequest, `{"__type":"ValidationException","message":"unexpected call"}` + } + return h +} + +func TestExecuteAndReport_SavingsPlanFailureFailsRun(t *testing.T) { + t.Setenv("DISABLE_PURCHASE_DELAY", "true") + out := captureAppLog(t) + cfg := auditCfg(t) + h := spHookFailing("o-2") + recs := []common.Recommendation{offeringSP("o-1", 1), offeringSP("o-2", 2)} + + err := executeAndReport(context.Background(), hookedAWSConfig(h), recs, false, "run-1", cfg, nil) + + require.ErrorIs(t, err, errPurchasesFailed) + assert.NotErrorIs(t, err, errAuditTrailLost) + assert.Contains(t, err.Error(), "1 of 2") + assert.Equal(t, 2, h.count("/CreateSavingsPlan")) + assert.Contains(t, out.String(), "Purchases: 1 succeeded, 1 failed") + assert.Contains(t, out.String(), "1 of 2 purchase(s) failed") +} + +func TestExecuteAndReport_AllSavingsPlansFailFailsRun(t *testing.T) { + t.Setenv("DISABLE_PURCHASE_DELAY", "true") + out := captureAppLog(t) + cfg := auditCfg(t) + h := spHookFailing("o-1", "o-2") + recs := []common.Recommendation{offeringSP("o-1", 1), offeringSP("o-2", 2)} + + err := executeAndReport(context.Background(), hookedAWSConfig(h), recs, false, "run-1", cfg, nil) + + require.ErrorIs(t, err, errPurchasesFailed) + assert.Contains(t, out.String(), "Purchases: 0 succeeded, 2 failed") + assert.NotContains(t, out.String(), "Purchase operations completed") +} + +func TestExecuteAndReport_SavingsPlanAndRIFailuresCountedTogether(t *testing.T) { + t.Setenv("DISABLE_PURCHASE_DELAY", "true") + out := captureAppLog(t) + cfg := auditCfg(t) + h := spHookFailing("o-2") + recs := []common.Recommendation{offeringSP("o-1", 1), offeringSP("o-2", 2), validEC2Rec()} + + err := executeAndReport(context.Background(), hookedAWSConfig(h), recs, false, "run-1", cfg, nil) + + require.ErrorIs(t, err, errPurchasesFailed) + assert.Contains(t, err.Error(), "2 of 3") + assert.Contains(t, out.String(), "Purchases: 1 succeeded, 2 failed") + assert.Contains(t, out.String(), "Overall success rate: 33.3%") +} + +func TestExecuteAndReport_AllSavingsPlansSucceedReturnsNil(t *testing.T) { + t.Setenv("DISABLE_PURCHASE_DELAY", "true") + out := captureAppLog(t) + cfg := auditCfg(t) + h := spHookFailing() + recs := []common.Recommendation{offeringSP("o-1", 1), offeringSP("o-2", 2)} + + err := executeAndReport(context.Background(), hookedAWSConfig(h), recs, false, "run-1", cfg, nil) + + require.NoError(t, err) + assert.Contains(t, out.String(), "Purchases: 2 succeeded, 0 failed") + assert.Contains(t, out.String(), "Purchase operations completed") + assert.NotContains(t, out.String(), "purchase(s) failed") +} + +func TestExecuteAndReport_DryRunPreconditionFailureReturnsNil(t *testing.T) { + out := captureAppLog(t) + cfg := auditCfg(t) + h := spHookFailing() + bad := offeringSP("o-1", 1) + bad.Details = nil // fails the purchase preconditions + + err := executeAndReport(context.Background(), hookedAWSConfig(h), []common.Recommendation{bad}, true, "run-1", cfg, nil) + + require.NoError(t, err, "a dry run buys nothing, so it does not fail the run") + assert.Zero(t, h.count("/CreateSavingsPlan")) + assert.Contains(t, out.String(), "cannot purchase", "the precondition failure is still reported") +} + +func TestRunCSVPurchases_SavingsPlanFailureFailsRun(t *testing.T) { + t.Setenv("DISABLE_PURCHASE_DELAY", "true") + out := captureAppLog(t) + cfg := auditCfg(t) + h := spHookFailing("o-2") + recs := []common.Recommendation{offeringSP("o-1", 1), offeringSP("o-2", 2)} + + err := runCSVPurchases(context.Background(), hookedAWSConfig(h), recs, false, cfg, "run-1") + + require.ErrorIs(t, err, errPurchasesFailed) + assert.Contains(t, err.Error(), "1 of 2") + assert.Contains(t, out.String(), "Purchases: 1 succeeded, 1 failed") +} + +func TestRunCSVPurchases_AllSucceedReturnsNil(t *testing.T) { + t.Setenv("DISABLE_PURCHASE_DELAY", "true") + out := captureAppLog(t) + cfg := auditCfg(t) + h := spHookFailing() + recs := []common.Recommendation{offeringSP("o-1", 1), offeringSP("o-2", 2)} + + err := runCSVPurchases(context.Background(), hookedAWSConfig(h), recs, false, cfg, "run-1") + + require.NoError(t, err) + assert.Contains(t, out.String(), "Purchases: 2 succeeded, 0 failed") +} + +func TestPrintMultiServiceSummary_CountsSavingsPlanFailures(t *testing.T) { + stats := map[common.ServiceType]ServiceProcessingStats{ + common.ServiceEC2: {Service: common.ServiceEC2, RecommendationsSelected: 2, SuccessfulPurchases: 1, FailedPurchases: 1}, + common.ServiceSavingsPlansCompute: {Service: common.ServiceSavingsPlansCompute, RecommendationsSelected: 2, FailedPurchases: 2}, + common.ServiceSavingsPlansEC2Instance: {Service: common.ServiceSavingsPlansEC2Instance, RecommendationsSelected: 1, SuccessfulPurchases: 1}, + } + + output := captureAppOutput(t, func() { + printMultiServiceSummary(nil, nil, stats, false) + }) + + assert.Contains(t, output, "Purchases: 2 succeeded, 3 failed") + assert.Contains(t, output, "Overall success rate: 40.0%") + assert.Contains(t, output, "3 of 5 purchase(s) failed") +} + +func TestPrintFinalMessage_SavingsPlanOnlySuccessShowsCompletion(t *testing.T) { + output := captureAppOutput(t, func() { + printMultiServiceSummary(nil, nil, map[common.ServiceType]ServiceProcessingStats{ + common.ServiceSavingsPlansCompute: {Service: common.ServiceSavingsPlansCompute, RecommendationsSelected: 1, SuccessfulPurchases: 1}, + }, false) + }) + + assert.Contains(t, output, "Purchase operations completed") + assert.NotContains(t, output, "purchase(s) failed") +} + +func TestPurchaseFailureError(t *testing.T) { + ok := common.PurchaseResult{Success: true} + bad := common.PurchaseResult{Success: false} + assert.NoError(t, purchaseFailureError(nil, false)) + assert.NoError(t, purchaseFailureError([]common.PurchaseResult{ok, ok}, false)) + assert.NoError(t, purchaseFailureError([]common.PurchaseResult{bad}, true), "dry run") + err := purchaseFailureError([]common.PurchaseResult{ok, bad, bad}, false) + require.ErrorIs(t, err, errPurchasesFailed) + assert.Contains(t, err.Error(), "2 of 3") +}