fix(purchase): address CodeRabbit review on PR #684

Three changes per CR pass-1 triage:

1. Actionable (execution.go): distinguish context.DeadlineExceeded from
   context.Canceled in the post-PurchaseCommitment error log. The previous
   branch logged "per-rec deadline 30s exceeded" for any non-nil recCtx.Err(),
   including parent cancellations -- mislabeling Canceled as a timeout makes
   diagnostic triage harder. Logic extracted into logRecCtxErr helper to keep
   executeSinglePurchase cyclomatic complexity within the gocyclo limit.

2. Nitpick (execution_test.go): replace mock.Anything with a deadline-aware
   mock.MatchedBy on the first arg of all 12 CreateAndValidateProvider
   expectations. The 30s per-rec budget is set before the factory call, so
   all three mocked stages (factory, GetServiceClient, PurchaseCommitment)
   now assert deadline propagation end-to-end.

3. Nitpick (purchasecfg/config.go + config_test.go): preserve the caller's
   *http.Client when one is provided in base config. NewConfig now shallow-
   copies the client and only overrides Timeout, keeping any custom Transport,
   Jar, or CheckRedirect. Falls back to a fresh client when base.HTTPClient is
   nil or a non-*http.Client implementation. Adds
   TestNewConfig_PreservesCustomTransport to pin the contract.

Author: cristim

internal/purchase/execution.go

@@ -2,6 +2,7 @@ package purchase
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strconv"
 	"strings"
@@ -684,6 +685,24 @@ func (m *Manager) buildPurchaseConfirmationData(exec *config.PurchaseExecution,
 	return data
 }
 
+// logRecCtxErr emits a diagnostic log line when a per-recommendation context has
+// been cancelled or timed out. It distinguishes DeadlineExceeded (the 30s per-rec
+// budget fired) from Canceled (a parent context stopped the execution) so that
+// CloudWatch filters can tell the two apart without parsing error strings.
+// It is a no-op when recCtxErr is nil.
+func logRecCtxErr(executionID, recTuple string, elapsed time.Duration, recCtxErr error) {
+	if recCtxErr == nil {
+		return
+	}
+	if errors.Is(recCtxErr, context.DeadlineExceeded) {
+		logging.Errorf("purchase[%s]: per-rec deadline exceeded for %s (elapsed=%s)",
+			executionID, recTuple, elapsed)
+	} else if errors.Is(recCtxErr, context.Canceled) {
+		logging.Errorf("purchase[%s]: recommendation context canceled for %s (elapsed=%s)",
+			executionID, recTuple, elapsed)
+	}
+}
+
 // executeSinglePurchase executes a single purchase using the appropriate provider.
 // provCfg carries optional per-account credentials; pass nil to use ambient credentials.
 // opts carries execution-level metadata (the source surface) that providers stamp
@@ -788,10 +807,7 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen
 	result, err := serviceClient.PurchaseCommitment(recCtx, recommendation, opts)
 	elapsed := time.Since(tCall)
 	if err != nil {
-		if recCtx.Err() != nil {
-			logging.Errorf("purchase[%s]: per-rec deadline 30s exceeded for %s (total elapsed %s)",
-				opts.ExecutionID, recTuple, elapsed)
-		}
+		logRecCtxErr(opts.ExecutionID, recTuple, elapsed, recCtx.Err())
 		logging.Errorf("purchase[%s]: %s/%s/%s/%s PurchaseCommitment failed after %s: %v",
 			opts.ExecutionID, rec.Provider, rec.Service, rec.Region, rec.ResourceType, elapsed, err)
 		return result, fmt.Errorf("purchase failed: %w", err)

internal/purchase/execution_test.go

@@ -67,7 +67,7 @@ func TestManager_ExecutePurchase(t *testing.T) {
 	}, nil)
 
 	// Mock provider factory to return a mock provider
-	mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil)
+	mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), "aws", mock.Anything).Return(mockProviderInst, nil)
 	mockProviderInst.On("GetServiceClient", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil)
 	mockServiceClient.On("PurchaseCommitment", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{
 		Success:      true,
@@ -127,7 +127,7 @@ func TestManager_ExecutePurchase_WebSourcePropagates(t *testing.T) {
 	mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")).Return(&sts.GetCallerIdentityOutput{
 		Account: aws.String("123456789012"),
 	}, nil)
-	mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil)
+	mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), "aws", mock.Anything).Return(mockProviderInst, nil)
 	mockProviderInst.On("GetServiceClient", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil)
 	// The deterministic idempotency token (issue #636) is derived per-rec from
 	// the execution ID and the rec's index, so the web path now also carries it.
