chore(providers/aws): periodic audit of findOfferingID across 7 services (closes #515)

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]: <Service> 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/<service>/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.

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)
+}

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)
+}

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{}