fix(exchange): refuse a quote with no PaymentDue instead of treating it as $0

internal/api/handler_ri_exchange.go

@@ -2387,17 +2387,18 @@ func handlerChooseEffectiveCap(dailyCap, dailySpent, perExchangeCap *big.Rat) *b
 
 // handlerAcceptedAmount extracts the confirmed payment amount from a fresh
 // Execute quote, falling back to fallback when freshQ is nil or empty (H3 fix).
+//
+// The empty case no longer means "zero-cost exchange": since #1964 an absent
+// PaymentDue is refused by checkInitialQuote and checkReQuote before Accept,
+// and a genuine zero arrives as an explicit "0.000000". Execute therefore
+// cannot return successfully with an empty amount, so this mirrors
+// exchange.acceptedAmountFromQuote and returns the caller's fallback rather
+// than fabricating "0".
 func handlerAcceptedAmount(freshQ *exchange.ExchangeQuoteSummary, fallback string) string {
-	if freshQ == nil {
-		return fallback
-	}
-	if freshQ.PaymentDueUSDStr != "" {
+	if freshQ != nil && 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"
+	return fallback
 }
 
 // checkCapsAndComputeHeadroom validates the spending-cap configuration, runs the

pkg/exchange/auto.go

@@ -237,13 +237,7 @@ func processRecommendation(ctx context.Context, params RunAutoExchangeParams, re
 		return false
 	}
 
-	// A nil PaymentDueUSD documents a zero-cost exchange (no payment due),
-	// distinct from a parse failure: processAutoExchange fails closed on any
-	// string that does not parse as a decimal.
-	paymentDueStr := "0"
-	if quote.PaymentDueUSD != nil {
-		paymentDueStr = quote.PaymentDueUSD.FloatString(6)
-	}
+	paymentDueStr := quote.PaymentDueUSD.FloatString(6)
 
 	if params.Config.Mode == "manual" {
 		outcome := processManualExchange(ctx, params, rec, offeringID, paymentDueStr)
@@ -289,8 +283,9 @@ func resolveOffering(ctx context.Context, params RunAutoExchangeParams, rec Resh
 	return offeringID, nil
 }
 
-// getValidatedQuote fetches and validates an exchange quote.
-// Returns the quote on success, or a SkippedRecommendation on failure.
+// getValidatedQuote fetches and validates an exchange quote. The returned
+// quote is valid, carries a PaymentDueUSD, and is within the per-exchange cap.
+// Returns a SkippedRecommendation otherwise.
 func getValidatedQuote(ctx context.Context, params RunAutoExchangeParams, rec ReshapeRecommendation, offeringID string, perExchangeCap *big.Rat) (*ExchangeQuoteSummary, *SkippedRecommendation) {
 	quote, err := params.ExchangeClient.GetQuote(ctx, ExchangeQuoteRequest{
 		Region:           params.Region,
@@ -315,7 +310,15 @@ func getValidatedQuote(ctx context.Context, params RunAutoExchangeParams, rec Re
 		}
 	}
 
-	if quote.PaymentDueUSD != nil && quote.PaymentDueUSD.Cmp(perExchangeCap) > 0 {
+	if quote.PaymentDueUSD == nil {
+		return nil, &SkippedRecommendation{
+			SourceRIID:         rec.SourceRIID,
+			SourceInstanceType: rec.SourceInstanceType,
+			Reason:             "quote reported no PaymentDue; cannot enforce the per-exchange cap against an unknown amount",
+		}
+	}
+
+	if quote.PaymentDueUSD.Cmp(perExchangeCap) > 0 {
 		return nil, &SkippedRecommendation{
 			SourceRIID:         rec.SourceRIID,
 			SourceInstanceType: rec.SourceInstanceType,
@@ -431,19 +434,15 @@ func chooseEffectiveCap(dailyCap, dailySpent, perExchangeCap *big.Rat) *big.Rat
 	return perExchangeCap
 }
 
-// acceptedAmountFromQuote returns the payment amount confirmed by a fresh
-// Execute quote, or fallback when freshQ is nil or carries an empty
-// PaymentDueUSDStr. Zero-cost exchanges (PaymentDueRaw empty, AWS returned
-// nil) are recorded as "0" so GetRIExchangeDailySpend's SUM is not distorted
-// by a NULL payment_due in the DB (H3 fix).
+// acceptedAmountFromQuote returns the payment amount confirmed by the fresh
+// Execute quote. Execute refuses a re-quote with no PaymentDue, so a fresh
+// quote without an amount can only come from a client that skipped that
+// check; the initial quoted amount is then the honest ledger value (H3 fix).
 func acceptedAmountFromQuote(freshQ *ExchangeQuoteSummary, fallback string) string {
-	if freshQ == nil {
-		return fallback
-	}
-	if freshQ.PaymentDueUSDStr != "" {
+	if freshQ != nil && freshQ.PaymentDueUSDStr != "" {
 		return freshQ.PaymentDueUSDStr
 	}
-	return "0"
+	return fallback
 }
 
 // saveLedgerRecord saves a completed exchange record with retry, returning a
@@ -511,11 +510,10 @@ func processAutoExchange(ctx context.Context, params RunAutoExchangeParams, rec
 		return outcome, false
 	}
 
-	// paymentDueStr is always a decimal here: processRecommendation sets it to
-	// "0" when the quote has no PaymentDueUSD (the documented nil-means-zero
-	// case) and to FloatString(6) otherwise. A parse failure therefore means a
-	// caller passed garbage; fail closed instead of counting $0 toward the
-	// daily cap, mirroring the dailySpent branch above.
+	// paymentDueStr is always FloatString(6) of an amount getValidatedQuote
+	// confirmed the quote carries. A parse failure therefore means a caller
+	// passed garbage; fail closed instead of counting $0 toward the daily cap,
+	// mirroring the dailySpent branch above.
 	paymentDue, err := ParseDecimalRat(paymentDueStr)
 	if err != nil {
 		logging.Errorf("failed to parse payment due %q for %s: %v", paymentDueStr, rec.SourceRIID, err)

pkg/exchange/auto_test.go

@@ -380,6 +380,90 @@ func TestProcessAutoExchange_UnparseablePaymentDue_FailsClosed(t *testing.T) {
 	}
 }
 
+// TestRunAutoExchange_MissingPaymentDue_RefusedBeforeAnyRecord is the
+// regression test for #1964 (audit A09-008): a quote that is valid but
+// carries no PaymentDue must be skipped before Execute is called or any
+// record is written, in both modes. Pre-fix, auto mode executed it and
+// manual mode issued an approval token for it, each recorded as costing "0".
+func TestRunAutoExchange_MissingPaymentDue_RefusedBeforeAnyRecord(t *testing.T) {
+	t.Parallel()
+	for _, mode := range []string{"auto", "manual"} {
+		t.Run(mode, func(t *testing.T) {
+			t.Parallel()
+			store := &mockExchangeStore{dailySpend: "0"}
+			client := &mockExchangeClient{
+				// PaymentDueRaw "", PaymentDueUSD nil, PaymentDueUSDStr "":
+				// the AWS response carried no PaymentDue at all.
+				quoteResult: &ExchangeQuoteSummary{
+					IsValidExchange: true,
+					CurrencyCode:    "USD",
+				},
+				executeResult: "exch-must-not-happen",
+			}
+			params := defaultParams(store, client)
+			params.Config.Mode = mode
+			params.Config.MaxPaymentPerExchangeUSD = 500.0
+
+			result, err := RunAutoExchange(context.Background(), params)
+			require.NoError(t, err)
+
+			assert.Zero(t, client.executeCalls, "Execute must not be called for a quote with no PaymentDue")
+			assert.Empty(t, store.savedRecords, "no record of any status may be written for an unpriced quote")
+			assert.Empty(t, result.Completed)
+			assert.Empty(t, result.Pending)
+			assert.Empty(t, result.Failed)
+			require.Len(t, result.Skipped, 1)
+			assert.Equal(t, "ri-001", result.Skipped[0].SourceRIID)
+			assert.Contains(t, result.Skipped[0].Reason, "no PaymentDue")
+		})
+	}
+}
+
+// TestProcessAutoExchange_FreshQuoteWithoutAmount_LedgerKeepsInitialQuote
+// (#1964): when the fresh Execute quote carries no amount, the completed
+// ledger row must keep the initial quoted amount, never "0", so the daily
+// spend total is not under-counted.
+func TestProcessAutoExchange_FreshQuoteWithoutAmount_LedgerKeepsInitialQuote(t *testing.T) {
+	t.Parallel()
+
+	preQuoteDue, _ := ParseDecimalRat("30.000000")
+	store := &mockExchangeStore{dailySpend: "0"}
+	client := &mockExchangeClient{
+		quoteResult: &ExchangeQuoteSummary{
+			IsValidExchange:  true,
+			PaymentDueRaw:    "30.000000",
+			PaymentDueUSD:    preQuoteDue,
+			PaymentDueUSDStr: "30.000000",
+			CurrencyCode:     "USD",
+		},
+		executeResult: "exch-no-fresh-amount",
+		// A client that skipped Execute's own re-quote check and returned
+		// a fresh quote without an amount.
+		executeQuoteResult: &ExchangeQuoteSummary{IsValidExchange: true, CurrencyCode: "USD"},
+	}
+	params := defaultParams(store, client)
+	params.Config.Mode = "auto"
+
+	rec := ReshapeRecommendation{
+		SourceRIID:         "ri-001",
+		SourceInstanceType: "m5.xlarge",
+		TargetInstanceType: "m5.large",
+		SourceCount:        1,
+		TargetCount:        2,
+		UtilizationPercent: 50.0,
+	}
+	perExchangeCap := new(big.Rat).SetFloat64(params.Config.MaxPaymentPerExchangeUSD)
+
+	outcome, halt := processAutoExchange(context.Background(), params, rec, "offering-123", "30.000000", perExchangeCap)
+
+	require.Empty(t, outcome.Error)
+	assert.False(t, halt)
+	require.Len(t, store.savedRecords, 1)
+	assert.Equal(t, "completed", store.savedRecords[0].Status)
+	assert.Equal(t, "30.000000", store.savedRecords[0].PaymentDue,
+		"ledger must keep the initial quoted amount when the fresh quote carries none, not \"0\"")
+}
+
 func TestRunAutoExchange_AutoMode_ExecutionFails(t *testing.T) {
 	t.Parallel()
 	store := &mockExchangeStore{dailySpend: "0"}

pkg/exchange/exchange.go

@@ -299,35 +299,46 @@ func ExecuteExchange(ctx context.Context, req ExchangeExecuteRequest) (exchangeI
 	return executeWithAPI(ctx, ec2.NewFromConfig(cfg), req)
 }
 
-// resolvePaymentDue returns the quote's PaymentDueUSD, treating nil as zero.
-// AWS may omit PaymentDue for zero-cost exchanges (e.g., same-RI-type conversions).
-func resolvePaymentDue(q *ExchangeQuoteSummary) *big.Rat {
-	if q.PaymentDueUSD != nil {
-		return q.PaymentDueUSD
-	}
-	return new(big.Rat)
+// requirePaymentDue returns the quote's PaymentDueUSD, refusing a quote that
+// carries none. AWS reports the true-up cost as a decimal that is zero or
+// more, so a zero-cost exchange parses to a non-nil zero; a nil means the
+// response had no amount at all, and a spend cap cannot be enforced against
+// an unknown amount.
+func requirePaymentDue(q *ExchangeQuoteSummary) (*big.Rat, error) {
+	if q.PaymentDueUSD == nil {
+		return nil, fmt.Errorf("quote reported no PaymentDue; refusing to enforce the spend cap against an unknown amount")
+	}
+	return q.PaymentDueUSD, nil
 }
 
-// checkInitialQuote returns an error if the quote is invalid or exceeds the spend cap.
+// checkInitialQuote returns an error if the quote is invalid, carries no
+// payment amount, or exceeds the spend cap.
 func checkInitialQuote(q *ExchangeQuoteSummary, maxPayment *big.Rat) error {
 	if !q.IsValidExchange {
 		return fmt.Errorf("exchange is not valid: %s", q.ValidationFailureReason)
 	}
-	paymentDue := resolvePaymentDue(q)
+	paymentDue, err := requirePaymentDue(q)
+	if err != nil {
+		return err
+	}
 	if paymentDue.Cmp(maxPayment) == 1 {
 		return fmt.Errorf("paymentDue %s exceeds max %s", paymentDue.FloatString(2), maxPayment.FloatString(2))
 	}
 	return nil
 }
 
-// checkReQuote returns an error if the pre-accept re-quote is invalid or exceeds the cap.
-// It is called immediately before AcceptReservedInstancesExchangeQuote to narrow the
-// race window between pricing changes.
+// checkReQuote returns an error if the pre-accept re-quote is invalid, carries
+// no payment amount, or exceeds the cap. It is called immediately before
+// AcceptReservedInstancesExchangeQuote to narrow the race window between
+// pricing changes.
 func checkReQuote(q *ExchangeQuoteSummary, maxPayment *big.Rat) error {
 	if !q.IsValidExchange {
 		return fmt.Errorf("exchange no longer valid at accept time: %s", q.ValidationFailureReason)
 	}
-	paymentDue := resolvePaymentDue(q)
+	paymentDue, err := requirePaymentDue(q)
+	if err != nil {
+		return fmt.Errorf("aborting exchange at accept time: %w", err)
+	}
 	if paymentDue.Cmp(maxPayment) == 1 {
 		return fmt.Errorf(
 			"aborting exchange: re-quoted payment %s USD exceeds cap %s USD (pricing changed between initial quote and accept)",

pkg/exchange/fail_loud_test.go

@@ -5,6 +5,7 @@ package exchange
 //   - M4: over-cap re-quote aborts before accept
 //   - M3: empty region returns an error (no us-east-1 default)
 //   - L2: Count <= 0 returns an error (no silent rewrite to 1)
+//   - #1964 / A09-004: a quote with no PaymentDue refuses before accept; an explicit zero proceeds
 
 import (
 	"context"
@@ -128,6 +129,122 @@ func TestExecute_ReQuoteWithinCapProceedsToAccept(t *testing.T) {
 	}
 }
 
+// seqQuoteOutNoPayment models a valid quote whose PaymentDue field is absent.
+func seqQuoteOutNoPayment() *ec2.GetReservedInstancesExchangeQuoteOutput {
+	return &ec2.GetReservedInstancesExchangeQuoteOutput{IsValidExchange: sdkaws.Bool(true)}
+}
+
+// TestExecute_MissingPaymentDueOnInitialQuote_RefusesBeforeAccept (#1964,
+// A09-004): a valid initial quote with no PaymentDue must be refused; Accept
+// must not be called and the re-quote must not even be attempted.
+func TestExecute_MissingPaymentDueOnInitialQuote_RefusesBeforeAccept(t *testing.T) {
+	t.Parallel()
+
+	f := &sequentialFakeEC2{
+		quoteOutputs: []*ec2.GetReservedInstancesExchangeQuoteOutput{seqQuoteOutNoPayment()},
+		quoteErrors:  []error{nil},
+		acceptOutput: &ec2.AcceptReservedInstancesExchangeQuoteOutput{ExchangeId: sdkaws.String("should-not-be-called")},
+	}
+	c := NewExchangeClientFromAPI(f)
+
+	_, _, err := c.Execute(context.Background(), ExchangeExecuteRequest{
+		ReservedIDs:      []string{"ri-1"},
+		TargetOfferingID: "off-A",
+		TargetCount:      1,
+		MaxPaymentDueUSD: new(big.Rat).SetInt64(50),
+	})
+
+	if err == nil {
+		t.Fatal("expected error when the initial quote carries no PaymentDue, got nil")
+	}
+	if !strings.Contains(err.Error(), "no PaymentDue") {
+		t.Errorf("error should name the missing PaymentDue; got: %v", err)
+	}
+	if f.acceptInput != nil {
+		t.Fatalf("Accept was called despite the quote carrying no PaymentDue; accept input: %+v", f.acceptInput)
+	}
+	if f.quoteCall != 1 {
+		t.Errorf("expected exactly one quote call before refusal, got %d", f.quoteCall)
+	}
+}
+
+// TestExecute_MissingPaymentDueOnReQuote_RefusesBeforeAccept (#1964,
+// A09-004): the initial quote is priced and within cap, but the pre-accept
+// re-quote carries no PaymentDue. The exchange must abort before Accept.
+func TestExecute_MissingPaymentDueOnReQuote_RefusesBeforeAccept(t *testing.T) {
+	t.Parallel()
+
+	f := &sequentialFakeEC2{
+		quoteOutputs: []*ec2.GetReservedInstancesExchangeQuoteOutput{
+			seqQuoteOut("40.00"),
+			seqQuoteOutNoPayment(),
+		},
+		quoteErrors:  []error{nil, nil},
+		acceptOutput: &ec2.AcceptReservedInstancesExchangeQuoteOutput{ExchangeId: sdkaws.String("should-not-be-called")},
+	}
+	c := NewExchangeClientFromAPI(f)
+
+	_, _, err := c.Execute(context.Background(), ExchangeExecuteRequest{
+		ReservedIDs:      []string{"ri-1"},
+		TargetOfferingID: "off-A",
+		TargetCount:      1,
+		MaxPaymentDueUSD: new(big.Rat).SetInt64(50),
+	})
+
+	if err == nil {
+		t.Fatal("expected error when the re-quote carries no PaymentDue, got nil")
+	}
+	if !strings.Contains(err.Error(), "no PaymentDue") {
+		t.Errorf("error should name the missing PaymentDue; got: %v", err)
+	}
+	if !strings.Contains(err.Error(), "accept time") {
+		t.Errorf("error should say the refusal happened at accept time; got: %v", err)
+	}
+	if f.acceptInput != nil {
+		t.Fatalf("Accept was called despite the re-quote carrying no PaymentDue; accept input: %+v", f.acceptInput)
+	}
+	if f.quoteCall != 2 {
+		t.Errorf("expected two quote calls (initial + re-quote) before refusal, got %d", f.quoteCall)
+	}
+}
+
+// TestExecute_ZeroPaymentDueProceedsToAccept is the positive control for
+// #1964: an explicit zero true-up cost is a real amount within any cap and
+// must not be confused with an absent PaymentDue.
+func TestExecute_ZeroPaymentDueProceedsToAccept(t *testing.T) {
+	t.Parallel()
+
+	f := &sequentialFakeEC2{
+		quoteOutputs: []*ec2.GetReservedInstancesExchangeQuoteOutput{
+			seqQuoteOut("0.000000"),
+			seqQuoteOut("0.000000"),
+		},
+		quoteErrors:  []error{nil, nil},
+		acceptOutput: &ec2.AcceptReservedInstancesExchangeQuoteOutput{ExchangeId: sdkaws.String("exch-zero")},
+	}
+	c := NewExchangeClientFromAPI(f)
+
+	exchangeID, freshQ, err := c.Execute(context.Background(), ExchangeExecuteRequest{
+		ReservedIDs:      []string{"ri-1"},
+		TargetOfferingID: "off-A",
+		TargetCount:      1,
+		MaxPaymentDueUSD: new(big.Rat).SetInt64(50),
+	})
+
+	if err != nil {
+		t.Fatalf("unexpected error for a zero-cost exchange: %v", err)
+	}
+	if exchangeID != "exch-zero" {
+		t.Fatalf("expected exchange ID 'exch-zero', got %q", exchangeID)
+	}
+	if f.acceptInput == nil {
+		t.Fatal("Accept was not called for a zero-cost exchange")
+	}
+	if freshQ == nil || freshQ.PaymentDueUSDStr != "0.000000" {
+		t.Fatalf("fresh quote must carry the explicit zero amount, got %+v", freshQ)
+	}
+}
+
 // TestLoadCfg_EmptyRegionErrors (M3):
 // loadCfg must return an error when region is empty; it must not default to us-east-1.
 func TestLoadCfg_EmptyRegionErrors(t *testing.T) {