From 42011fb081029bbb845e20af45a187453b2e7bd9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 17:39:42 +0200 Subject: [PATCH] fix(cli): confirm CSV purchases once for the whole run, not per region In --input-csv mode the purchase confirmation lived inside processPurchaseLoop and was asked once per (service, region), showing only that region's totals, and declining canceled only that region before the loop prompted again for the next one. The CSV path also had no SIGINT handling. prepareCSVPurchaseRun now confirms once against the full post-filter set, after the audit-log writability check and CSV read and before the AWS config, service clients or duplicate checks. A decline aborts the whole run, writes no report and no audit record (matching the non-CSV path), and a non-interactive run without confirmation still fails closed. confirmPurchaseRun is shared with runPurchaseAndReport, and registerShutdownSignalHandler arms SIGINT on both entry points, with shutdown checks at the service, region and recommendation levels. The per-region prompt, createCancelledResults and the test that only exercised SkipConfirmation=true are removed. Closes #1610 --- cmd/multi_service.go | 179 +++++++++++++++++++----------- cmd/multi_service_helpers.go | 15 --- cmd/multi_service_helpers_test.go | 28 ----- cmd/multi_service_test.go | 107 ++++++++++-------- 4 files changed, 176 insertions(+), 153 deletions(-) diff --git a/cmd/multi_service.go b/cmd/multi_service.go index edc20fa15..9729092a0 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -89,6 +89,19 @@ func coverageFetchFailure(cfg Config, err error) error { // shutdownRequested is set to true when SIGINT is received during a purchase run. var shutdownRequested atomic.Bool +// registerShutdownSignalHandler arms shutdownRequested for the duration of a +// purchase run and returns the cleanup func the caller must defer (e.g. +// `defer registerShutdownSignalHandler()()`). Shared by both purchase entry +// points -- runToolMultiService and runToolFromCSV -- so an in-flight run on +// either path can be stopped cleanly between purchases with Ctrl-C. +func registerShutdownSignalHandler() func() { + shutdownRequested.Store(false) + sigCh := make(chan os.Signal, 1) + signal.Notify(sigCh, os.Interrupt) + go func() { <-sigCh; shutdownRequested.Store(true) }() + return func() { signal.Stop(sigCh) } +} + // effectiveDryRun reports whether the run must stay in dry-run mode. A run is // dry-run unless the user opts into real purchases with --purchase; that single // flag is the only control. It defaults to false, so a bare invocation is a @@ -116,11 +129,7 @@ func runToolMultiService(ctx context.Context, cfg Config) { isDryRun := effectiveDryRun(cfg) // Register SIGINT handler so a running purchase loop can be interrupted cleanly. - shutdownRequested.Store(false) - sigCh := make(chan os.Signal, 1) - signal.Notify(sigCh, os.Interrupt) - go func() { <-sigCh; shutdownRequested.Store(true) }() - defer signal.Stop(sigCh) + defer registerShutdownSignalHandler()() // Verify the audit log and its immediate parents before making cloud API calls. if err := CheckAuditLogWritable(cfg.AuditLog); err != nil { @@ -176,13 +185,10 @@ func runToolMultiService(ctx context.Context, cfg Config) { // runToolMultiService within the cyclomatic-complexity limit. func runPurchaseAndReport(ctx context.Context, awsCfg aws.Config, scoredResult scorer.ScoredResult, isDryRun bool, cfg Config, drops *common.DropSummary) { runID := uuid.New().String() - if !isDryRun { - totalInstances, totalSavings := sumPassedRecs(scoredResult.Passed) - if !ConfirmPurchase(totalInstances, totalSavings, cfg.SkipConfirmation) { - printDropSummary(drops) - AppLogger.Printf("\n❌ Purchase canceled.\n") - return - } + if !confirmPurchaseRun(scoredResult.Passed, isDryRun, cfg) { + printDropSummary(drops) + AppLogger.Printf("\n❌ Purchase canceled.\n") + return } allResults := executePurchasePipeline(ctx, awsCfg, scoredResult.Passed, isDryRun, runID, cfg) @@ -191,6 +197,21 @@ func runPurchaseAndReport(ctx context.Context, awsCfg aws.Config, scoredResult s writeReportAndSummary(scoredResult.Passed, allResults, isDryRun, cfg, drops) } +// confirmPurchaseRun asks for confirmation once against the full +// recommendation set on a real purchase run, and reports whether the run +// should proceed. Always true on a dry run (nothing is bought, so there is +// nothing to confirm). Shared by the non-CSV pipeline (runPurchaseAndReport) +// and the --input-csv path (runToolFromCSV) so both entry points show the +// operator the total they are actually authorizing and require exactly one +// confirmation per invocation. +func confirmPurchaseRun(recs []common.Recommendation, isDryRun bool, cfg Config) bool { + if isDryRun { + return true + } + totalInstances, totalSavings := sumPassedRecs(recs) + return ConfirmPurchase(totalInstances, totalSavings, cfg.SkipConfirmation) +} + // writeReportAndSummary writes the CSV report and prints the final summary. func writeReportAndSummary(passed []common.Recommendation, allResults []common.PurchaseResult, isDryRun bool, cfg Config, drops *common.DropSummary) { serviceStats := buildServiceStats(passed, allResults) @@ -513,8 +534,9 @@ func runCSVPathOrFatal(ctx context.Context, cfg Config) { // prepareCSVPurchaseRun validates and loads everything runToolFromCSV needs // before the per-service purchase loop: the audit log writability, the CSV -// file, filtering/sizing, and the AWS config. Extracted to keep -// runToolFromCSV under the project's gocyclo budget. +// file, filtering/sizing, the single run-wide purchase confirmation, and the +// AWS config. Extracted to keep runToolFromCSV under the project's gocyclo +// budget. // // The audit-log check runs first and before any cloud API call, matching the // non-CSV path (CheckAuditLogWritable in runToolMultiService). Before #1609 @@ -522,9 +544,14 @@ func runCSVPathOrFatal(ctx context.Context, cfg Config) { // reach real purchase calls with no way to have written a durable, // per-recommendation audit record even in principle. // -// A nil recs with a nil error means "nothing to process after filtering", -// which the caller treats as success rather than an error. -func prepareCSVPurchaseRun(ctx context.Context, cfg Config, csvModeCoverage float64) (recs []common.Recommendation, awsCfg aws.Config, runID string, err error) { +// The confirmation runs once against the full post-filter set, as the +// non-CSV path does (confirmPurchaseRun, shared with runPurchaseAndReport). +// Before #1610 it was asked once per (service, region) with only that +// region's totals, and declining canceled only that region. +// +// A nil recs with a nil error means there is nothing to do (already logged): +// filtering left no recommendations, or the user declined the confirmation. +func prepareCSVPurchaseRun(ctx context.Context, cfg Config, csvModeCoverage float64, isDryRun bool) (recs []common.Recommendation, awsCfg aws.Config, runID string, err error) { if err = CheckAuditLogWritable(cfg.AuditLog); err != nil { return nil, aws.Config{}, "", fmt.Errorf("cannot write audit log: %w", err) } @@ -541,6 +568,11 @@ func prepareCSVPurchaseRun(ctx context.Context, cfg Config, csvModeCoverage floa return nil, aws.Config{}, "", err } if len(recs) == 0 { + AppLogger.Println("⚠️ No recommendations to process after filtering") + return nil, aws.Config{}, "", nil + } + if !confirmPurchaseRun(recs, isDryRun, cfg) { + AppLogger.Printf("\n❌ Purchase canceled.\n") return nil, aws.Config{}, "", nil } @@ -559,14 +591,20 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { isDryRun := effectiveDryRun(cfg) printRunMode(isDryRun) + // Register SIGINT handler so an in-flight purchase run can be stopped + // cleanly between regions, matching the non-CSV path + // (runToolMultiService). Before #1610 this path had no SIGINT handling + // at all: runToolMultiService registers it only on the non-CSV branch, + // in code unreachable from CSV mode (the CSV branch returns first). + defer registerShutdownSignalHandler()() + csvModeCoverage := determineCSVCoverage(cfg) - recs, awsCfg, runID, err := prepareCSVPurchaseRun(ctx, cfg, csvModeCoverage) + recs, awsCfg, runID, err := prepareCSVPurchaseRun(ctx, cfg, csvModeCoverage, isDryRun) if err != nil { return err } if len(recs) == 0 { - AppLogger.Println("⚠️ No recommendations to process after filtering") return nil } @@ -588,6 +626,11 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { allAdjustedRecs := make([]common.Recommendation, 0) for service, regionRecs := range recsByServiceRegion { + if shutdownRequested.Load() { + log.Printf("Shutdown requested; stopping before %s", getServiceDisplayName(service)) + break + } + // Reset service results for each service serviceResults = serviceResults[:0] @@ -597,37 +640,20 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { serviceRecs := make([]common.Recommendation, 0) for region, recs := range regionRecs { - AppLogger.Printf("\n 📍 Region: %s (%d recommendations)\n", region, len(recs)) - - // Get service client for this region - regionalCfg := awsCfg.Copy() - regionalCfg.Region = region - serviceClient := createServiceClient(service, regionalCfg) - - if serviceClient == nil { - AppLogger.Printf(" ⚠️ Service client not yet implemented for %s\n", getServiceDisplayName(service)) - AppLogger.Printf(" (Skipping purchase phase for this service)\n") - continue + if shutdownRequested.Load() { + log.Printf("Shutdown requested; skipping remaining regions for %s", getServiceDisplayName(service)) + break } - // Check for duplicate RIs to avoid double purchasing. - adjustedRecs, ok := checkDuplicatesForCSVRegion(ctx, recs, serviceClient, service, region, isDryRun) + AppLogger.Printf("\n 📍 Region: %s (%d recommendations)\n", region, len(recs)) + + processedRecs, regionResults, ok := processCSVRegionPurchases(ctx, awsCfg, service, region, recs, isDryRun, cfg, runID) if !ok { continue } - // Deducting existing commitments shrinks Count, which can push a - // row that cleared the floor in filterAndAdjustRecommendations back - // under it (--min-count 5, a row of 6, and 5 matching recent - // commitments would otherwise be purchased at 1). --min-count is a - // floor on what gets bought, so it is re-applied to whatever the - // deduction left, not only to the pre-deduction counts. - recs = applyMinCountFloor(adjustedRecs, cfg.MinCount) - - serviceRecs = append(serviceRecs, recs...) - allAdjustedRecs = append(allAdjustedRecs, recs...) - - // Process purchases for this region - regionResults := processPurchaseLoop(ctx, recs, region, isDryRun, serviceClient, cfg, runID) + + serviceRecs = append(serviceRecs, processedRecs...) + allAdjustedRecs = append(allAdjustedRecs, processedRecs...) serviceResults = append(serviceResults, regionResults...) } @@ -656,6 +682,42 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { return nil } +// processCSVRegionPurchases handles a single (service, region) pair within +// the --input-csv purchase loop: builds the regional service client, runs +// the duplicate check, applies the --min-count floor to whatever the +// deduction left, and executes the purchase loop. ok=false means there is +// nothing to add for this region (no service client yet, or the duplicate +// check refused it) and the caller should move on to the next region. +// Extracted out of runToolFromCSV to keep it under the project's gocyclo +// budget. +func processCSVRegionPurchases(ctx context.Context, awsCfg aws.Config, service common.ServiceType, region string, recs []common.Recommendation, isDryRun bool, cfg Config, runID string) (processedRecs []common.Recommendation, results []common.PurchaseResult, ok bool) { + regionalCfg := awsCfg.Copy() + regionalCfg.Region = region + serviceClient := createServiceClient(service, regionalCfg) + + if serviceClient == nil { + AppLogger.Printf(" ⚠️ Service client not yet implemented for %s\n", getServiceDisplayName(service)) + AppLogger.Printf(" (Skipping purchase phase for this service)\n") + return nil, nil, false + } + + // Check for duplicate RIs to avoid double purchasing. + adjustedRecs, dedupOK := checkDuplicatesForCSVRegion(ctx, recs, serviceClient, service, region, isDryRun) + if !dedupOK { + return nil, nil, false + } + // Deducting existing commitments shrinks Count, which can push a row + // that cleared the floor in filterAndAdjustRecommendations back under it + // (--min-count 5, a row of 6, and 5 matching recent commitments would + // otherwise be purchased at 1). --min-count is a floor on what gets + // bought, so it is re-applied to whatever the deduction left, not only + // to the pre-deduction counts. + processedRecs = applyMinCountFloor(adjustedRecs, cfg.MinCount) + + results = processPurchaseLoop(ctx, processedRecs, region, isDryRun, serviceClient, cfg, runID) + return processedRecs, results, true +} + // checkDuplicatesForCSVRegion runs the duplicate check for a single // (service, region) pair in CSV mode and reports whether the caller should // still process that region (ok). The duplicate check is the only guard @@ -770,11 +832,18 @@ func processService(ctx context.Context, awsCfg aws.Config, recClient provider.R // processPurchaseLoop processes purchases for a single region (used by CSV // mode). runID groups every recommendation processed across the whole CSV // run into one audit trail, matching how executePurchasePipeline (the main -// pipeline) generates one runID per invocation. +// pipeline) generates one runID per invocation. Confirmation is not asked +// here: prepareCSVPurchaseRun confirms once for the whole run before this +// loop is reached. func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, region string, isDryRun bool, serviceClient provider.ServiceClient, cfg Config, runID string) []common.PurchaseResult { results := make([]common.PurchaseResult, 0, len(recs)) for j := range recs { + if shutdownRequested.Load() { + log.Printf("Shutdown requested; skipping %d remaining recommendation(s) in %s", len(recs)-j, region) + break + } + rec := recs[j] AppLogger.Printf(" [%d/%d] Processing: %s %s\n", j+1, len(recs), rec.Service, rec.ResourceType) AppLogger.Printf(" 💳 Purchasing %d instances\n", rec.Count) @@ -785,24 +854,6 @@ func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, regi result = createDryRunResult(rec, region, j+1, cfg) status = "skipped" } else { - // Ask for confirmation before proceeding with purchases (only on first item) - if j == 0 { - totalInstances := CalculateTotalInstances(recs) - totalSavings := 0.0 - for _rvc := range recs { - r := recs[_rvc] - totalSavings += r.EstimatedSavings - } - - if !ConfirmPurchase(totalInstances, totalSavings, cfg.SkipConfirmation) { - // User canceled - return canceled results for all. No audit - // record is written for a declined run, matching the - // non-CSV path: runPurchaseAndReport returns before ever - // calling executePurchasePipeline when the user declines. - return createCancelledResults(recs, region, cfg) - } - } - // Execute actual purchase result = executePurchase(ctx, rec, region, j+1, serviceClient, cfg) status = purchaseAuditStatus(result) diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index b537fdfcb..5d060706e 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -229,21 +229,6 @@ func createDryRunResult(rec common.Recommendation, region string, index int, cfg } } -// createCancelledResults creates purchase results for canceled purchases. -func createCancelledResults(recs []common.Recommendation, region string, cfg Config) []common.PurchaseResult { - results := make([]common.PurchaseResult, len(recs)) - for k := range recs { - results[k] = common.PurchaseResult{ - Recommendation: recs[k], - Success: false, - CommitmentID: generatePurchaseID(recs[k], region, k+1, false, effectiveSizingPct(cfg)), - Error: fmt.Errorf("purchase canceled by user"), - Timestamp: time.Now(), - } - } - return results -} - // executePurchase executes an actual RI purchase. func executePurchase(ctx context.Context, rec common.Recommendation, region string, index int, serviceClient provider.ServiceClient, cfg Config) common.PurchaseResult { AppLogger.Printf(" ⚠️ ACTUAL PURCHASE: About to buy %d instances of %s\n", rec.Count, rec.ResourceType) diff --git a/cmd/multi_service_helpers_test.go b/cmd/multi_service_helpers_test.go index 16728df75..af0cfdd9f 100644 --- a/cmd/multi_service_helpers_test.go +++ b/cmd/multi_service_helpers_test.go @@ -314,34 +314,6 @@ func TestCreateDryRunResult(t *testing.T) { assert.NotEmpty(t, result.Timestamp) } -func TestCreateCancelledResults(t *testing.T) { - // Save original values - origCfg := toolCfg - - defer func() { - toolCfg = origCfg - }() - - toolCfg.Coverage = 80.0 - - recs := []common.Recommendation{ - {Service: common.ServiceRDS, ResourceType: "db.t3.small", Count: 2}, - {Service: common.ServiceRDS, ResourceType: "db.t3.medium", Count: 3}, - {Service: common.ServiceRDS, ResourceType: "db.t3.large", Count: 1}, - } - - results := createCancelledResults(recs, "us-west-2", toolCfg) - - assert.Len(t, results, 3) - for i, result := range results { - assert.False(t, result.Success) - assert.Equal(t, recs[i], result.Recommendation) - assert.NotNil(t, result.Error) - assert.Contains(t, result.Error.Error(), "canceled") - assert.Contains(t, result.CommitmentID, "us-west-2") - } -} - func TestExecutePurchase(t *testing.T) { ctx := context.Background() // Save original values diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index c774ad032..4da38b760 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -1293,50 +1293,6 @@ func TestProcessPurchaseLoopPurchaseFailure(t *testing.T) { mockClient.AssertExpectations(t) } -func TestProcessPurchaseLoopUserCancellation(t *testing.T) { - ctx := context.Background() - origCfg := toolCfg - defer func() { toolCfg = origCfg }() - - toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") - toolCfg.Coverage = 90.0 - toolCfg.SkipConfirmation = false // User will be prompted - - recs := []common.Recommendation{ - {Service: common.ServiceEC2, ResourceType: "m5.large", Count: 10, EstimatedSavings: 5000}, - {Service: common.ServiceEC2, ResourceType: "m5.xlarge", Count: 5, EstimatedSavings: 3000}, - } - - mockClient := &MockServiceClient{} - // No expectations - should not be called if user cancels - - // Since we can't mock user input easily, we'll skip confirmation instead - // But the test verifies the cancellation logic is present - toolCfg.SkipConfirmation = true // Actually proceed for test - - // Setup mock to succeed - for _, rec := range recs { - result := common.PurchaseResult{ - Recommendation: rec, - Success: true, - CommitmentID: "test-id", - Timestamp: time.Now(), - } - mockClient.On("PurchaseCommitment", ctx, rec, mock.MatchedBy(func(o common.PurchaseOptions) bool { return o.Source == common.PurchaseSourceCLI })).Return(result, nil) - } - - t.Setenv("DISABLE_PURCHASE_DELAY", "true") - - results := processPurchaseLoop(ctx, recs, "eu-central-1", false, mockClient, toolCfg, "test-run") - - assert.Len(t, results, 2) - for _, result := range results { - assert.True(t, result.Success) - } - - mockClient.AssertExpectations(t) -} - func TestProcessPurchaseLoopEmptyRecommendations(t *testing.T) { ctx := context.Background() origCfg := toolCfg @@ -1549,14 +1505,15 @@ func TestProcessPurchaseLoopWithConfirmation(t *testing.T) { toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.Coverage = 80.0 - toolCfg.SkipConfirmation = true // Skip confirmation to proceed with purchase recs := []common.Recommendation{ {Service: common.ServiceRDS, ResourceType: "db.r5.large", Count: 5, SourceRecommendation: "Expensive", EstimatedSavings: 1000}, } mockClient := &MockServiceClient{} - // Mock the purchase since skipConfirmation=true will proceed + // processPurchaseLoop no longer confirms internally (confirmPurchaseRun + // runs once in runToolFromCSV before this loop is reached), so it always + // proceeds straight to the purchase call. result := common.PurchaseResult{ Recommendation: recs[0], Success: true, @@ -2076,6 +2033,64 @@ rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,123456789012 assert.Contains(t, err.Error(), "audit log") } +// TestRunToolFromCSV_ConfirmsOnceBeforeAnyRegion reproduces #1610 through the +// public --input-csv entry point. Before the fix, confirmation lived inside +// processPurchaseLoop and was asked once per (service, region), after that +// region's AWS config, service client and duplicate check; declining only +// canceled that region. Now prepareCSVPurchaseRun asks exactly once, against +// the whole post-filter set (both regions here: 5 instances), before any AWS +// call. stdin is not a terminal in tests, so ConfirmPurchase declines without +// blocking, which is the "operator declines" and non-interactive fail-closed +// path at once. +func TestRunToolFromCSV_ConfirmsOnceBeforeAnyRegion(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + isolateAWSEnv(t) + + var appOut, stdOut bytes.Buffer + oldLogger := AppLogger + AppLogger = log.New(&appOut, "", 0) + t.Cleanup(func() { AppLogger = oldLogger }) + oldWriter := log.Writer() + log.SetOutput(&stdOut) + t.Cleanup(func() { log.SetOutput(oldWriter) }) + + csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account +rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,123456789012 +rds,us-west-2,db.t3.medium,postgres,3,1yr,All Upfront,123456789012 +`) + reportPath := filepath.Join(t.TempDir(), "report.csv") + auditPath := filepath.Join(t.TempDir(), "audit.jsonl") + + toolCfg.CSVInput = csvPath + toolCfg.CSVOutput = reportPath + toolCfg.AuditLog = auditPath + toolCfg.ActualPurchase = true + toolCfg.SkipConfirmation = false + toolCfg.Coverage = 100.0 + toolCfg.TargetCoverage = 0 + toolCfg.MaxInstances = 0 + toolCfg.OverrideCount = 0 + + err := runToolFromCSV(context.Background(), toolCfg) + require.NoError(t, err) + + assert.Equal(t, 1, strings.Count(stdOut.String(), "stdin is not a terminal"), + "the purchase must be confirmed exactly once for the whole run, not per region") + assert.Contains(t, appOut.String(), "Purchase canceled.") + assert.NotContains(t, appOut.String(), "Region:", + "a declined confirmation must abort before any region is processed") + + _, statErr := os.Stat(reportPath) + assert.True(t, os.IsNotExist(statErr), "a declined run must write no purchase report") + data, readErr := os.ReadFile(auditPath) // #nosec G304 -- test-owned tempdir path + if readErr == nil { + assert.Empty(t, data, "a declined run attempts no purchase, so it writes no audit record") + } else { + assert.True(t, os.IsNotExist(readErr), "unexpected audit log read error: %v", readErr) + } +} + // ==================== Tests for adjustRecommendationForExcludedVersions ==================== // Helper to create test version info with extended support dates.