Skip to content

Commit 0ca106d

Browse files
committed
fix(cli): confirm CSV purchases once for the whole run, not per region
The --input-csv purchase confirmation was prompted once per (service, region) inside processPurchaseLoop, showing only that region's totals, and declining canceled only that region -- the outer service/region loops then continued and prompted again for the next region. An operator never saw the total they were actually authorizing, and "no" did not mean no. The SIGINT handler was also never registered on this path (runToolMultiService registers it only on the non-CSV branch, which the CSV branch returns before reaching). Moved confirmation to run once, in the new loadAndConfirmCSVPurchase, against the full post-filter recommendation set, before any per-region processing starts -- exactly matching the non-CSV path. A decline now aborts the whole run. Extracted confirmPurchaseRun as the shared sum+confirm logic between the non-CSV path (runPurchaseAndReport) and the CSV path, so both show the operator the same total and the non-CSV path's existing confirm/cancel-message ordering is unchanged (still covered by the existing TestRunPurchaseAndReportDeclinedConfirmationPrintsDropsBeforeCancellation). Registered the same SIGINT handler used by the non-CSV path (extracted into registerShutdownSignalHandler, shared by both) at the top of runToolFromCSV, and added shutdownRequested checks at each loop level (service, region, and per-recommendation inside processPurchaseLoop) so an in-flight run can be stopped cleanly between purchases. Removed createCancelledResults and its test: it existed only to produce per-region "canceled" results for the old per-region decline path, which no longer exists now that a decline is terminal before any region is reached. Extracted loadAndConfirmCSVPurchase and processCSVRegionPurchases out of runToolFromCSV, and registerShutdownSignalHandler out of the SIGINT-registration blocks, to keep runToolFromCSV under the project's gocyclo budget after adding the new confirm/shutdown branches. Regression tests: - TestLoadAndConfirmCSVPurchase_ConfirmsOnceAgainstFullSet calls the new function directly with a 2-region CSV and a declined confirmation (stdin is not a terminal in tests), asserting ok=false before any region is ever touched. - TestRunToolFromCSV_DeclineAbortsWholeRun confirms the effect at the public entry point: a declined confirmation produces no purchase report at all. Proved TestLoadAndConfirmCSVPurchase_ConfirmsOnceAgainstFullSet fails on the pre-fix code: loadAndConfirmCSVPurchase and confirmPurchaseRun did not exist before this change (confirmation lived inline inside processPurchaseLoop), so the equivalent call fails to compile against a worktree of the pre-fix branch base. Verification: go build ./cmd, go vet ./cmd/..., golangci-lint v2.10.1 (the exact CI pin) run --timeout=10m clean, gocyclo -over 10 clean, go mod tidy -diff empty, go test -race -short ./... all green. Closes #1610
1 parent 5036518 commit 0ca106d

4 files changed

Lines changed: 214 additions & 157 deletions

File tree

‎cmd/multi_service.go‎