@@ -183,7 +183,7 @@ func TestManager_ExecutePurchase_InvalidSourceFallsBackUntagged(t *testing.T) {
 	mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")).Return(&sts.GetCallerIdentityOutput{
 		Account: aws.String("123456789012"),
 	}, nil)
-	mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil)
+	mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), "aws", mock.Anything).Return(mockProviderInst, nil)
 	mockProviderInst.On("GetServiceClient", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil)
 	// Expect EMPTY source, not "cudly-evil" — NormalizeSource must have wiped it.
 	// The idempotency token (issue #636) is still derived regardless of source.
@@ -528,7 +528,7 @@ func TestManager_ExecutePurchase_MultiAccount(t *testing.T) {
 	mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil).Times(2)
 	mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil).Times(2)
 
-	mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil)
+	mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), "aws", mock.Anything).Return(mockProvider, nil)
 	mockProvider.On("GetServiceClient", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil)
 	mockServiceClient.On("PurchaseCommitment", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{
 		Success: true, CommitmentID: "ri-99999",
@@ -756,7 +756,7 @@ func TestExecuteMultiAccount_PartialFailure_IsolatesAccounts(t *testing.T) {
 
 	// Only account-V reaches the provider; account-I fails at credential resolution.
 	// Use .Once() so testify fails if the factory is called for account-I as well.
-	mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil).Once()
+	mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), "aws", mock.Anything).Return(mockProviderInst, nil).Once()
 	mockProviderInst.On("GetServiceClient", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil).Once()
 	mockServiceClient.On("PurchaseCommitment", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }),
 		mock.AnythingOfType("common.Recommendation"),
@@ -921,7 +921,7 @@ func TestExecuteMultiAccount_RunsAccountsInParallel(t *testing.T) {
 		return nil
 	}
 
-	mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil).Twice()
+	mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), "aws", mock.Anything).Return(mockProviderInst, nil).Twice()
 	mockProviderInst.On("GetServiceClient", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil).Twice()
 
 	// Each PurchaseCommitment call blocks for perCallDelay. Under serial
@@ -1039,7 +1039,7 @@ func TestExecutePurchase_SingleAccount_AzureUsesResolvedCreds(t *testing.T) {
 	// The core assertion: factory must receive a non-nil provCfg.
 	// Before the fix, nil was passed and Azure SDK fell back to DefaultAzureCredential.
 	// After the fix, resolveAzureProvider populates ProviderOverride on the config.
-	mockFactory.On("CreateAndValidateProvider", mock.Anything, "azure",
+	mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), "azure",
 		mock.MatchedBy(func(cfg *provider.ProviderConfig) bool {
 			return cfg != nil && cfg.ProviderOverride != nil
 		}),
@@ -1168,7 +1168,7 @@ func TestExecutePurchase_AzureCanonicalServiceTypes(t *testing.T) {
 				Account: aws.String("123456789012"),
 			}, nil)
 
-			mockFactory.On("CreateAndValidateProvider", mock.Anything, "azure",
+			mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), "azure",
 				mock.MatchedBy(func(cfg *provider.ProviderConfig) bool {
 					return cfg != nil && cfg.ProviderOverride != nil
 				}),
@@ -1391,7 +1391,7 @@ func TestManager_ExecuteAndFinalize_HistorySaveFailure_StaysVisible(t *testing.T
 	mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")).Return(&sts.GetCallerIdentityOutput{
 		Account: aws.String("123456789012"),
 	}, nil)
-	mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil)
+	mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), "aws", mock.Anything).Return(mockProviderInst, nil)
 	mockProviderInst.On("GetServiceClient", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil)
 	mockServiceClient.On("PurchaseCommitment", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{
 		Success:      true,
@@ -1465,7 +1465,7 @@ func TestManager_ExecuteAndFinalize_SingleAccount_PartialSuccess(t *testing.T) {
 		Account: aws.String("123456789012"),
 	}, nil).Maybe()
 
-	mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil)
+	mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), "aws", mock.Anything).Return(mockProviderInst, nil)
 	mockProviderInst.On("GetServiceClient", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil)
 	// m5.large succeeds; c5.xlarge fails.
 	mockServiceClient.On("PurchaseCommitment", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }),
