diff --git a/providers/aws/services/AUDIT.md b/providers/aws/services/AUDIT.md new file mode 100644 index 000000000..4478c7d08 --- /dev/null +++ b/providers/aws/services/AUDIT.md @@ -0,0 +1,182 @@ +# findOfferingID Quarterly Audit Checklist + +This document is the authoritative checklist for the periodic audit of all seven +AWS service clients' `findOfferingID` implementations (or `lookupOfferingID` for +SavingsPlans). The audit was introduced in issue #515 following the PR #690 +rework. Run this checklist whenever: + +- A quarterly reminder fires. +- AWS announces a new offering attribute for any of the services below. +- A `findOfferingID` function is modified on a PR. + +--- + +## Contract Items (all 7 services must satisfy every item) + +### C1 -- Typed first-class fields, not Filter[]-heavy queries + +Use SDK-typed request fields (e.g. `InstanceType`, `ProductDescription`, +`OfferingType`) to narrow the result set server-side where the AWS API supports +them. Relying solely on client-side filtering inside `Filters[]` caused AWS to +return empty pages with a NextToken, walking until the Lambda budget expired +(issue #688). + +**Exception**: OpenSearch and Redshift have no server-side filters for instance +type or payment option; all matching must be done client-side. This is +intentional and documented in those clients' function comments. + +### C2 -- ctx.Err() check at the top of each pagination iteration + +```go +for { + if err := ctx.Err(); err != nil { + return "", err + } + // ... page fetch ... +} +``` + +Context cancellation is terminal (see `feedback_ctx_cancel_terminal.md`). The +check must be the first statement inside the loop body, before any API call. +This is regression-tested by `TestFindOfferingID_CtxCancelledBeforePage` in +each service's `client_test.go`. + +### C3 -- Page cap (maxOfferingPages) with explicit diagnostic error + +```go +const maxOfferingPages = 5 + +if page > maxOfferingPages { + return "", fmt.Errorf("pagination cap reached after %d pages ... (issue #688)", ...) +} +``` + +The cap must reference issue #688 in the error string to aid log triage. It +must fire before the (cap+1)th API call. Regression-tested by +`TestFindOfferingID_PaginationCapFires` / `TestLookupOfferingID_PaginationCapFires`. + +### C4 -- Per-page log line with timing + +```go +log.Printf("purchase[%s]: findOfferingID page %d: %d offerings in %s", + tag, page, len(results), time.Since(pageStart)) +``` + +Both the page number and the elapsed time since page start must appear. The tag +(`execID` or `"no-exec"`) provides log correlation across concurrent purchases. + +### C5 -- Soft-skip of mismatched variants + +When a returned offering does not match the requested payment option (or offering +type), the behaviour depends on whether the API accepts a payment-option filter: + +- **Filter-capable APIs** (RDS, ElastiCache, MemoryDB): API filter is set, so a + mismatch indicates an API bug, not a routine variant. Return an error with + `"API filter mismatch"` in the message. +- **Client-side-only APIs** (EC2, OpenSearch): skip the mismatched offering (log + it) and continue to the next one. Do not fail the entire search. +- **Redshift**: the AWS offering type is `"Regular"` or `"Upgradable"`, which is + unrelated to payment option. Payment option is not a server-side or client-side + filter dimension. Any value outside those two is surfaced as an error. +- **SavingsPlans**: `PaymentOptions` filter is passed on the request; the first + result is returned without a secondary check. + +### C6 -- Pagination terminator handles BOTH nil AND empty-string token + +```go +// Pattern for *string token: +if token == nil || aws.ToString(token) == "" { + break +} + +// Pattern via helper (EC2, MemoryDB): +func isLastXxxPage(token *string) bool { + return token == nil || aws.ToString(token) == "" +} +``` + +The AWS SDK may return either `nil` or a pointer to `""` for the last page. +Both must end pagination to avoid an extra redundant API call. Regression-tested +by `TestFindOfferingID_EmptyStringTokenEndsPagination` / +`TestLookupOfferingID_EmptyStringTokenEndsPagination`. + +--- + +## Service-by-Service Status (last audited 2026-Q2, issue #515) + +| Service | C1 typed fields | C2 ctx.Err() | C3 page cap | C4 per-page log | C5 variant skip | C6 nil+empty token | +|--------------|:---------:|:---------:|:---------:|:---------:|:---------:|:---------:| +| EC2 | PASS | PASS | PASS | PASS | PASS (soft-skip) | PASS (`isLastEC2Page`) | +| RDS | PASS | PASS | PASS | PASS | PASS (hard-error on mismatch) | PASS (inline) | +| ElastiCache | PASS | PASS | PASS | PASS | PASS (hard-error on mismatch) | PASS (inline) | +| MemoryDB | PASS | PASS | PASS | PASS | PASS (hard-error on mismatch) | PASS (`isLastMemoryDBPage`) | +| OpenSearch | PASS* | PASS | PASS | PASS | PASS (error only when type+duration match but payment differs) | PASS (inline) | +| Redshift | PASS* | PASS | PASS | PASS | PASS (error on unknown offering type) | PASS (inline) | +| SavingsPlans | PASS | PASS | PASS | PASS | PASS (filter on request) | PASS (inline) | + +\* OpenSearch and Redshift have no server-side filters; client-side matching is +the only option. Documented in function comments. + +--- + +## Watchlist Items (no code change needed today, monitor each quarter) + +### MemoryDB -- Valkey engine dimension + +AWS added Valkey alongside Redis for MemoryDB in 2024. +`DescribeReservedNodesOfferings` does not currently expose engine as a separate +offering attribute (a Valkey RI is still matched by node type). If AWS ever +splits the offering schema on engine, `findOfferingID` would silently pick the +first match for the given node type, potentially buying a Redis offering for a +Valkey recommendation. + +**Check each quarter**: call `aws memorydb describe-reserved-nodes-offerings` +and inspect whether any new field like `Engine` or `EngineVersion` appears on +the `ReservedNodesOffering` struct. If it does, update `findOfferingID` to +filter on it and add `Engine` to `common.MemoryDBDetails` if needed. + +### OpenSearch / Redshift -- Graviton architecture dimension + +If AWS introduces an ARM-vs-x86 dimension on +`DescribeReservedInstanceOfferings` (OpenSearch) or +`DescribeReservedNodeOfferings` (Redshift), the current first-match-by-type loop +would become ambiguous between Graviton and non-Graviton offerings for the same +node type. + +**Check each quarter**: inspect the SDK types for any new field on +`ReservedInstanceOffering` (OpenSearch) or `ReservedNodeOffering` (Redshift). +If a new dimension appears, extend `common.SearchDetails` / +`common.RedshiftDetails` accordingly and add a client-side filter in the +respective `scan*OfferingPage` function. + +--- + +## How the Contract is Enforced in CI + +Each of the seven `client_test.go` files contains three tests that pin the +contract at the code level: + +| Test name | Catches regression on | +|-----------|----------------------| +| `TestFindOfferingID_PaginationCapFires` | C3 -- page cap | +| `TestFindOfferingID_CtxCancelledBeforePage` | C2 -- ctx.Err() | +| `TestFindOfferingID_EmptyStringTokenEndsPagination` | C6 -- nil + empty token | +| `TestFindOfferingID_WrongVariantRejected` | C5 -- variant mismatch | +| `TestFindOfferingID_HappyPath` | overall happy path | + +SavingsPlans names the equivalent tests `TestLookupOfferingID_*` because +pagination lives in `lookupOfferingID`. + +Running `go test ./providers/aws/services/...` covers all seven services. + +--- + +## Performing the Quarterly Audit + +1. Run `go test ./providers/aws/services/...` -- all tests must pass. +2. For each service, open `providers/aws/services//client.go` and walk + the checklist items C1-C6 against the current implementation. +3. Grep for the watchlist dimensions described above. +4. Update the "last audited" date and the status table in this file. +5. If any item regresses, open a new issue referencing #515 and fix it in the + same PR. diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 05f623f61..920ea0198 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -1225,3 +1225,68 @@ func TestFindOfferingID_OfferingClassReachesSDKCall(t *testing.T) { }) } } + +// TestFindOfferingID_CtxCancelledBeforePage asserts that findOfferingID returns +// context.Canceled immediately when the context is already cancelled at the top +// of the first pagination iteration, without calling the AWS API (issue #515). +func TestFindOfferingID_CtxCancelledBeforePage(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: "1yr", + Details: &common.ComputeDetails{ + Platform: "Linux/UNIX", + Tenancy: "default", + Scope: "Region", + }, + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before the first iteration + + // The mock must not be called: ctx.Err() fires at the top of the loop. + _, err := client.findOfferingID(ctx, rec, "", "") + + assert.ErrorIs(t, err, context.Canceled) + mockEC2.AssertNumberOfCalls(t, "DescribeReservedInstancesOfferings", 0) +} + +// TestFindOfferingID_EmptyStringTokenEndsPagination asserts that a page whose +// NextToken is a pointer to an empty string (rather than nil) is treated as the +// terminal page and does not cause an extra API call (issue #515). +func TestFindOfferingID_EmptyStringTokenEndsPagination(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: "1yr", + Details: &common.ComputeDetails{ + Platform: "Linux/UNIX", + Tenancy: "default", + Scope: "Region", + }, + } + + // Single page with zero results and NextToken = ""; must not loop again. + mockEC2.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Return(&ec2.DescribeReservedInstancesOfferingsOutput{ + ReservedInstancesOfferings: []types.ReservedInstancesOffering{}, + NextToken: aws.String(""), + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec, "", "") + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "no offerings found") + } + mockEC2.AssertNumberOfCalls(t, "DescribeReservedInstancesOfferings", 1) +} diff --git a/providers/aws/services/elasticache/client_test.go b/providers/aws/services/elasticache/client_test.go index 186ff9479..1666cb2af 100644 --- a/providers/aws/services/elasticache/client_test.go +++ b/providers/aws/services/elasticache/client_test.go @@ -721,3 +721,58 @@ func TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall(t *testing.T) { } mockEC.AssertNotCalled(t, "DescribeReservedCacheNodesOfferings", mock.Anything, mock.Anything) } + +// TestFindOfferingID_CtxCancelledBeforePage asserts that findOfferingID returns +// context.Canceled immediately when the context is already cancelled before the +// first pagination iteration, without calling the AWS API (issue #515). +func TestFindOfferingID_CtxCancelledBeforePage(t *testing.T) { + mockEC := &MockElastiCacheClient{} + t.Cleanup(func() { mockEC.AssertExpectations(t) }) + client := &Client{client: mockEC, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "cache.r6g.large", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.CacheDetails{Engine: "redis"}, + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before the first iteration + + // The mock must not be called: ctx.Err() fires at the top of the loop. + _, err := client.findOfferingID(ctx, rec, "") + + assert.ErrorIs(t, err, context.Canceled) + mockEC.AssertNumberOfCalls(t, "DescribeReservedCacheNodesOfferings", 0) +} + +// TestFindOfferingID_EmptyStringTokenEndsPagination asserts that a page whose +// Marker is a pointer to an empty string (rather than nil) is treated as the +// terminal page and does not cause an extra API call (issue #515). +func TestFindOfferingID_EmptyStringTokenEndsPagination(t *testing.T) { + mockEC := &MockElastiCacheClient{} + t.Cleanup(func() { mockEC.AssertExpectations(t) }) + client := &Client{client: mockEC, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "cache.r6g.large", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.CacheDetails{Engine: "redis"}, + } + + // Single page with zero results and Marker = ""; must not loop again. + mockEC.On("DescribeReservedCacheNodesOfferings", mock.Anything, mock.Anything). + Return(&elasticache.DescribeReservedCacheNodesOfferingsOutput{ + ReservedCacheNodesOfferings: []types.ReservedCacheNodesOffering{}, + Marker: aws.String(""), + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec, "") + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "no offerings found") + } + mockEC.AssertNumberOfCalls(t, "DescribeReservedCacheNodesOfferings", 1) +} diff --git a/providers/aws/services/memorydb/client_test.go b/providers/aws/services/memorydb/client_test.go index f68018212..a04ce8d60 100644 --- a/providers/aws/services/memorydb/client_test.go +++ b/providers/aws/services/memorydb/client_test.go @@ -384,6 +384,59 @@ func TestFindOfferingID_HappyPath(t *testing.T) { assert.Equal(t, "offering-ok", id) } +// TestFindOfferingID_CtxCancelledBeforePage asserts that findOfferingID returns +// context.Canceled immediately when the context is already cancelled before the +// first pagination iteration, without calling the AWS API (issue #515). +func TestFindOfferingID_CtxCancelledBeforePage(t *testing.T) { + mockMDB := &MockMemoryDBClient{} + t.Cleanup(func() { mockMDB.AssertExpectations(t) }) + client := &Client{client: mockMDB, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "db.r6g.large", + PaymentOption: "no-upfront", + Term: "1yr", + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before the first iteration + + // The mock must not be called: ctx.Err() fires at the top of the loop. + _, err := client.findOfferingID(ctx, rec, "") + + assert.ErrorIs(t, err, context.Canceled) + mockMDB.AssertNumberOfCalls(t, "DescribeReservedNodesOfferings", 0) +} + +// TestFindOfferingID_EmptyStringTokenEndsPagination asserts that a page whose +// NextToken is a pointer to an empty string (rather than nil) is treated as the +// terminal page and does not cause an extra API call (issue #515). +func TestFindOfferingID_EmptyStringTokenEndsPagination(t *testing.T) { + mockMDB := &MockMemoryDBClient{} + t.Cleanup(func() { mockMDB.AssertExpectations(t) }) + client := &Client{client: mockMDB, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "db.r6g.large", + PaymentOption: "no-upfront", + Term: "1yr", + } + + // Single page with zero results and NextToken = ""; must not loop again. + mockMDB.On("DescribeReservedNodesOfferings", mock.Anything, mock.Anything). + Return(&memorydb.DescribeReservedNodesOfferingsOutput{ + ReservedNodesOfferings: []types.ReservedNodesOffering{}, + NextToken: aws.String(""), + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec, "") + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "no offerings found") + } + mockMDB.AssertNumberOfCalls(t, "DescribeReservedNodesOfferings", 1) +} + func TestClient_SetMemoryDBAPI(t *testing.T) { client := &Client{region: "us-east-1"} mockAPI := &MockMemoryDBClient{} diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index e9662fd5a..548799769 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -1008,3 +1008,56 @@ func TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall(t *testing.T) { } mockOS.AssertNotCalled(t, "DescribeReservedInstanceOfferings", mock.Anything, mock.Anything) } + +// TestFindOfferingID_CtxCancelledBeforePage asserts that findOfferingID returns +// context.Canceled immediately when the context is already cancelled before the +// first pagination iteration, without calling the AWS API (issue #515). +func TestFindOfferingID_CtxCancelledBeforePage(t *testing.T) { + mockOS := &MockOpenSearchClient{} + t.Cleanup(func() { mockOS.AssertExpectations(t) }) + client := &Client{client: mockOS, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "m5.xlarge.search", + PaymentOption: "no-upfront", + Term: "1yr", + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before the first iteration + + // The mock must not be called: ctx.Err() fires at the top of the loop. + _, err := client.findOfferingID(ctx, rec, "") + + assert.ErrorIs(t, err, context.Canceled) + mockOS.AssertNumberOfCalls(t, "DescribeReservedInstanceOfferings", 0) +} + +// TestFindOfferingID_EmptyStringTokenEndsPagination asserts that a page whose +// NextToken is a pointer to an empty string (rather than nil) is treated as the +// terminal page and does not cause an extra API call (issue #515). +func TestFindOfferingID_EmptyStringTokenEndsPagination(t *testing.T) { + mockOS := &MockOpenSearchClient{} + t.Cleanup(func() { mockOS.AssertExpectations(t) }) + client := &Client{client: mockOS, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "m5.xlarge.search", + PaymentOption: "no-upfront", + Term: "1yr", + } + + // Single page with zero results and NextToken = ""; must not loop again. + mockOS.On("DescribeReservedInstanceOfferings", mock.Anything, mock.Anything). + Return(&opensearch.DescribeReservedInstanceOfferingsOutput{ + ReservedInstanceOfferings: []types.ReservedInstanceOffering{}, + NextToken: aws.String(""), + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec, "") + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "no offerings found") + } + mockOS.AssertNumberOfCalls(t, "DescribeReservedInstanceOfferings", 1) +} diff --git a/providers/aws/services/rds/client_test.go b/providers/aws/services/rds/client_test.go index 01fe1f385..434e38f19 100644 --- a/providers/aws/services/rds/client_test.go +++ b/providers/aws/services/rds/client_test.go @@ -983,6 +983,51 @@ func TestNormalizeEngineName_EditionTokensPassThrough(t *testing.T) { } } +// TestFindOfferingID_CtxCancelledBeforePage asserts that findOfferingID returns +// context.Canceled immediately when the context is already cancelled before the +// first pagination iteration, without calling the AWS API (issue #515). +func TestFindOfferingID_CtxCancelledBeforePage(t *testing.T) { + mockRDS := &MockRDSClient{} + t.Cleanup(func() { mockRDS.AssertExpectations(t) }) + client := &Client{client: mockRDS, region: "us-east-1"} + + rec := idempotencyTestRec() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before the first iteration + + // The mock must not be called: ctx.Err() fires at the top of the loop. + _, err := client.findOfferingID(ctx, rec, "") + + assert.ErrorIs(t, err, context.Canceled) + mockRDS.AssertNumberOfCalls(t, "DescribeReservedDBInstancesOfferings", 0) +} + +// TestFindOfferingID_EmptyStringTokenEndsPagination asserts that a page whose +// Marker is a pointer to an empty string (rather than nil) is treated as the +// terminal page and does not cause an extra API call (issue #515). +func TestFindOfferingID_EmptyStringTokenEndsPagination(t *testing.T) { + mockRDS := &MockRDSClient{} + t.Cleanup(func() { mockRDS.AssertExpectations(t) }) + client := &Client{client: mockRDS, region: "us-east-1"} + + rec := idempotencyTestRec() + + // Single page with zero results and Marker = ""; must not loop again. + mockRDS.On("DescribeReservedDBInstancesOfferings", mock.Anything, mock.Anything). + Return(&rds.DescribeReservedDBInstancesOfferingsOutput{ + ReservedDBInstancesOfferings: []types.ReservedDBInstancesOffering{}, + Marker: aws.String(""), + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec, "") + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "no offerings found") + } + mockRDS.AssertNumberOfCalls(t, "DescribeReservedDBInstancesOfferings", 1) +} + // TestClient_PurchaseCommitment_NoToken_RichReservationName asserts the // no-token CLI path (issue #687) composes a self-describing // ReservedDBInstanceId carrying the service code, region, SKU, count, and diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index 2899fc1f9..407c5ce1d 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -1491,3 +1491,48 @@ func TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall(t *testing.T) { } mockRS.AssertNotCalled(t, "DescribeReservedNodeOfferings", mock.Anything, mock.Anything) } + +// TestFindOfferingID_CtxCancelledBeforePage asserts that findOfferingID returns +// context.Canceled immediately when the context is already cancelled before the +// first pagination iteration, without calling the AWS API (issue #515). +func TestFindOfferingID_CtxCancelledBeforePage(t *testing.T) { + mockRS := &MockRedshiftClient{} + t.Cleanup(func() { mockRS.AssertExpectations(t) }) + client := &Client{client: mockRS, region: "us-east-1"} + + rec := rsIdemRec() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before the first iteration + + // The mock must not be called: ctx.Err() fires at the top of the loop. + _, err := client.findOfferingID(ctx, rec, "") + + assert.ErrorIs(t, err, context.Canceled) + mockRS.AssertNumberOfCalls(t, "DescribeReservedNodeOfferings", 0) +} + +// TestFindOfferingID_EmptyStringTokenEndsPagination asserts that a page whose +// Marker is a pointer to an empty string (rather than nil) is treated as the +// terminal page and does not cause an extra API call (issue #515). +func TestFindOfferingID_EmptyStringTokenEndsPagination(t *testing.T) { + mockRS := &MockRedshiftClient{} + t.Cleanup(func() { mockRS.AssertExpectations(t) }) + client := &Client{client: mockRS, region: "us-east-1"} + + rec := rsIdemRec() + + // Single page with no matching node type and Marker = ""; must not loop again. + mockRS.On("DescribeReservedNodeOfferings", mock.Anything, mock.Anything). + Return(&redshift.DescribeReservedNodeOfferingsOutput{ + ReservedNodeOfferings: []types.ReservedNodeOffering{}, + Marker: aws.String(""), + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec, "") + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "no offerings found") + } + mockRS.AssertNumberOfCalls(t, "DescribeReservedNodeOfferings", 1) +} diff --git a/providers/aws/services/savingsplans/client_test.go b/providers/aws/services/savingsplans/client_test.go index d42441d8a..a47db1057 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -1303,6 +1303,47 @@ func TestLookupOfferingID_DeterministicSortAcrossPages(t *testing.T) { "findOfferingID must sort across pages before selecting") } +// TestLookupOfferingID_CtxCancelledBeforePage asserts that lookupOfferingID +// returns context.Canceled immediately when the context is already cancelled +// before the first pagination iteration, without calling the AWS API (issue #515). +func TestLookupOfferingID_CtxCancelledBeforePage(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{client: mockSP, region: "us-east-1", planType: types.SavingsPlanTypeCompute} + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before the first iteration + + // The mock must not be called: ctx.Err() fires at the top of the loop. + _, err := client.findOfferingID(ctx, spRec(), "") + + assert.ErrorIs(t, err, context.Canceled) + mockSP.AssertNumberOfCalls(t, "DescribeSavingsPlansOfferings", 0) +} + +// TestLookupOfferingID_EmptyStringTokenEndsPagination asserts that a page whose +// NextToken is a pointer to an empty string (rather than nil) is treated as the +// terminal page and does not cause an extra API call (issue #515). +func TestLookupOfferingID_EmptyStringTokenEndsPagination(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{client: mockSP, region: "us-east-1", planType: types.SavingsPlanTypeCompute} + + // Single page with zero results and NextToken = ""; must not loop again. + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, mock.Anything). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{}, + NextToken: aws.String(""), + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), spRec(), "") + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "no Savings Plans offerings found") + } + mockSP.AssertNumberOfCalls(t, "DescribeSavingsPlansOfferings", 1) +} + // --- C1 adversarial-review regression tests --- // These tests pin the fix for the wrong-family purchase defect: an EC2Instance // SP recommendation carries InstanceFamily and Region from Cost Explorer, and