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

* 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.

* fix(exchange): handle ParseDecimalRat error, extract checkCapsAndComputeHeadroom

Fixes golangci-lint errcheck failure on handler_ri_exchange.go:1351 where
`exchange.ParseDecimalRat` error was discarded with `_`. The fix extracts a
`checkCapsAndComputeHeadroom` helper that bundles the three cap-validation
checks (MaxDailyUSD==0, checkDailyCap, MaxPerExchangeUSD==0) and the headroom
calculation, properly returning a reason string on any parse failure (fail
closed). This also brings `executeApprovedExchange` back under gocyclo -over 10
(complexity 8) after the helper reduces it from the 12 that errcheck enforcement
would have caused.

Rebased onto main (after PR #1456 merged; no conflicts in store_postgres.go).

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,78 @@ 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"
+}
+
+// checkCapsAndComputeHeadroom validates the spending-cap configuration, runs the
+// daily-cap check, and computes the effective MaxPaymentDueUSD that Execute must
+// not exceed (H2: remaining daily headroom vs per-exchange cap, whichever is
+// smaller). Returns a non-empty reason string on any failure so the caller can
+// forward it to failExchange.
+func checkCapsAndComputeHeadroom(dailySpendStr, paymentDue string, cfg *config.GlobalConfig) (effectiveCap *big.Rat, reason string) {
+	if cfg.RIExchangeMaxDailyUSD == 0 {
+		return nil, "daily spending cap is not configured (RIExchangeMaxDailyUSD is 0)"
+	}
+	if reason := checkDailyCap(dailySpendStr, paymentDue, cfg.RIExchangeMaxDailyUSD); reason != "" {
+		return nil, reason
+	}
+	if cfg.RIExchangeMaxPerExchangeUSD == 0 {
+		return nil, "per-exchange spending cap is not configured (RIExchangeMaxPerExchangeUSD is 0)"
+	}
+	// checkDailyCap already verified dailySpendStr is parseable; a second failure
+	// is an internal error - fail closed to avoid executing with wrong headroom.
+	dailySpent, err := exchange.ParseDecimalRat(dailySpendStr)
+	if err != nil || dailySpent == nil {
+		return nil, fmt.Sprintf("daily spend re-parse failed (internal error): %v", err)
+	}
+	dailyCap := new(big.Rat).SetFloat64(cfg.RIExchangeMaxDailyUSD)
+	perExchangeCap := new(big.Rat).SetFloat64(cfg.RIExchangeMaxPerExchangeUSD)
+	return handlerChooseEffectiveCap(dailyCap, dailySpent, perExchangeCap), ""
+}
+
 // 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())
@@ -1280,13 +1352,6 @@ func (h *Handler) executeApprovedExchange(ctx context.Context, id string, record
 		return h.failExchange(ctx, id, "config load failed")
 	}
 
-	if globalCfg.RIExchangeMaxDailyUSD == 0 {
-		return h.failExchange(ctx, id, "daily spending cap is not configured (RIExchangeMaxDailyUSD is 0)")
-	}
-	if reason := checkDailyCap(dailySpendStr, record.PaymentDue, globalCfg.RIExchangeMaxDailyUSD); reason != "" {
-		return h.failExchange(ctx, id, reason)
-	}
-
 	region := record.Region
 	if region == "" {
 		// Region is a required field captured at record-creation time.
@@ -1295,31 +1360,46 @@ func (h *Handler) executeApprovedExchange(ctx context.Context, id string, record
 		return h.failExchange(ctx, id, "exchange record has no region; cannot execute safely")
 	}
 
-	if globalCfg.RIExchangeMaxPerExchangeUSD == 0 {
-		return h.failExchange(ctx, id, "per-exchange spending cap is not configured (RIExchangeMaxPerExchangeUSD is 0)")
+	effectiveCap, reason := checkCapsAndComputeHeadroom(dailySpendStr, record.PaymentDue, globalCfg)
+	if reason != "" {
+		return h.failExchange(ctx, id, reason)
 	}
 
-	perExchangeCap := new(big.Rat).SetFloat64(globalCfg.RIExchangeMaxPerExchangeUSD)
-	exchangeID, _, execErr := exchange.ExecuteExchange(ctx, exchange.ExchangeExecuteRequest{
+	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 +1411,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)