Lines changed: 139 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,19 @@ func coverageFetchFailure(cfg Config, err error) error {
8989
// shutdownRequested is set to true when SIGINT is received during a purchase run.
9090
var shutdownRequested atomic.Bool
9191

92+
// registerShutdownSignalHandler arms shutdownRequested for the duration of a
93+
// purchase run and returns the cleanup func the caller must defer (e.g.
94+
// `defer registerShutdownSignalHandler()()`). Shared by both purchase entry
95+
// points -- runToolMultiService and runToolFromCSV -- so an in-flight run on
96+
// either path can be stopped cleanly between purchases with Ctrl-C.
97+
func registerShutdownSignalHandler() func() {
98+
shutdownRequested.Store(false)
99+
sigCh := make(chan os.Signal, 1)
100+
signal.Notify(sigCh, os.Interrupt)
101+
go func() { <-sigCh; shutdownRequested.Store(true) }()
102+
return func() { signal.Stop(sigCh) }
103+
}
104+
92105
// effectiveDryRun reports whether the run must stay in dry-run mode. A run is
93106
// dry-run unless the user opts into real purchases with --purchase; that single
94107
// 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) {
116129
isDryRun := effectiveDryRun(cfg)
117130

118131
// Register SIGINT handler so a running purchase loop can be interrupted cleanly.
119-
shutdownRequested.Store(false)
120-
sigCh := make(chan os.Signal, 1)
121-
signal.Notify(sigCh, os.Interrupt)
122-
go func() { <-sigCh; shutdownRequested.Store(true) }()
123-
defer signal.Stop(sigCh)
132+
defer registerShutdownSignalHandler()()
124133

125134
// Verify audit log is writable before making any cloud API calls.
126135
if err := CheckAuditLogWritable(cfg.AuditLog); err != nil {
@@ -176,13 +185,10 @@ func runToolMultiService(ctx context.Context, cfg Config) {
176185
// runToolMultiService within the cyclomatic-complexity limit.
177186
func runPurchaseAndReport(ctx context.Context, awsCfg aws.Config, scoredResult scorer.ScoredResult, isDryRun bool, cfg Config, drops *common.DropSummary) {
178187
runID := uuid.New().String()
179-
if !isDryRun {
180-
totalInstances, totalSavings := sumPassedRecs(scoredResult.Passed)
181-
if !ConfirmPurchase(totalInstances, totalSavings, cfg.SkipConfirmation) {
182-
printDropSummary(drops)
183-
AppLogger.Printf("\n❌ Purchase canceled.\n")
184-
return
185-
}
188+
if !confirmPurchaseRun(scoredResult.Passed, isDryRun, cfg) {
189+
printDropSummary(drops)
190+
AppLogger.Printf("\n❌ Purchase canceled.\n")
191+
return
186192
}
187193

188194
allResults := executePurchasePipeline(ctx, awsCfg, scoredResult.Passed, isDryRun, runID, cfg)
@@ -191,6 +197,21 @@ func runPurchaseAndReport(ctx context.Context, awsCfg aws.Config, scoredResult s
191197
writeReportAndSummary(scoredResult.Passed, allResults, isDryRun, cfg, drops)
192198
}
193199

200+
// confirmPurchaseRun asks for confirmation once against the full
201+
// recommendation set on a real purchase run, and reports whether the run
202+
// should proceed. Always true on a dry run (nothing is bought, so there is
203+
// nothing to confirm). Shared by the non-CSV pipeline (runPurchaseAndReport)
204+
// and the --input-csv path (runToolFromCSV) so both entry points show the
205+
// operator the total they are actually authorizing and require exactly one
206+
// confirmation per invocation.
207+
func confirmPurchaseRun(recs []common.Recommendation, isDryRun bool, cfg Config) bool {
208+
if isDryRun {
209+
return true
210+
}
211+
totalInstances, totalSavings := sumPassedRecs(recs)
212+
return ConfirmPurchase(totalInstances, totalSavings, cfg.SkipConfirmation)
213+
}
214+
194215
// writeReportAndSummary writes the CSV report and prints the final summary.
195216
func writeReportAndSummary(passed []common.Recommendation, allResults []common.PurchaseResult, isDryRun bool, cfg Config, drops *common.DropSummary) {
196217
serviceStats := buildServiceStats(passed, allResults)
@@ -489,33 +510,64 @@ func runCSVPathOrFatal(ctx context.Context, cfg Config) {
489510
}
490511
}
491512

513+
// loadAndConfirmCSVPurchase reads and filters the CSV, then confirms once
514+
// against the full post-filter recommendation set before any per-region
515+
// purchase loop runs -- exactly as the non-CSV path does (confirmPurchaseRun,
516+
// shared with runPurchaseAndReport), rather than once per (service, region)
517+
// with only that region's totals. Before #1610 an operator was prompted once
518+
// per region, each showing only that region's slice, and declining canceled
519+
// only that region -- the run then immediately prompted again for the next
520+
// region rather than treating "no" as terminal.
521+
//
522+
// Extracted out of runToolFromCSV to keep it under the project's gocyclo
523+
// budget. ok=false with a nil error means there is nothing to do (already
524+
// logged): either filtering left no recommendations, or the user declined.
525+
func loadAndConfirmCSVPurchase(cfg Config, csvModeCoverage float64, isDryRun bool) (recs []common.Recommendation, ok bool, err error) {
526+
AppLogger.Printf("📄 Reading recommendations from CSV: %s\n", cfg.CSVInput)
527+
recs, err = loadRecommendationsFromCSV(cfg.CSVInput)
528+
if err != nil {
529+
return nil, false, fmt.Errorf("failed to read CSV file: %w", err)
530+
}
531+
AppLogger.Printf("✅ Loaded %d recommendations from CSV\n", len(recs))
532+
533+
recs, err = filterAndAdjustRecommendations(recs, csvModeCoverage, cfg)
534+
if err != nil {
535+
return nil, false, err
536+
}
537+
if len(recs) == 0 {
538+
AppLogger.Println("⚠️ No recommendations to process after filtering")
539+
return nil, false, nil
540+
}
541+
542+
if !confirmPurchaseRun(recs, isDryRun, cfg) {
543+
AppLogger.Printf("\n❌ Purchase canceled.\n")
544+
return nil, false, nil
545+
}
546+
547+
return recs, true, nil
548+
}
549+
492550
// runToolFromCSV processes recommendations from a CSV input file.
493551
// It returns an error instead of exiting so the orchestration glue is
494552
// unit-testable; the caller (runCSVPathOrFatal) turns errors fatal.
495553
func runToolFromCSV(ctx context.Context, cfg Config) error {
496554
isDryRun := effectiveDryRun(cfg)
497555
printRunMode(isDryRun)
498556

499-
csvModeCoverage := determineCSVCoverage(cfg)
500-
501-
AppLogger.Printf("📄 Reading recommendations from CSV: %s\n", cfg.CSVInput)
557+
// Register SIGINT handler so an in-flight purchase run can be stopped
558+
// cleanly between regions, matching the non-CSV path
559+
// (runToolMultiService). Before #1610 this path had no SIGINT handling
560+
// at all: runToolMultiService registers it only on the non-CSV branch,
561+
// in code unreachable from CSV mode (the CSV branch returns first).
562+
defer registerShutdownSignalHandler()()
502563

503-
// Read recommendations from CSV
504-
recs, err := loadRecommendationsFromCSV(cfg.CSVInput)
505-
if err != nil {
506-
return fmt.Errorf("failed to read CSV file: %w", err)
507-
}
508-
509-
AppLogger.Printf("✅ Loaded %d recommendations from CSV\n", len(recs))
564+
csvModeCoverage := determineCSVCoverage(cfg)
510565

511-
// Filter and adjust recommendations
512-
recs, err = filterAndAdjustRecommendations(recs, csvModeCoverage, cfg)
566+
recs, ok, err := loadAndConfirmCSVPurchase(cfg, csvModeCoverage, isDryRun)
513567
if err != nil {
514568
return err
515569
}
516-
517-
if len(recs) == 0 {
518-
AppLogger.Println("⚠️ No recommendations to process after filtering")
570+
if !ok {
519571
return nil
520572
}
521573

@@ -542,6 +594,11 @@ func runToolFromCSV(ctx context.Context, cfg Config) error {
542594
allAdjustedRecs := make([]common.Recommendation, 0)
543595

544596
for service, regionRecs := range recsByServiceRegion {
597+
if shutdownRequested.Load() {
598+
log.Printf("Shutdown requested — stopping before %s", getServiceDisplayName(service))
599+
break
600+
}
601+
545602
// Reset service results for each service
546603
serviceResults = serviceResults[:0]
547604

@@ -551,37 +608,20 @@ func runToolFromCSV(ctx context.Context, cfg Config) error {
551608

552609
serviceRecs := make([]common.Recommendation, 0)
553610
for region, recs := range regionRecs {
554-
AppLogger.Printf("\n 📍 Region: %s (%d recommendations)\n", region, len(recs))
555-
556-
// Get service client for this region
557-
regionalCfg := awsCfg.Copy()
558-
regionalCfg.Region = region
559-
serviceClient := createServiceClient(service, regionalCfg)
560-
561-
if serviceClient == nil {
562-
AppLogger.Printf(" ⚠️ Service client not yet implemented for %s\n", getServiceDisplayName(service))
563-
AppLogger.Printf(" (Skipping purchase phase for this service)\n")
564-
continue
611+
if shutdownRequested.Load() {
612+
log.Printf("Shutdown requested — skipping remaining regions for %s", getServiceDisplayName(service))
613+
break
565614
}
566615

567-
// Check for duplicate RIs to avoid double purchasing.
568-
adjustedRecs, ok := checkDuplicatesForCSVRegion(ctx, recs, serviceClient, service, region, isDryRun)
616+
AppLogger.Printf("\n 📍 Region: %s (%d recommendations)\n", region, len(recs))
617+
618+
processedRecs, regionResults, ok := processCSVRegionPurchases(ctx, awsCfg, service, region, recs, isDryRun, cfg)
569619
if !ok {
570620
continue
571621
}
572-
// Deducting existing commitments shrinks Count, which can push a
573-
// row that cleared the floor in filterAndAdjustRecommendations back
574-
// under it (--min-count 5, a row of 6, and 5 matching recent
575-
// commitments would otherwise be purchased at 1). --min-count is a
576-
// floor on what gets bought, so it is re-applied to whatever the
577-
// deduction left, not only to the pre-deduction counts.
578-
recs = applyMinCountFloor(adjustedRecs, cfg.MinCount)
579-
580-
serviceRecs = append(serviceRecs, recs...)
581-
allAdjustedRecs = append(allAdjustedRecs, recs...)
582-
583-
// Process purchases for this region
584-
regionResults := processPurchaseLoop(ctx, recs, region, isDryRun, serviceClient, cfg)
622+
623+
serviceRecs = append(serviceRecs, processedRecs...)
624+
allAdjustedRecs = append(allAdjustedRecs, processedRecs...)
585625
serviceResults = append(serviceResults, regionResults...)
586626
}
587627

@@ -610,6 +650,42 @@ func runToolFromCSV(ctx context.Context, cfg Config) error {
610650
return nil
611651
}
612652

653+
// processCSVRegionPurchases handles a single (service, region) pair within
654+
// the --input-csv purchase loop: builds the regional service client, runs
655+
// the duplicate check, applies the --min-count floor to whatever the
656+
// deduction left, and executes the purchase loop. ok=false means there is
657+
// nothing to add for this region (no service client yet, or the duplicate
658+
// check refused it) and the caller should move on to the next region.
659+
// Extracted out of runToolFromCSV to keep it under the project's gocyclo
660+
// budget.
661+
func processCSVRegionPurchases(ctx context.Context, awsCfg aws.Config, service common.ServiceType, region string, recs []common.Recommendation, isDryRun bool, cfg Config) (processedRecs []common.Recommendation, results []common.PurchaseResult, ok bool) {
662+
regionalCfg := awsCfg.Copy()
663+
regionalCfg.Region = region
664+
serviceClient := createServiceClient(service, regionalCfg)
665+
666+
if serviceClient == nil {
667+
AppLogger.Printf(" ⚠️ Service client not yet implemented for %s\n", getServiceDisplayName(service))
668+
AppLogger.Printf(" (Skipping purchase phase for this service)\n")
669+
return nil, nil, false
670+
}
671+
672+
// Check for duplicate RIs to avoid double purchasing.
673+
adjustedRecs, dedupOK := checkDuplicatesForCSVRegion(ctx, recs, serviceClient, service, region, isDryRun)
674+
if !dedupOK {
675+
return nil, nil, false
676+
}
677+
// Deducting existing commitments shrinks Count, which can push a row
678+
// that cleared the floor in filterAndAdjustRecommendations back under it
679+
// (--min-count 5, a row of 6, and 5 matching recent commitments would
680+
// otherwise be purchased at 1). --min-count is a floor on what gets
681+
// bought, so it is re-applied to whatever the deduction left, not only
682+
// to the pre-deduction counts.
683+
processedRecs = applyMinCountFloor(adjustedRecs, cfg.MinCount)
684+
685+
results = processPurchaseLoop(ctx, processedRecs, region, isDryRun, serviceClient, cfg)
686+
return processedRecs, results, true
687+
}
688+
613689
// checkDuplicatesForCSVRegion runs the duplicate check for a single
614690
// (service, region) pair in CSV mode and reports whether the caller should
615691
// still process that region (ok). The duplicate check is the only guard
@@ -721,11 +797,20 @@ func processService(ctx context.Context, awsCfg aws.Config, recClient provider.R
721797
return serviceRecs, serviceResults
722798
}
723799

724-
// processPurchaseLoop processes purchases for a single region (used by CSV mode).
800+
// processPurchaseLoop processes purchases for a single region (used by CSV
801+
// mode). Confirmation is no longer asked here: runToolFromCSV confirms once
802+
// against the full run's recommendation set before this loop is ever
803+
// reached (see confirmPurchaseRun), so a decline aborts the whole run
804+
// instead of only the region processed first.
725805
func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, region string, isDryRun bool, serviceClient provider.ServiceClient, cfg Config) []common.PurchaseResult {
726806
results := make([]common.PurchaseResult, 0, len(recs))
727807

728808
for j := range recs {
809+
if shutdownRequested.Load() {
810+
log.Printf("Shutdown requested — skipping %d remaining recommendation(s) in %s", len(recs)-j, region)
811+
break
812+
}
813+
729814
rec := recs[j]
730815
AppLogger.Printf(" [%d/%d] Processing: %s %s\n", j+1, len(recs), rec.Service, rec.ResourceType)
731816
AppLogger.Printf(" 💳 Purchasing %d instances\n", rec.Count)
@@ -734,21 +819,6 @@ func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, regi
734819
if isDryRun {
735820
result = createDryRunResult(rec, region, j+1, cfg)
736821
} else {
737-
// Ask for confirmation before proceeding with purchases (only on first item)
738-
if j == 0 {
739-
totalInstances := CalculateTotalInstances(recs)
740-
totalSavings := 0.0
741-
for _rvc := range recs {
742-
r := recs[_rvc]
743-
totalSavings += r.EstimatedSavings
744-
}
745-
746-
if !ConfirmPurchase(totalInstances, totalSavings, cfg.SkipConfirmation) {
747-
// User canceled - return canceled results for all
748-
return createCancelledResults(recs, region, cfg)
749-
}
750-
}
751-
752822
// Execute actual purchase
753823
result = executePurchase(ctx, rec, region, j+1, serviceClient, cfg)
754824

‎cmd/multi_service_helpers.go‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -228,21 +228,6 @@ func createDryRunResult(rec common.Recommendation, region string, index int, cfg
228228
}
229229
}
230230

231-
// createCancelledResults creates purchase results for canceled purchases.
232-
func createCancelledResults(recs []common.Recommendation, region string, cfg Config) []common.PurchaseResult {
233-
results := make([]common.PurchaseResult, len(recs))
234-
for k := range recs {
235-
results[k] = common.PurchaseResult{
236-
Recommendation: recs[k],
237-
Success: false,
238-
CommitmentID: generatePurchaseID(recs[k], region, k+1, false, effectiveSizingPct(cfg)),
239-
Error: fmt.Errorf("purchase canceled by user"),
240-
Timestamp: time.Now(),
241-
}
242-
}
243-
return results
244-
}
245-
246231
// executePurchase executes an actual RI purchase.
247232
func executePurchase(ctx context.Context, rec common.Recommendation, region string, index int, serviceClient provider.ServiceClient, cfg Config) common.PurchaseResult {
248233
AppLogger.Printf(" ⚠️ ACTUAL PURCHASE: About to buy %d instances of %s\n", rec.Count, rec.ResourceType)

‎cmd/multi_service_helpers_test.go‎

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -314,34 +314,6 @@ func TestCreateDryRunResult(t *testing.T) {
314314
assert.NotEmpty(t, result.Timestamp)
315315
}
316316

317-
func TestCreateCancelledResults(t *testing.T) {
318-
// Save original values
319-
origCfg := toolCfg
320-
321-
defer func() {
322-
toolCfg = origCfg
323-
}()
324-
325-
toolCfg.Coverage = 80.0
326-
327-
recs := []common.Recommendation{
328-
{Service: common.ServiceRDS, ResourceType: "db.t3.small", Count: 2},
329-
{Service: common.ServiceRDS, ResourceType: "db.t3.medium", Count: 3},
330-
{Service: common.ServiceRDS, ResourceType: "db.t3.large", Count: 1},
331-
}
332-
333-
results := createCancelledResults(recs, "us-west-2", toolCfg)
334-
335-
assert.Len(t, results, 3)
336-
for i, result := range results {
337-
assert.False(t, result.Success)
338-
assert.Equal(t, recs[i], result.Recommendation)
339-
assert.NotNil(t, result.Error)
340-
assert.Contains(t, result.Error.Error(), "canceled")
341-
assert.Contains(t, result.CommitmentID, "us-west-2")
342-
}
343-
}
344-
345317
func TestExecutePurchase(t *testing.T) {
346318
ctx := context.Background()
347319
// Save original values

0 commit comments

Comments
 (0)