fix(aws/ec2): reject unrecognized RI term instead of silently defaulting to 1yr (follow-up to #1207)

internal/scheduler/scheduler.go

@@ -1299,15 +1299,21 @@ func (s *Scheduler) convertRecommendations(recs []common.Recommendation, provide
 				providerName, rec.PaymentOption, rec.Account, rec.Service, rec.ResourceType)
 		}
 
-		// Parse term to integer (e.g., "3yr" -> 3)
-		term := 3
-		if rec.Term != "" {
-			termStr := strings.TrimSuffix(rec.Term, "yr")
-			if parsed, err := strconv.Atoi(termStr); err == nil && parsed > 0 {
-				term = parsed
-			} else {
-				logging.Warnf("Invalid term value %q, defaulting to 3 years", rec.Term)
-			}
+		// Parse term to integer (e.g., "3yr" -> 3). An empty or unparseable
+		// term must NOT be coerced to a valid default: rec.Term round-trips
+		// through this int on the purchase path (internal/purchase/execution.go
+		// formats it back as "%dyr"), so silently defaulting here would let a
+		// malformed term launder into a valid, purchasable commitment length
+		// downstream, bypassing the ARCH-04 whitelist checks in the provider
+		// clients entirely, since "3yr" is itself a whitelisted value (issue
+		// #1207 follow-up). Drop the recommendation instead of persisting a
+		// fabricated term.
+		termStr := strings.TrimSuffix(rec.Term, "yr")
+		term, err := strconv.Atoi(termStr)
+		if err != nil || term <= 0 {
+			logging.Errorf("convertRecommendations: dropping %s rec (account=%s service=%s sku=%s region=%s): invalid term %q",
+				providerName, rec.Account, rec.Service, rec.ResourceType, rec.Region, rec.Term)
+			continue
 		}
 
 		// The ID must be unique per logically-distinct rec — otherwise

internal/scheduler/scheduler_test.go

@@ -1190,6 +1190,66 @@ func TestScheduler_ConvertRecommendations_Empty(t *testing.T) {
 	assert.Len(t, records, 0)
 }
 
+// TestScheduler_ConvertRecommendations_InvalidTermDropped is the ARCH-04
+// follow-up (issue #1207) regression test: a rec with an empty or
+// unparseable Term must be dropped rather than silently persisted with
+// Term=3. rec.Term round-trips through this int on the purchase path
+// (internal/purchase/execution.go formats it back as "%dyr"), so a
+// fabricated default here would let a malformed term launder into a real
+// 3-year purchase, bypassing every provider client's ARCH-04 whitelist
+// (since "3yr" is itself a valid, purchasable term). This test fails on
+// the pre-fix code (which appends a record with Term=3 for both bad recs)
+// and passes once the invalid-term recs are skipped.
+func TestScheduler_ConvertRecommendations_InvalidTermDropped(t *testing.T) {
+	scheduler := &Scheduler{}
+
+	recommendations := []common.Recommendation{
+		// Empty Term: what a 0/NULL Term DB row produces on the scheduler
+		// purchase path.
+		{
+			Provider:         common.ProviderAWS,
+			Service:          common.ServiceEC2,
+			Region:           "us-east-1",
+			ResourceType:     "m5.large",
+			Count:            1,
+			Term:             "",
+			PaymentOption:    "all-upfront",
+			CommitmentCost:   1000.0,
+			EstimatedSavings: 500.0,
+		},
+		// Garbage Term.
+		{
+			Provider:         common.ProviderAWS,
+			Service:          common.ServiceEC2,
+			Region:           "us-east-1",
+			ResourceType:     "m5.xlarge",
+			Count:            1,
+			Term:             "banana",
+			PaymentOption:    "all-upfront",
+			CommitmentCost:   2000.0,
+			EstimatedSavings: 900.0,
+		},
+		// Valid rec in the same batch must still be persisted.
+		{
+			Provider:         common.ProviderAWS,
+			Service:          common.ServiceEC2,
+			Region:           "us-east-1",
+			ResourceType:     "m5.2xlarge",
+			Count:            1,
+			Term:             "1yr",
+			PaymentOption:    "all-upfront",
+			CommitmentCost:   3000.0,
+			EstimatedSavings: 1400.0,
+		},
+	}
+
+	records := scheduler.convertRecommendations(recommendations, "aws")
+
+	require.Len(t, records, 1, "both invalid-term recs must be dropped, not persisted with a fabricated Term")
+	assert.Equal(t, "m5.2xlarge", records[0].ResourceType)
+	assert.Equal(t, 1, records[0].Term)
+}
+
 // TestScheduler_ConvertRecommendations_OnDemandCost pins #274: the
 // canonical on-demand baseline must round-trip from common.Recommendation
 // through to the persisted RecommendationRecord so the frontend can use

providers/aws/services/ec2/client.go

@@ -461,7 +461,11 @@ func (c *Client) buildEC2QueryFromRec(rec common.Recommendation) (ec2OfferingQue
 	if err != nil {
 		return ec2OfferingQuery{}, err
 	}
-	q, err := buildEC2OfferingQuery(rec, details, c.getDurationValue(rec.Term))
+	duration, err := c.getDurationValue(rec.Term)
+	if err != nil {
+		return ec2OfferingQuery{}, err
+	}
+	q, err := buildEC2OfferingQuery(rec, details, duration)
 	if err != nil {
 		return ec2OfferingQuery{}, err
 	}
@@ -661,12 +665,19 @@ const (
 	ThreeYearSeconds = 94608000 // 3 * 365 days in seconds
 )
 
-// getDurationValue converts term string to seconds for EC2 API
-func (c *Client) getDurationValue(term string) int64 {
-	if term == "3yr" || term == "3" {
-		return ThreeYearSeconds
+// getDurationValue converts a term string to seconds for the EC2 API.
+// Returns an error on any unrecognized or empty input so callers fail loud
+// rather than silently buying a 1-year reservation when another commitment
+// length was intended.
+func (c *Client) getDurationValue(term string) (int64, error) {
+	switch term {
+	case "3yr", "3":
+		return ThreeYearSeconds, nil
+	case "1yr", "1":
+		return OneYearSeconds, nil
+	default:
+		return 0, fmt.Errorf("unsupported EC2 reservation term %q: must be one of 1yr, 1, 3yr, 3", term)
 	}
-	return OneYearSeconds
 }
 
 // ConvertibleRI represents an active convertible Reserved Instance.

providers/aws/services/ec2/client_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/aws/aws-sdk-go-v2/service/ec2/types"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 )
 
 // MockEC2Client implements EC2API for testing
@@ -516,19 +517,35 @@ func TestClient_GetDurationValue(t *testing.T) {
 	client := &Client{}
 
 	tests := []struct {
-		name     string
-		term     string
-		expected int64
+		name      string
+		term      string
+		expected  int64
+		expectErr bool
 	}{
-		{"1 year", "1yr", 31536000},
-		{"3 years", "3yr", 94608000},
-		{"3 numeric", "3", 94608000},
-		{"default", "invalid", 31536000},
+		{"1 year", "1yr", 31536000, false},
+		{"1 numeric", "1", 31536000, false},
+		{"3 years", "3yr", 94608000, false},
+		{"3 numeric", "3", 94608000, false},
+		// Regression for ARCH-04 follow-up (issue #1207): unrecognized or
+		// empty terms must error instead of silently mapping to a 1-year
+		// purchase. An empty term is what a 0/NULL Term DB row produces on
+		// the scheduler purchase path.
+		{"invalid term errors", "invalid", 0, true},
+		{"empty term errors", "", 0, true},
+		{"zero term errors", "0", 0, true},
+		{"2yr term errors", "2yr", 0, true},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := client.getDurationValue(tt.term)
+			result, err := client.getDurationValue(tt.term)
+			if tt.expectErr {
+				if assert.Error(t, err) {
+					assert.Contains(t, err.Error(), "unsupported EC2 reservation term")
+				}
+				return
+			}
+			assert.NoError(t, err)
 			assert.Equal(t, tt.expected, result)
 		})
 	}
@@ -710,6 +727,36 @@ func TestFindOfferingID_HappyPath(t *testing.T) {
 	assert.Equal(t, "offering-ok", id)
 }
 
+// TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall is the ARCH-04
+// follow-up (issue #1207) call-path regression test: an unrecognized or
+// empty term must abort the offering lookup before any
+// DescribeReservedInstancesOfferings call, rather than silently matching
+// (and buying) a 1-year offering. A "0" term is what a 0/NULL Term DB row
+// produces on the scheduler purchase path.
+func TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall(t *testing.T) {
+	t.Parallel()
+	mockEC2 := &MockEC2Client{}
+	t.Cleanup(func() { mockEC2.AssertExpectations(t) })
+	client := &Client{client: mockEC2, region: "us-east-1"}
+
+	rec := common.Recommendation{
+		ResourceType:  "t4g.nano",
+		PaymentOption: "no-upfront",
+		Term:          "0",
+		Details: &common.ComputeDetails{
+			Platform: "Linux/UNIX",
+			Tenancy:  "default",
+			Scope:    "Region",
+		},
+	}
+
+	_, err := client.findOfferingID(context.Background(), rec, "", "")
+
+	require.Error(t, err, "findOfferingID must error on an unrecognized term (ARCH-04)")
+	assert.Contains(t, err.Error(), "unsupported EC2 reservation term")
+	mockEC2.AssertNotCalled(t, "DescribeReservedInstancesOfferings", mock.Anything, mock.Anything)
+}
+
 // TestClient_tagReservedInstance_NameTagPresent asserts that a purchase on the
 // no-token CLI path (issue #687) stamps a self-describing Name tag on the EC2
 // RI. EC2 PurchaseReservedInstancesOfferingInput has no customer-supplied name