@@ -1547,7 +1547,7 @@ func TestExecuteForAccount_PartialSuccess(t *testing.T) {
 		},
 	}
 
-	mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil)
+	mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), "aws", mock.Anything).Return(mockProviderInst, nil)
 	mockProviderInst.On("GetServiceClient", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil)
 	mockServiceClient.On("PurchaseCommitment", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }),
 		mock.MatchedBy(func(r common.Recommendation) bool { return r.ResourceType == "m5.large" }),
@@ -1685,7 +1685,7 @@ func TestManager_ExecutePurchase_SingleAccount_StampsTargetAccount(t *testing.T)
 			mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")).
 				Return(&sts.GetCallerIdentityOutput{Account: aws.String(hostAccount)}, nil).Maybe()
 
-			mockFactory.On("CreateAndValidateProvider", mock.Anything, tc.provider, mock.Anything).Return(mockProviderInst, nil)
+			mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), tc.provider, mock.Anything).Return(mockProviderInst, nil)
 			mockProviderInst.On("GetServiceClient", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }), tc.expectedType, tc.region).Return(mockServiceClient, nil)
 			mockServiceClient.On("PurchaseCommitment", mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok }),
 				mock.AnythingOfType("common.Recommendation"),

providers/aws/internal/purchasecfg/config.go

@@ -38,9 +38,20 @@ const (
 // Applies:
 //   - RetryMaxAttempts = 2 (overrides the SDK default of 3)
 //   - HTTPClient with Timeout = 15s (overrides the SDK default of no timeout)
+//
+// If base.HTTPClient is an *http.Client, it is cloned and its Timeout is set
+// to HTTPTimeout, preserving any custom Transport, Jar, or CheckRedirect the
+// caller installed. If base.HTTPClient is nil or a non-*http.Client
+// implementation, a fresh *http.Client{Timeout: HTTPTimeout} is used instead.
 func NewConfig(base aws.Config) aws.Config {
 	cfg := base.Copy()
 	cfg.RetryMaxAttempts = MaxAttempts
-	cfg.HTTPClient = &http.Client{Timeout: HTTPTimeout}
+	if hc, ok := base.HTTPClient.(*http.Client); ok && hc != nil {
+		clone := *hc // shallow copy preserves Transport, Jar, CheckRedirect
+		clone.Timeout = HTTPTimeout
+		cfg.HTTPClient = &clone
+	} else {
+		cfg.HTTPClient = &http.Client{Timeout: HTTPTimeout}
+	}
 	return cfg
 }

providers/aws/internal/purchasecfg/config_test.go

@@ -52,6 +52,29 @@ func TestNewConfig_DoesNotMutateBase(t *testing.T) {
 	assert.Equal(t, 60*time.Second, httpClient.Timeout, "NewConfig must not modify the original config's HTTPClient")
 }
 
+// TestNewConfig_PreservesCustomTransport verifies that a non-nil *http.Client
+// supplied in the base config has its Transport kept intact in the result.
+// NewConfig must not silently drop custom transports, TLS settings, or
+// instrumentation hooks by replacing the whole client unconditionally.
+func TestNewConfig_PreservesCustomTransport(t *testing.T) {
+	t.Helper()
+	customTransport := &http.Transport{MaxIdleConns: 42}
+	base := aws.Config{
+		Region:     "us-east-1",
+		HTTPClient: &http.Client{Transport: customTransport, Timeout: 60 * time.Second},
+	}
+
+	cfg := NewConfig(base)
+
+	require.NotNil(t, cfg.HTTPClient, "HTTPClient must not be nil after NewConfig")
+	httpClient, ok := cfg.HTTPClient.(*http.Client)
+	require.True(t, ok, "HTTPClient must be *http.Client")
+	assert.Equal(t, HTTPTimeout, httpClient.Timeout,
+		"NewConfig must override the Timeout to HTTPTimeout even when cloning a caller-provided client")
+	assert.Same(t, customTransport, httpClient.Transport,
+		"NewConfig must preserve the caller-provided Transport on the cloned client")
+}
+
 func TestNewConfig_PreservesRegion(t *testing.T) {
 	t.Helper()
 	base := aws.Config{Region: "ap-southeast-1"}