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.