fix(cmd): make Savings Plan purchases work and the dry run honest

cmd/multi_service.go

@@ -481,22 +481,32 @@ func purchaseAuditStatus(result common.PurchaseResult) string {
 
 // purchaseSingleRec executes or dry-runs a single purchase and returns the result + audit status.
 func purchaseSingleRec(ctx context.Context, awsCfg aws.Config, rec common.Recommendation, index int, isDryRun bool, cfg Config) (purchaseResult common.PurchaseResult, auditStatus string) {
-	AppLogger.Printf("  [%d] %s %s %s (count=%d)\n", index, rec.Service, rec.Region, rec.ResourceType, rec.Count)
+	label := purchaseRegionLabel(rec, rec.Region)
+	AppLogger.Printf("  [%d] %s %s %s (count=%d)\n", index, rec.Service, label, rec.ResourceType, rec.Count)
+	if err := validatePurchasePreconditions(rec, rec.Region); err != nil {
+		AppLogger.Printf("    ❌ cannot purchase: %v\n", err)
+		return preconditionFailure(rec, err, isDryRun), "error"
+	}
 	if isDryRun {
-		result := createDryRunResult(rec, rec.Region, index, cfg)
+		result := createDryRunResult(rec, label, index, cfg)
 		AppLogger.Printf("    [dry-run] %s\n", result.CommitmentID)
 		return result, "skipped"
 	}
 
+	clientRegion, err := clientRegionFor(rec.Service, rec.Region)
+	if err != nil {
+		AppLogger.Printf("    ❌ %v\n", err)
+		return common.PurchaseResult{Recommendation: rec, Success: false, Error: err, Timestamp: time.Now()}, "error"
+	}
 	regionalCfg := awsCfg.Copy()
-	regionalCfg.Region = rec.Region
+	regionalCfg.Region = clientRegion
 	serviceClient := createServiceClient(rec.Service, regionalCfg)
 	if serviceClient == nil {
 		AppLogger.Printf("    ⚠️  No service client for %s\n", rec.Service)
 		return common.PurchaseResult{Success: false}, "error"
 	}
 
-	result := executePurchase(ctx, rec, rec.Region, index, serviceClient, cfg)
+	result := executePurchase(ctx, rec, label, index, serviceClient, cfg)
 	status := purchaseAuditStatus(result)
 	if result.Success {
 		AppLogger.Printf("    ✅ %s\n", result.CommitmentID)
@@ -566,6 +576,11 @@ func prepareCSVPurchaseRun(ctx context.Context, cfg Config, csvModeCoverage floa
 	}
 	AppLogger.Printf("✅ Loaded %d recommendations from CSV\n", len(recs))
 
+	err = rejectDuplicateSavingsPlanRows(recs)
+	if err != nil {
+		return nil, aws.Config{}, "", err
+	}
+
 	recs, err = filterAndAdjustRecommendations(recs, csvModeCoverage, cfg)
 	if err != nil {
 		return nil, aws.Config{}, "", err
@@ -694,8 +709,13 @@ func runToolFromCSV(ctx context.Context, cfg Config) error {
 // 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) {
+	clientRegion, err := clientRegionFor(service, region)
+	if err != nil {
+		AppLogger.Printf("  ❌ Cannot purchase %d %s recommendation(s) from the CSV: %v\n", len(recs), getServiceDisplayName(service), err)
+		return nil, nil, false
+	}
 	regionalCfg := awsCfg.Copy()
-	regionalCfg.Region = region
+	regionalCfg.Region = clientRegion
 	serviceClient := createServiceClient(service, regionalCfg)
 
 	if serviceClient == nil {
@@ -848,17 +868,21 @@ func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, regi
 		}
 
 		rec := recs[j]
+		label := purchaseRegionLabel(rec, region)
 		AppLogger.Printf("    [%d/%d] Processing: %s %s\n", j+1, len(recs), rec.Service, rec.ResourceType)
 		AppLogger.Printf("    💳 Purchasing %d instances\n", rec.Count)
 
 		var result common.PurchaseResult
 		var status string
-		if isDryRun {
-			result = createDryRunResult(rec, region, j+1, cfg)
+		if err := validatePurchasePreconditions(rec, region); err != nil {
+			result = preconditionFailure(rec, err, isDryRun)
+			status = "error"
+		} else if isDryRun {
+			result = createDryRunResult(rec, label, j+1, cfg)
 			status = "skipped"
 		} else {
 			// Execute actual purchase
-			result = executePurchase(ctx, rec, region, j+1, serviceClient, cfg)
+			result = executePurchase(ctx, rec, label, j+1, serviceClient, cfg)
 			status = purchaseAuditStatus(result)
 
 			// Add delay between purchases to avoid rate limiting

cmd/multi_service_csv.go

@@ -8,6 +8,7 @@ import (
 	"log"
 	"math"
 	"os"
+	"slices"
 	"sort"
 	"strconv"
 	"strings"
@@ -123,36 +124,13 @@ func parseCSVRecord(record []string, colIdx map[string]int) (common.Recommendati
 		return rec, err
 	}
 
-	// Reconstruct the service Details from the Engine/Deployment columns. The
-	// purchase path needs them: RDS findOfferingID rejects a rec with nil
-	// Details ("invalid service details for RDS"), and RI offerings are keyed
-	// by engine and Multi-AZ. This mirrors the writer side (extractEngine /
-	// extractDeployment emit DatabaseDetails / CacheDetails / ComputeDetails),
-	// so a CSV the tool wrote round-trips losslessly. Engine is stored in Cost
-	// Explorer format ("Aurora MySQL"); findOfferingID normalizes it. Guarded
-	// on a non-empty Engine so minimal CSVs and Savings Plans rows (no Engine
-	// column) keep their previous nil-Details behavior.
-	if engine := getCSVField(record, colIdx, "Engine"); engine != "" {
-		deployment := getCSVField(record, colIdx, "Deployment")
-		switch rec.Service {
-		case common.ServiceRDS, common.ServiceRelationalDB:
-			rec.Details = &common.DatabaseDetails{
-				Engine:        engine,
-				AZConfig:      deployment,
-				InstanceClass: rec.ResourceType,
-			}
-		case common.ServiceElastiCache, common.ServiceCache:
-			rec.Details = &common.CacheDetails{
-				Engine:   engine,
-				NodeType: rec.ResourceType,
-			}
-		case common.ServiceEC2, common.ServiceCompute:
-			rec.Details = &common.ComputeDetails{
-				InstanceType: rec.ResourceType,
-				Platform:     engine,
-			}
-		}
+	// Details come from the Engine/Deployment columns and the appended detail
+	// columns (see csvDetails).
+	details, err := csvDetails(rec, record, colIdx)
+	if err != nil {
+		return rec, err
 	}
+	rec.Details = details
 
 	return rec, nil
 }
@@ -240,7 +218,7 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string
 	// every row, which adds noise without information; the underlying fields
 	// stay on the Recommendation struct for internal use (SP no-signal
 	// guard, etc.).
-	header := []string{
+	baseHeader := []string{
 		"Service", "Region", "ResourceType", "Family", "Engine", "Deployment",
 		"Instances", "CoveredInstances",
 		"Count", "NormalizedUnits", "RecommendedCount",
@@ -249,6 +227,8 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string
 		"CommitmentID", "Success", "Error", "Timestamp",
 		"ExistingCoverage", "ProjectedCoverage",
 	}
+	// Appended, never reordered: existing readers look columns up by name.
+	header := slices.Concat(baseHeader, csvDetailColumns)
 	if err := writer.Write(header); err != nil {
 		return fmt.Errorf("failed to write CSV header: %w", err)
 	}
@@ -272,7 +252,7 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string
 			errStr = r.Error.Error()
 		}
 
-		row := []string{
+		baseRow := []string{
 			string(rec.Service),
 			rec.Region,
 			rec.ResourceType,
@@ -298,6 +278,7 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string
 			formatExistingCoverage(rec),
 			formatPercentOrBlank(rec.ProjectedCoverage),
 		}
+		row := slices.Concat(baseRow, detailCells(rec))
 		if err := writer.Write(row); err != nil {
 			return fmt.Errorf("failed to write CSV row: %w", err)
 		}
@@ -346,7 +327,7 @@ func buildTotalRow(results []common.PurchaseResult) []string {
 	if totalNU > 0 {
 		nuCell = fmt.Sprintf("%g", totalNU)
 	}
-	return []string{
+	totals := []string{
 		"TOTAL", "", "", "", "", "", // Service through Deployment
 		"", "", // Instances, CoveredInstances
 		fmt.Sprintf("%d", totalCount), nuCell, "", // Count, NormalizedUnits, RecommendedCount
@@ -355,6 +336,7 @@ func buildTotalRow(results []common.PurchaseResult) []string {
 		"", "", "", "", // CommitmentID, Success, Error, Timestamp
 		"", "", // ExistingCoverage, ProjectedCoverage
 	}
+	return slices.Concat(totals, make([]string, len(csvDetailColumns))) // detail columns do not aggregate
 }
 
 // formatIntOrBlank renders an int as its decimal string when non-zero, ""

cmd/multi_service_csv_details.go

@@ -0,0 +1,91 @@
+package main
+
+import (
+	"strconv"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+	"github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/savingsplans"
+)
+
+// csvDetailColumns are the report columns that carry the fields of a
+// recommendation's polymorphic Details that Engine/Deployment cannot: Savings
+// Plan identity and EC2 tenancy and scope. They are appended after the
+// original columns so existing readers, which look columns up by name, keep
+// working. The writer and csvDetails below must stay in step.
+var csvDetailColumns = []string{"PlanType", "HourlyCommitment", "InstanceFamily", "OfferingID", "DetailsRegion", "Tenancy", "Scope"}
+
+// detailCells renders rec.Details for csvDetailColumns, in that order.
+func detailCells(rec common.Recommendation) []string {
+	cells := make([]string, len(csvDetailColumns))
+	switch d := rec.Details.(type) {
+	case *common.SavingsPlanDetails:
+		if d != nil {
+			cells[0] = d.PlanType
+			cells[1] = strconv.FormatFloat(d.HourlyCommitment, 'f', -1, 64)
+			cells[2] = d.InstanceFamily
+			cells[3] = d.OfferingID
+			cells[4] = d.Region
+		}
+	case *common.ComputeDetails:
+		if d != nil {
+			cells[5], cells[6] = d.Tenancy, d.Scope
+		}
+	case common.ComputeDetails:
+		cells[5], cells[6] = d.Tenancy, d.Scope
+	}
+	return cells
+}
+
+// csvDetails reconstructs the service Details of a CSV row. The purchase path
+// needs them: RDS findOfferingID rejects nil Details, Savings Plan purchases
+// need the plan type and hourly commitment, and EC2 offerings are keyed by
+// platform, tenancy and scope. This mirrors the writer (extractEngine,
+// extractDeployment, detailCells), so a CSV the tool wrote round-trips.
+//
+// A row without the identifying columns (a minimal or older CSV) keeps nil
+// Details; validatePurchasePreconditions then reports it rather than the
+// loader inventing values.
+func csvDetails(rec common.Recommendation, record []string, colIdx map[string]int) (common.ServiceDetails, error) {
+	if _, isSP := savingsplans.PlanTypeForServiceType(rec.Service); isSP {
+		return csvSavingsPlanDetails(record, colIdx)
+	}
+	engine := getCSVField(record, colIdx, "Engine")
+	if engine == "" {
+		return nil, nil
+	}
+	switch rec.Service {
+	case common.ServiceRDS, common.ServiceRelationalDB:
+		return &common.DatabaseDetails{
+			Engine:        engine,
+			AZConfig:      getCSVField(record, colIdx, "Deployment"),
+			InstanceClass: rec.ResourceType,
+		}, nil
+	case common.ServiceElastiCache, common.ServiceCache:
+		return &common.CacheDetails{Engine: engine, NodeType: rec.ResourceType}, nil
+	case common.ServiceEC2, common.ServiceCompute:
+		return &common.ComputeDetails{
+			InstanceType: rec.ResourceType,
+			Platform:     engine,
+			Tenancy:      getCSVField(record, colIdx, "Tenancy"),
+			Scope:        getCSVField(record, colIdx, "Scope"),
+		}, nil
+	}
+	return nil, nil
+}
+
+func csvSavingsPlanDetails(record []string, colIdx map[string]int) (common.ServiceDetails, error) {
+	planType := getCSVField(record, colIdx, "PlanType")
+	if planType == "" && getCSVField(record, colIdx, "HourlyCommitment") == "" {
+		return nil, nil
+	}
+	d := &common.SavingsPlanDetails{
+		PlanType:       planType,
+		InstanceFamily: getCSVField(record, colIdx, "InstanceFamily"),
+		OfferingID:     getCSVField(record, colIdx, "OfferingID"),
+		Region:         getCSVField(record, colIdx, "DetailsRegion"),
+	}
+	if err := parseCSVFloat(record, colIdx, "HourlyCommitment", &d.HourlyCommitment); err != nil {
+		return nil, err
+	}
+	return d, nil
+}

cmd/multi_service_helpers.go

@@ -254,17 +254,23 @@ func executePurchase(ctx context.Context, rec common.Recommendation, region stri
 
 // determineRegionsForService determines which regions to process for a given service.
 func determineRegionsForService(ctx context.Context, awsCfg aws.Config, recClient provider.RecommendationsClient, service common.ServiceType, configuredRegions []string) ([]string, error) {
+	// Savings Plans are account-level, not regional - only query once, even
+	// when --regions lists several. The recommendation call returns the same
+	// account-wide set for every region, so honoring --regions here repeats
+	// each Savings Plan once per listed region and, now that Savings Plan
+	// purchases work, would buy it that many times. EC2Instance plans are
+	// still narrowed to the requested regions by the region filters on
+	// Details.Region.
+	if common.IsSavingsPlan(service) {
+		AppLogger.Printf("🌍 Fetching account-level Savings Plans recommendations...\n")
+		return []string{spAPIRegion}, nil // Single query for account-level data
+	}
+
 	// If regions are explicitly configured, use those
 	if len(configuredRegions) > 0 {
 		return configuredRegions, nil
 	}
 
-	// Savings Plans are account-level, not regional - only query once
-	if common.IsSavingsPlan(service) {
-		AppLogger.Printf("🌍 Fetching account-level Savings Plans recommendations...\n")
-		return []string{"us-east-1"}, nil // Single query for account-level data
-	}
-
 	// Default to all AWS regions for other services
 	AppLogger.Printf("🌍 Processing all AWS regions for %s...\n", getServiceDisplayName(service))
 	allRegions, err := getAllAWSRegions(ctx, awsCfg)

cmd/multi_service_test.go

@@ -1459,8 +1459,8 @@ func TestProcessPurchaseLoopActualPurchase(t *testing.T) {
 	toolCfg.Coverage = 80.0
 
 	recs := []common.Recommendation{
-		{Service: common.ServiceEC2, ResourceType: "t3.small", Count: 1, SourceRecommendation: "EC2 Test 1", EstimatedSavings: 100},
-		{Service: common.ServiceEC2, ResourceType: "t3.medium", Count: 2, SourceRecommendation: "EC2 Test 2", EstimatedSavings: 200},
+		{Service: common.ServiceEC2, ResourceType: "t3.small", Count: 1, SourceRecommendation: "EC2 Test 1", EstimatedSavings: 100, Details: &common.ComputeDetails{InstanceType: "t3.small", Platform: "Linux/UNIX", Tenancy: "default", Scope: "regional"}},
+		{Service: common.ServiceEC2, ResourceType: "t3.medium", Count: 2, SourceRecommendation: "EC2 Test 2", EstimatedSavings: 200, Details: &common.ComputeDetails{InstanceType: "t3.medium", Platform: "Linux/UNIX", Tenancy: "default", Scope: "regional"}},
 	}
 
 	mockClient := &MockServiceClient{}