fix(exchange): enforce daily-cap integrity on manual path, actual-paid ledger, and headroom (adversarial-review follow-ups)

H1: checkDailyCap fails closed on unparseable paymentDue (was treating as $0).
H2: executeApprovedExchange and processAutoExchange bound Execute MaxPaymentDueUSD
    by min(perExchangeCap, dailyCap-dailySpent) so re-quotes cannot breach headroom.
H3: Add CompleteRIExchangeWithPayment to persist the accepted (not pre-execution)
    amount; both auto and manual paths use the fresh quote from Execute.
H4: Retry ledger write 3x after money moves; halt auto run / return 500 on
    persistent failure to prevent cap bypass via missing ledger rows.
M5: GetRIExchangeDailySpend includes processing rows (COALESCE on completed_at/
    updated_at) to close concurrent-approval TOCTOU window.

Each defect has a regression test that fails pre-fix and passes post-fix.

Author: cristim

internal/analytics/collector_test.go

@@ -313,6 +313,9 @@ func (m *mockConfigStore) TransitionRIExchangeStatus(ctx context.Context, id str
 func (m *mockConfigStore) CompleteRIExchange(ctx context.Context, id string, exchangeID string) error {
 	return nil
 }
+func (m *mockConfigStore) CompleteRIExchangeWithPayment(_ context.Context, _, _, _ string) error {
+	return nil
+}
 func (m *mockConfigStore) FailRIExchange(ctx context.Context, id string, errorMsg string) error {
 	return nil
 }

internal/api/exchange_helpers_test.go

@@ -36,10 +36,12 @@ func TestCheckDailyCap_InvalidDailySpend(t *testing.T) {
 }
 
 func TestCheckDailyCap_InvalidPaymentDue(t *testing.T) {
-	// Unparseable payment due → treated as $0, within cap
+	// H1 fix: an unparseable payment-due string must fail closed (return a
+	// blocking reason) instead of being treated as $0. Proceeding as $0 would
+	// allow an exchange of unknown cost through the daily cap check.
 	reason := checkDailyCap("100.00", "not-a-number", 500.0)
-	// $100 + $0 = $100 < $500 → allowed
-	assert.Equal(t, "", reason)
+	assert.NotEmpty(t, reason, "unparseable payment due must block the exchange (fail closed)")
+	assert.Contains(t, reason, "could not parse payment due")
 }
 
 func TestCheckDailyCap_ExactlyAtCap(t *testing.T) {

internal/api/handler.go

@@ -16,6 +16,7 @@ import (
 	"github.com/LeanerCloud/CUDly/internal/email"
 	"github.com/LeanerCloud/CUDly/internal/oidc"
 	"github.com/LeanerCloud/CUDly/internal/runtime"
+	"github.com/LeanerCloud/CUDly/pkg/exchange"
 	"github.com/LeanerCloud/CUDly/pkg/logging"
 	"github.com/aws/aws-lambda-go/events"
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -120,6 +121,12 @@ type Handler struct {
 	// validation no-ops, deferring to the frontend's hardcoded rules.
 	commitmentOpts CommitmentOptsInterface
 
+	// executeExchangeFn is the RI exchange execution function injected by tests.
+	// When nil (the production default), executeApprovedExchange calls
+	// exchange.ExecuteExchange directly. Tests inject a stub so the handler
+	// can be exercised end-to-end without live AWS credentials or a real RI.
+	executeExchangeFn func(ctx context.Context, req exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error)
+
 	// encryptionKeySource is the env var name that resolved the credential
 	// encryption key. Empty when no credStore is configured. Used by the
 	// /health endpoint only — never logged outside that one place.

internal/api/handler_ri_exchange.go

@@ -1268,6 +1268,52 @@ func (h *Handler) failExchange(ctx context.Context, id, reason string) (any, err
 	return map[string]any{"status": "failed", "reason": reason}, nil
 }
 
+// retryCompleteWithPayment calls CompleteRIExchangeWithPayment up to
+// maxLedgerAttempts times, logging retries. Returns a non-nil error if all
+// attempts fail. Extracted from executeApprovedExchange to reduce its
+// cyclomatic complexity (H4 fix).
+func (h *Handler) retryCompleteWithPayment(ctx context.Context, id, exchangeID, acceptedPaymentDue string) error {
+	const maxAttempts = 3
+	var err error
+	for attempt := 1; attempt <= maxAttempts; attempt++ {
+		err = h.config.CompleteRIExchangeWithPayment(ctx, id, exchangeID, acceptedPaymentDue)
+		if err == nil {
+			return nil
+		}
+		if attempt < maxAttempts {
+			logging.Warnf("ledger write retry %d/%d for exchange %s after money moved: %v",
+				attempt, maxAttempts, id, err)
+		}
+	}
+	return err
+}
+
+// handlerChooseEffectiveCap returns the smaller of perExchangeCap and daily
+// headroom (dailyCap - dailySpent), bounding Execute's MaxPaymentDueUSD so a
+// fresh re-quote cannot exceed the remaining daily budget (H2 fix).
+func handlerChooseEffectiveCap(dailyCap, dailySpent, perExchangeCap *big.Rat) *big.Rat {
+	remaining := new(big.Rat).Sub(dailyCap, dailySpent)
+	if remaining.Cmp(perExchangeCap) < 0 {
+		return remaining
+	}
+	return perExchangeCap
+}
+
+// handlerAcceptedAmount extracts the confirmed payment amount from a fresh
+// Execute quote, falling back to fallback when freshQ is nil or empty (H3 fix).
+func handlerAcceptedAmount(freshQ *exchange.ExchangeQuoteSummary, fallback string) string {
+	if freshQ == nil {
+		return fallback
+	}
+	if freshQ.PaymentDueUSDStr != "" {
+		return freshQ.PaymentDueUSDStr
+	}
+	// Zero-cost exchange: PaymentDueRaw was empty (AWS returned nil) so
+	// PaymentDueUSDStr is also empty. Use "0" to avoid a NULL payment_due in
+	// the DB that would silently distort GetRIExchangeDailySpend's SUM.
+	return "0"
+}
+
 // executeApprovedExchange checks caps and executes the exchange after approval.
 func (h *Handler) executeApprovedExchange(ctx context.Context, id string, record *config.RIExchangeRecord) (any, error) {
 	dailySpendStr, err := h.config.GetRIExchangeDailySpend(ctx, time.Now())
@@ -1299,27 +1345,49 @@ func (h *Handler) executeApprovedExchange(ctx context.Context, id string, record
 		return h.failExchange(ctx, id, "per-exchange spending cap is not configured (RIExchangeMaxPerExchangeUSD is 0)")
 	}
 
+	// H2: bound Execute's MaxPaymentDueUSD by remaining daily headroom so a
+	// fresh re-quote inside Execute cannot accept an amount that would breach
+	// the daily cap. dailySpend was verified parseable by checkDailyCap above.
+	dailySpent, _ := exchange.ParseDecimalRat(dailySpendStr)
+	dailyCap := new(big.Rat).SetFloat64(globalCfg.RIExchangeMaxDailyUSD)
 	perExchangeCap := new(big.Rat).SetFloat64(globalCfg.RIExchangeMaxPerExchangeUSD)
-	exchangeID, _, execErr := exchange.ExecuteExchange(ctx, exchange.ExchangeExecuteRequest{
+	effectiveCap := handlerChooseEffectiveCap(dailyCap, dailySpent, perExchangeCap)
+
+	execFn := exchange.ExecuteExchange
+	if h.executeExchangeFn != nil {
+		execFn = h.executeExchangeFn
+	}
+	exchangeID, freshQ, execErr := execFn(ctx, exchange.ExchangeExecuteRequest{
 		Region:           region,
 		ReservedIDs:      record.SourceRIIDs,
 		TargetOfferingID: record.TargetOfferingID,
 		TargetCount:      int32(record.TargetCount), // #nosec G115 -- RI quantity stored from validated API request; AWS limits RI counts well below math.MaxInt32
-		MaxPaymentDueUSD: perExchangeCap,
+		MaxPaymentDueUSD: effectiveCap,
 	})
 	if execErr != nil {
 		return h.failExchange(ctx, id, execErr.Error())
 	}
 
-	if err := h.config.CompleteRIExchange(ctx, id, exchangeID); err != nil {
-		logging.Errorf("failed to mark exchange %s as completed: %v", id, err)
+	// H3: persist the amount AWS actually accepted, not the stale pre-execution
+	// quote stored in record.PaymentDue.
+	acceptedPaymentDue := handlerAcceptedAmount(freshQ, record.PaymentDue)
+
+	// H4: retry the ledger write via retryCompleteWithPayment; persistent
+	// failure is returned as an error (HTTP 500) so the caller knows money
+	// moved but the record was not updated.
+	if completeErr := h.retryCompleteWithPayment(ctx, id, exchangeID, acceptedPaymentDue); completeErr != nil {
+		logging.Errorf("all ledger write attempts failed for exchange %s after money moved: %v",
+			id, completeErr)
+		return nil, fmt.Errorf("exchange executed (id=%s) but ledger update failed: %w",
+			exchangeID, completeErr)
 	}
 
 	return map[string]any{"status": "completed", "exchange_id": exchangeID}, nil
 }
 
 // checkDailyCap verifies the exchange payment won't exceed the daily spending cap.
-// Returns an empty string if within cap, or a reason string if exceeded.
+// Returns an empty string if within cap, or a reason string if exceeded or if
+// either input cannot be parsed (fail closed on parse errors).
 func checkDailyCap(dailySpendStr, paymentDueStr string, maxDailyUSD float64) string {
 	dailyCap := new(big.Rat).SetFloat64(maxDailyUSD)
 	dailySpent, err := exchange.ParseDecimalRat(dailySpendStr)
@@ -1331,8 +1399,11 @@ func checkDailyCap(dailySpendStr, paymentDueStr string, maxDailyUSD float64) str
 	}
 	paymentDue, err := exchange.ParseDecimalRat(paymentDueStr)
 	if err != nil || paymentDue == nil {
-		logging.Warnf("checkDailyCap: failed to parse payment due string %q: %v; treating as $0", paymentDueStr, err)
-		paymentDue = new(big.Rat)
+		// H1 fix: fail closed on an unparseable payment-due string instead of
+		// treating it as $0. An unparseable value means we cannot determine the
+		// true cost of this exchange, so proceeding risks exceeding the cap.
+		logging.Warnf("checkDailyCap: failed to parse payment due string %q: %v; blocking exchange to avoid cap bypass", paymentDueStr, err)
+		return fmt.Sprintf("daily spend check failed: could not parse payment due value %q", paymentDueStr)
 	}
 
 	newTotal := new(big.Rat).Add(dailySpent, paymentDue)

internal/api/handler_ri_exchange_test.go

@@ -3,11 +3,13 @@ package api
 import (
 	"context"
 	"fmt"
+	"math/big"
 	"testing"
 	"time"
 
 	"github.com/LeanerCloud/CUDly/internal/auth"
 	"github.com/LeanerCloud/CUDly/internal/config"
+	"github.com/LeanerCloud/CUDly/pkg/exchange"
 	"github.com/LeanerCloud/CUDly/providers/aws/recommendations"
 	ec2svc "github.com/LeanerCloud/CUDly/providers/aws/services/ec2"
 	azurecompute "github.com/LeanerCloud/CUDly/providers/azure/services/compute"
@@ -1582,3 +1584,190 @@ func TestRejectRIExchange_TokenPathActorIsNil(t *testing.T) {
 	_, err := (&Handler{config: mockStore}).rejectRIExchange(ctx, id, "tok")
 	require.NoError(t, err)
 }
+
+// ─── H1: checkDailyCap fails closed on unparseable paymentDue ─────────────────
+
+// TestCheckDailyCap_UnparseablePaymentDue_FailsClosed is the regression test
+// for H1: checkDailyCap must return a blocking reason when paymentDueStr cannot
+// be parsed, never treat it as $0 (which would allow an unknown-cost exchange
+// to proceed when the daily cap might be exceeded).
+func TestCheckDailyCap_UnparseablePaymentDue_FailsClosed(t *testing.T) {
+	t.Parallel()
+	cases := []struct {
+		name          string
+		paymentDueStr string
+	}{
+		{"not-a-number", "not-a-number"},
+		{"empty string", ""},
+	}
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			reason := checkDailyCap("100.00", tc.paymentDueStr, 500.0)
+			assert.NotEmpty(t, reason,
+				"checkDailyCap must fail closed when paymentDueStr=%q cannot be parsed", tc.paymentDueStr)
+			assert.Contains(t, reason, "could not parse payment due",
+				"reason must reference the payment-due parsing failure, not treat it as $0")
+		})
+	}
+}
+
+// TestCheckDailyCap_ValidPaymentDue_WithinCap_Passes verifies the happy path:
+// valid parseable amounts within the daily cap must return an empty reason.
+func TestCheckDailyCap_ValidPaymentDue_WithinCap_Passes(t *testing.T) {
+	t.Parallel()
+	reason := checkDailyCap("100.00", "50.00", 500.0)
+	assert.Empty(t, reason, "within-cap exchange must not be blocked")
+}
+
+// ─── H2: effectiveCap bounded by daily headroom ───────────────────────────────
+
+// TestExecuteApprovedExchange_EffectiveCap_BoundedByDailyHeadroom is the
+// regression test for H2: when dailyCap-dailySpent < perExchangeCap, the cap
+// passed to Execute (MaxPaymentDueUSD) must be the daily headroom, not the full
+// perExchangeCap. This prevents a fresh re-quote from accepting an amount that
+// exceeds the daily budget.
+func TestExecuteApprovedExchange_EffectiveCap_BoundedByDailyHeadroom(t *testing.T) {
+	ctx := context.Background()
+	const id = "550e8400-e29b-41d4-a716-000000000012"
+
+	mockStore := new(MockConfigStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+
+	// dailySpent=$450, dailyCap=$500, perExchangeCap=$100 -> headroom=$50
+	mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("450.00", nil)
+	mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{
+		RIExchangeMaxDailyUSD:       500,
+		RIExchangeMaxPerExchangeUSD: 100,
+	}, nil)
+
+	var capturedReq exchange.ExchangeExecuteRequest
+	h := &Handler{
+		config: mockStore,
+		executeExchangeFn: func(_ context.Context, req exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) {
+			capturedReq = req
+			return "exch-h2-handler", &exchange.ExchangeQuoteSummary{PaymentDueUSDStr: "40.000000"}, nil
+		},
+	}
+
+	mockStore.On("CompleteRIExchangeWithPayment", ctx, id, "exch-h2-handler", "40.000000").Return(nil)
+
+	record := &config.RIExchangeRecord{
+		ID:               id,
+		Region:           "us-east-1",
+		SourceRIIDs:      []string{"ri-1"},
+		TargetOfferingID: "offering-1",
+		TargetCount:      1,
+		PaymentDue:       "40.00",
+	}
+
+	_, err := h.executeApprovedExchange(ctx, id, record)
+	require.NoError(t, err)
+
+	// MaxPaymentDueUSD must equal headroom ($50), not perExchangeCap ($100).
+	require.NotNil(t, capturedReq.MaxPaymentDueUSD,
+		"MaxPaymentDueUSD must be set on the Execute request")
+	expectedCap := new(big.Rat).SetFrac64(50, 1)
+	assert.Equal(t, 0, capturedReq.MaxPaymentDueUSD.Cmp(expectedCap),
+		"MaxPaymentDueUSD must be daily headroom ($50), got $%s",
+		capturedReq.MaxPaymentDueUSD.FloatString(2))
+}
+
+// ─── H3: ledger records fresh accepted amount ─────────────────────────────────
+
+// TestExecuteApprovedExchange_AcceptedAmountFromFreshQuote is the regression
+// test for H3: CompleteRIExchangeWithPayment must be called with the amount
+// AWS confirmed during Execute (freshQ.PaymentDueUSDStr), not the stale
+// record.PaymentDue stored from the pre-execution quote.
+func TestExecuteApprovedExchange_AcceptedAmountFromFreshQuote(t *testing.T) {
+	ctx := context.Background()
+	const id = "550e8400-e29b-41d4-a716-000000000013"
+
+	mockStore := new(MockConfigStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+
+	mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil)
+	mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{
+		RIExchangeMaxDailyUSD:       1000,
+		RIExchangeMaxPerExchangeUSD: 500,
+	}, nil)
+
+	// Execute returns a fresh quote ($35.50) different from pre-execution ($30.00).
+	h := &Handler{
+		config: mockStore,
+		executeExchangeFn: func(_ context.Context, _ exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) {
+			return "exch-h3-fresh", &exchange.ExchangeQuoteSummary{
+				PaymentDueUSDStr: "35.500000",
+			}, nil
+		},
+	}
+
+	// The FRESH amount "35.500000" must reach CompleteRIExchangeWithPayment,
+	// not the stale "30.00" from record.PaymentDue.
+	mockStore.On("CompleteRIExchangeWithPayment", ctx, id, "exch-h3-fresh", "35.500000").Return(nil)
+
+	record := &config.RIExchangeRecord{
+		ID:               id,
+		Region:           "us-east-1",
+		SourceRIIDs:      []string{"ri-1"},
+		TargetOfferingID: "offering-1",
+		TargetCount:      1,
+		PaymentDue:       "30.00", // stale pre-execution quote
+	}
+
+	resp, err := h.executeApprovedExchange(ctx, id, record)
+	require.NoError(t, err)
+	respMap, ok := resp.(map[string]any)
+	require.True(t, ok)
+	assert.Equal(t, "completed", respMap["status"])
+}
+
+// ─── H4: persistent ledger failure returns error ──────────────────────────────
+
+// TestExecuteApprovedExchange_LedgerWriteFailure_ReturnsError is the regression
+// test for H4: when CompleteRIExchangeWithPayment fails persistently (all retry
+// attempts exhausted) after money has already moved, executeApprovedExchange must
+// return an error rather than silently logging and returning a success response.
+// The caller (approveRIExchange) must see this as an error so it can return
+// HTTP 500 and the operator knows the ledger is inconsistent.
+func TestExecuteApprovedExchange_LedgerWriteFailure_ReturnsError(t *testing.T) {
+	ctx := context.Background()
+	const id = "550e8400-e29b-41d4-a716-000000000014"
+
+	mockStore := new(MockConfigStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+
+	mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil)
+	mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{
+		RIExchangeMaxDailyUSD:       1000,
+		RIExchangeMaxPerExchangeUSD: 500,
+	}, nil)
+
+	h := &Handler{
+		config: mockStore,
+		executeExchangeFn: func(_ context.Context, _ exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) {
+			return "exch-h4-test", &exchange.ExchangeQuoteSummary{PaymentDueUSDStr: "0"}, nil
+		},
+	}
+
+	// Fail all three retry attempts.
+	mockStore.On("CompleteRIExchangeWithPayment", ctx, id, "exch-h4-test", "0").
+		Return(fmt.Errorf("DB write failed")).Times(3)
+
+	record := &config.RIExchangeRecord{
+		ID:               id,
+		Region:           "us-east-1",
+		SourceRIIDs:      []string{"ri-1"},
+		TargetOfferingID: "offering-1",
+		TargetCount:      1,
+		PaymentDue:       "0",
+	}
+
+	_, err := h.executeApprovedExchange(ctx, id, record)
+	require.Error(t, err, "persistent ledger failure must propagate as a non-nil error")
+	assert.Contains(t, err.Error(), "ledger update failed",
+		"error must describe the ledger failure for operator triage")
+	assert.Contains(t, err.Error(), "exch-h4-test",
+		"error must include the exchange ID so operators can correlate with AWS")
+}

internal/config/interfaces.go

@@ -246,9 +246,15 @@ type StoreInterface interface {
 	// actor is the UUID of the user performing the transition (nil for system-initiated paths).
 	TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string, actor *string) (*RIExchangeRecord, error)
 	CompleteRIExchange(ctx context.Context, id string, exchangeID string) error
+	// CompleteRIExchangeWithPayment marks an RI exchange as completed and
+	// updates payment_due to the amount AWS actually accepted. Use this instead
+	// of CompleteRIExchange on the manual-approval path so the daily-spend
+	// ledger (GetRIExchangeDailySpend) reflects the real accepted amount rather
+	// than the stale pre-execution quote (H3 fix).
+	CompleteRIExchangeWithPayment(ctx context.Context, id string, exchangeID string, acceptedPaymentDue string) error
 	// StampRIExchangeApprovedBy sets the approved_by column on a completed
-	// exchange row (issue #300). Called after CompleteRIExchange when the
-	// approval came from a session-authed user rather than an email token.
+	// exchange row (issue #300). Called after CompleteRIExchangeWithPayment when
+	// the approval came from a session-authed user rather than an email token.
 	StampRIExchangeApprovedBy(ctx context.Context, id string, approverEmail string) error
 	FailRIExchange(ctx context.Context, id string, errorMsg string) error
 	GetRIExchangeDailySpend(ctx context.Context, date time.Time) (string, error)