test(coverage): assert CE lookback window width + add --rec-lookback-period flag (refs #360)

cmd/main.go

@@ -11,6 +11,7 @@ import (
 	"github.com/LeanerCloud/CUDly/pkg/common"
 	"github.com/LeanerCloud/CUDly/pkg/provider"
 	_ "github.com/LeanerCloud/CUDly/providers/aws"
+	"github.com/LeanerCloud/CUDly/providers/aws/recommendations"
 	"github.com/LeanerCloud/CUDly/providers/aws/services/ec2"
 	"github.com/LeanerCloud/CUDly/providers/aws/services/elasticache"
 	"github.com/LeanerCloud/CUDly/providers/aws/services/memorydb"
@@ -69,6 +70,12 @@ type Config struct {
 	ActualPurchase         bool
 	DryRun                 bool
 	SkipConfirmation       bool
+	// RecLookbackPeriod controls the LookbackPeriodInDays passed to
+	// GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d"
+	// (recommendations.DefaultRecLookbackPeriod is the shared default).
+	// A longer window smooths seasonal spikes; a shorter window weights
+	// recent demand more heavily.
+	RecLookbackPeriod string
 }
 
 func main() {
@@ -146,6 +153,10 @@ func init() {
 			"below this threshold. Useful with --target-coverage to skip tiny pools "+
 			"that integer arithmetic forces above target (e.g. avg=1 cannot hit 80%%). "+
 			"Default 0 = no filter.")
+	rootCmd.Flags().StringVar(&toolCfg.RecLookbackPeriod, "rec-lookback-period", recommendations.DefaultRecLookbackPeriod,
+		"Historical window for GetReservationPurchaseRecommendation. "+
+			"Valid values: 7d, 30d, 60d. A longer window smooths seasonal spikes; "+
+			"a shorter window weights recent demand more heavily. Default 7d.")
 }
 
 // Package-level Config that cobra flags bind to.

cmd/multi_service.go

@@ -117,6 +117,9 @@ func runToolMultiService(ctx context.Context, cfg Config) {
 
 	accountCache := NewAccountAliasCache(awsCfg)
 	recClient := awsprovider.NewRecommendationsClient(awsCfg)
+	if adapter, ok := recClient.(*awsprovider.RecommendationsClientAdapter); ok && cfg.RecLookbackPeriod != "" {
+		adapter.SetRecLookbackPeriod(cfg.RecLookbackPeriod)
+	}
 	engineData := fetchEngineVersionData(ctx, cfg)
 
 	// Fetch existing-RI coverage so --target-coverage can subtract what
@@ -137,7 +140,14 @@ func runToolMultiService(ctx context.Context, cfg Config) {
 		return
 	}
 
-	// Phase 3: confirm (skipped in dry-run).
+	// Phases 3-4: confirm, purchase, and produce summary outputs.
+	runPurchaseAndReport(ctx, awsCfg, scoredResult, isDryRun, cfg)
+}
+
+// runPurchaseAndReport handles the confirm, execute, and report phases of
+// the multi-service pipeline. It is a separate function to keep
+// runToolMultiService within the cyclomatic-complexity limit.
+func runPurchaseAndReport(ctx context.Context, awsCfg aws.Config, scoredResult scorer.ScoredResult, isDryRun bool, cfg Config) {
 	runID := uuid.New().String()
 	if !isDryRun {
 		totalInstances, totalSavings := sumPassedRecs(scoredResult.Passed)
@@ -147,10 +157,8 @@ func runToolMultiService(ctx context.Context, cfg Config) {
 		}
 	}
 
-	// Phase 4: purchase each recommendation and write audit records.
 	allResults := executePurchasePipeline(ctx, awsCfg, scoredResult.Passed, isDryRun, runID, cfg)
 
-	// Produce summary outputs.
 	serviceStats := buildServiceStats(scoredResult.Passed, allResults)
 	finalCSVOutput := generateCSVFilename(isDryRun, cfg)
 	if err := writeMultiServiceCSVReport(allResults, finalCSVOutput); err != nil {

cmd/multi_service_coverage_test.go

@@ -797,9 +797,11 @@ func TestFetchExistingCoverage_LookbackDays(t *testing.T) {
 		assert.Nil(t, got, "non-AWS provider must return nil (no CE integration)")
 	})
 
-	t.Run("custom lookback stored in Config", func(t *testing.T) {
-		cfg := Config{TargetCoverage: 80, CoverageLookbackDays: 60, Regions: []string{"us-east-1"}}
-		// CoverageLookbackDays field value is preserved in the struct.
-		assert.Equal(t, 60, cfg.CoverageLookbackDays)
-	})
+	// The previous "custom lookback stored in Config" subcase asserted only that
+	// a struct field equals what was just assigned -- a tautology that passes even
+	// if CoverageLookbackDays is never forwarded to GetRICoverageMap.  The real
+	// assertion -- that the lookback value reaches the CE TimePeriod -- is covered
+	// by TestGetRICoverageMap_LookbackWindowWidth in
+	// providers/aws/recommendations/coverage_test.go, which directly verifies
+	// end-start == lookbackDays on the actual CE input.  No redundant subcase here.
 }

cmd/multi_service_helpers.go

@@ -423,12 +423,16 @@ func fetchRecommendationsForRegion(
 		termStr = "3yr"
 	}
 
+	lookback := cfg.RecLookbackPeriod
+	if lookback == "" {
+		lookback = recommendations.DefaultRecLookbackPeriod
+	}
 	params := common.RecommendationParams{
 		Service:        service,
 		Region:         region,
 		PaymentOption:  cfg.PaymentOption,
 		Term:           termStr,
-		LookbackPeriod: "7d",
+		LookbackPeriod: lookback,
 		// Savings Plans specific filters
 		IncludeSPTypes: cfg.IncludeSPTypes,
 		ExcludeSPTypes: cfg.ExcludeSPTypes,

cmd/validators.go

@@ -29,6 +29,19 @@ func validateFlags(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	if err := validateRecLookbackPeriod(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// validateRecLookbackPeriod validates the --rec-lookback-period flag.
+func validateRecLookbackPeriod() error {
+	valid := map[string]bool{"7d": true, "30d": true, "60d": true}
+	if !valid[toolCfg.RecLookbackPeriod] {
+		return fmt.Errorf("invalid rec-lookback-period %q: must be one of 7d, 30d, 60d", toolCfg.RecLookbackPeriod)
+	}
 	return nil
 }
 

cmd/validators_test.go

@@ -537,3 +537,39 @@ func TestValidateCoverageLookbackDays(t *testing.T) {
 		})
 	}
 }
+
+// TestValidateRecLookbackPeriod verifies that validateRecLookbackPeriod accepts
+// the three valid values and rejects anything else, including empty string.
+func TestValidateRecLookbackPeriod(t *testing.T) {
+	tests := []struct {
+		name      string
+		period    string
+		wantErr   bool
+		errSubstr string
+	}{
+		{name: "7d valid", period: "7d", wantErr: false},
+		{name: "30d valid", period: "30d", wantErr: false},
+		{name: "60d valid", period: "60d", wantErr: false},
+		{name: "empty rejected", period: "", wantErr: true, errSubstr: "invalid rec-lookback-period"},
+		{name: "14d rejected", period: "14d", wantErr: true, errSubstr: "invalid rec-lookback-period"},
+		{name: "90d rejected", period: "90d", wantErr: true, errSubstr: "invalid rec-lookback-period"},
+		{name: "SEVEN_DAYS rejected", period: "SEVEN_DAYS", wantErr: true, errSubstr: "invalid rec-lookback-period"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			origCfg := toolCfg
+			defer func() { toolCfg = origCfg }()
+			toolCfg.RecLookbackPeriod = tt.period
+			err := validateRecLookbackPeriod()
+			if tt.wantErr {
+				if err == nil {
+					t.Errorf("validateRecLookbackPeriod() expected error containing %q, got nil", tt.errSubstr)
+				} else if tt.errSubstr != "" && !strings.Contains(err.Error(), tt.errSubstr) {
+					t.Errorf("validateRecLookbackPeriod() error = %v, want substring %q", err, tt.errSubstr)
+				}
+			} else if err != nil {
+				t.Errorf("validateRecLookbackPeriod() unexpected error = %v", err)
+			}
+		})
+	}
+}

providers/aws/recommendations/client.go

@@ -22,6 +22,16 @@ import (
 // payer org we have seen. Exceeding the cap returns a diagnostic error (issue #692).
 const maxRecommendationPages = 20
 
+// DefaultRecLookbackPeriod is the LookbackPeriod string forwarded to
+// GetReservationPurchaseRecommendation when --rec-lookback-period is not
+// specified. Kept in the recommendations package so the cmd flag default,
+// the cmd-side fallback, and the client-side fallback all refer to a single
+// source of truth (avoids the magic-value duplication called out by
+// feedback_no_hardcoded_magic_values.md). Valid CE values are 7d/30d/60d
+// (see convertLookbackPeriodE); 7d matches the prior hardcoded behaviour
+// from before --rec-lookback-period existed.
+const DefaultRecLookbackPeriod = "7d"
+
 // CostExplorerAPI defines the interface for Cost Explorer operations
 type CostExplorerAPI interface {
 	GetReservationPurchaseRecommendation(ctx context.Context, params *costexplorer.GetReservationPurchaseRecommendationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationPurchaseRecommendationOutput, error)
@@ -55,6 +65,10 @@ type Client struct {
 	// lazily once per Client lifetime via sync.Once (one DescribeInstanceTypes
 	// fan-out per scheduler tick).
 	skuCatalog skuCatalog
+
+	// recLookbackPeriod is forwarded to GetReservationPurchaseRecommendation
+	// as LookbackPeriodInDays. Defaults to "7d" when empty.
+	recLookbackPeriod string
 }
 
 // NewClient creates a new recommendations client
@@ -100,7 +114,7 @@ func (c *Client) SetInstanceTypePagerFactory(f func() InstanceTypePager) {
 // instanceTypeLookup returns the cached SKU entry for instanceType.
 // On the first call the catalogue is built by calling the pager factory.
 // ok=false when no factory is configured, the catalogue fetch failed, or
-// the instance type was not in the catalogue — the caller falls back to
+// the instance type was not in the catalogue -- the caller falls back to
 // VCPU=0/MemoryGB=0 (graceful-degradation contract from Azure PR #810).
 func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (instanceTypeSKUEntry, bool) {
 	if c.instanceTypePagerFactory == nil {
@@ -109,6 +123,13 @@ func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (i
 	return c.skuCatalog.lookup(ctx, instanceType, c.instanceTypePagerFactory)
 }
 
+// SetRecLookbackPeriod configures the LookbackPeriodInDays used by
+// GetRecommendationsForService. Valid values: "7d", "30d", "60d".
+// An empty or unrecognised value falls back to "7d" at call time.
+func (c *Client) SetRecLookbackPeriod(period string) {
+	c.recLookbackPeriod = period
+}
+
 // GetRecommendations fetches Reserved Instance recommendations for any service
 func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) {
 	// Handle Savings Plans separately — they use a different Cost Explorer API
@@ -231,9 +252,47 @@ var defaultDiscoveryTerms = []string{"1yr", "3yr"}
 // rows and render as distinct UI rows for free.
 var defaultDiscoveryPaymentOptions = []string{"all-upfront", "partial-upfront", "no-upfront"}
 
+// fetchSingleComboRecs fetches recommendations for one (term, payment) pair.
+// If the context is already done before the call, it returns (nil, ctx.Err()).
+// If GetRecommendations returns an error after ctx cancellation, it also
+// returns (nil, ctx.Err()) so the caller exits the sweep immediately. Per-combo
+// errors (throttle, 5xx) return (nil, err) with ctx.Err() == nil, signalling
+// skip-and-continue tolerance in the outer loop.
+func (c *Client) fetchSingleComboRecs(ctx context.Context, service common.ServiceType, term string, payment string) ([]common.Recommendation, error) {
+	if ctx.Err() != nil {
+		return nil, ctx.Err()
+	}
+	lookback := c.recLookbackPeriod
+	if lookback == "" {
+		lookback = DefaultRecLookbackPeriod
+	}
+	params := common.RecommendationParams{
+		Service:        service,
+		PaymentOption:  payment,
+		Term:           term,
+		LookbackPeriod: lookback,
+		Region:         "",
+	}
+	recs, err := c.GetRecommendations(ctx, params)
+	if err != nil {
+		// A canceled / deadline-exceeded ctx is NOT a per-combo
+		// failure to be tolerated -- every subsequent combo
+		// would just hit the same dead context and waste time
+		// while we accumulate "failures" that hide the real
+		// reason. Short-circuit so the caller sees the ctx
+		// error verbatim. Per-combo errors (throttle, 5xx)
+		// keep the existing skip-and-continue tolerance.
+		if ctx.Err() != nil {
+			return nil, ctx.Err()
+		}
+		return nil, err
+	}
+	return recs, nil
+}
+
 // GetRecommendationsForService fetches recommendations for a specific
-// service across the full Cartesian product of defaultDiscoveryTerms ×
-// defaultDiscoveryPaymentOptions (currently 2 × 3 = 6 Cost Explorer
+// service across the full Cartesian product of defaultDiscoveryTerms x
+// defaultDiscoveryPaymentOptions (currently 2 x 3 = 6 Cost Explorer
 // calls per service). Each call returns the recs for that single
 // (term, payment) cell and the parser tags them with params.Term /
 // params.PaymentOption so the resulting slice contains every combo
@@ -250,26 +309,9 @@ func (c *Client) GetRecommendationsForService(ctx context.Context, service commo
 	attempts := 0
 	for _, term := range defaultDiscoveryTerms {
 		for _, payment := range defaultDiscoveryPaymentOptions {
-			if ctx.Err() != nil {
-				return nil, ctx.Err()
-			}
 			attempts++
-			params := common.RecommendationParams{
-				Service:        service,
-				PaymentOption:  payment,
-				Term:           term,
-				LookbackPeriod: "7d",
-				Region:         "",
-			}
-			recs, err := c.GetRecommendations(ctx, params)
+			recs, err := c.fetchSingleComboRecs(ctx, service, term, payment)
 			if err != nil {
-				// A canceled / deadline-exceeded ctx is NOT a per-combo
-				// failure to be tolerated — every subsequent combo
-				// would just hit the same dead context and waste time
-				// while we accumulate "failures" that hide the real
-				// reason. Short-circuit so the caller sees the ctx
-				// error verbatim. Per-combo errors (throttle, 5xx)
-				// keep the existing skip-and-continue tolerance.
 				if ctx.Err() != nil {
 					return nil, ctx.Err()
 				}

providers/aws/recommendations/client_test.go

@@ -917,3 +917,37 @@ func TestMergeServiceResults_AllFailIsError(t *testing.T) {
 	require.NoError(t, err)
 	assert.Empty(t, recs)
 }
+
+// TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation asserts
+// that SetRecLookbackPeriod propagates the chosen period into the
+// LookbackPeriodInDays field of every GetReservationPurchaseRecommendation
+// call issued by GetRecommendationsForService (refs #360). This test is
+// intentionally discriminating: it would fail if recLookbackPeriod were
+// ignored and the hardcoded "7d" default were sent instead.
+func TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation(t *testing.T) {
+	cases := []struct {
+		period   string
+		wantEnum types.LookbackPeriodInDays
+	}{
+		{"7d", types.LookbackPeriodInDaysSevenDays},
+		{"30d", types.LookbackPeriodInDaysThirtyDays},
+		{"60d", types.LookbackPeriodInDaysSixtyDays},
+	}
+	for _, tc := range cases {
+		t.Run(tc.period, func(t *testing.T) {
+			mock := &mockCostExplorerAPI{
+				riRecommendations: &costexplorer.GetReservationPurchaseRecommendationOutput{},
+			}
+			client := NewClientWithAPI(mock, "us-east-1")
+			client.SetRecLookbackPeriod(tc.period)
+
+			_, err := client.GetRecommendationsForService(context.Background(), common.ServiceEC2)
+			require.NoError(t, err)
+			require.NotEmpty(t, mock.riCalls, "GetReservationPurchaseRecommendation must have been called")
+			for i, call := range mock.riCalls {
+				assert.Equal(t, tc.wantEnum, call.LookbackPeriodInDays,
+					"call[%d]: LookbackPeriodInDays must match --rec-lookback-period=%s", i, tc.period)
+			}
+		})
+	}
+}