From ca18029fef825af406f97cd5249db26532d55261 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 11 Jun 2026 00:58:08 -0700 Subject: [PATCH] fix(exchange): fail closed on unparseable PaymentDue in daily-cap check The auto-exchange daily-cap check warned and counted $0 toward the MaxPaymentDailyUSD guardrail when paymentDueStr failed to parse, in contrast to the dailySpent parse failure five lines above which aborts with a failed record. A silent $0 coercion on a money path undercounts the daily spend cap if any future caller produces a non-decimal value. Make the parse failure abort the exchange and persist a failed record, mirroring the dailySpent branch. The documented nil-means-zero-cost quote case stays separate: processRecommendation still maps a nil PaymentDueUSD to the explicit "0" string before the parse. Regression test exercises processAutoExchange with unparseable and empty PaymentDue values; confirmed failing pre-fix (exchange executed with $0 counted) and passing post-fix (aborted, failed record saved, Execute never called). Closes #1166 --- pkg/exchange/auto.go | 16 +++++++++++---- pkg/exchange/auto_test.go | 43 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/pkg/exchange/auto.go b/pkg/exchange/auto.go index b53be2f1f..255d13dc9 100644 --- a/pkg/exchange/auto.go +++ b/pkg/exchange/auto.go @@ -230,6 +230,9 @@ func processRecommendation(ctx context.Context, params RunAutoExchangeParams, re return } + // 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) @@ -444,12 +447,17 @@ func processAutoExchange(ctx context.Context, params RunAutoExchangeParams, rec return outcome } + // 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. paymentDue, err := ParseDecimalRat(paymentDueStr) if err != nil { - logging.Warnf("failed to parse payment due %q: %v, using zero", paymentDueStr, err) - } - if paymentDue == nil { - paymentDue = new(big.Rat) + logging.Errorf("failed to parse payment due %q for %s: %v", paymentDueStr, rec.SourceRIID, err) + outcome.Error = fmt.Sprintf("failed to parse payment due %q: %v", paymentDueStr, err) + saveFailedRecord(ctx, params, rec, offeringID, paymentDueStr, outcome.Error, ExchangeModeAuto) + return outcome } newTotal := new(big.Rat).Add(dailySpent, paymentDue) diff --git a/pkg/exchange/auto_test.go b/pkg/exchange/auto_test.go index babaaea6e..6c640eed9 100644 --- a/pkg/exchange/auto_test.go +++ b/pkg/exchange/auto_test.go @@ -3,6 +3,7 @@ package exchange import ( "context" "fmt" + "math/big" "testing" "time" @@ -112,6 +113,7 @@ type mockExchangeClient struct { quoteErr error executeResult string executeErr error + executeCalls int } func (m *mockExchangeClient) GetQuote(_ context.Context, _ ExchangeQuoteRequest) (*ExchangeQuoteSummary, error) { @@ -119,6 +121,7 @@ func (m *mockExchangeClient) GetQuote(_ context.Context, _ ExchangeQuoteRequest) } func (m *mockExchangeClient) Execute(_ context.Context, _ ExchangeExecuteRequest) (string, *ExchangeQuoteSummary, error) { + m.executeCalls++ return m.executeResult, m.quoteResult, m.executeErr } @@ -315,6 +318,46 @@ func TestRunAutoExchange_AutoMode_DailySpendDBError_FailsClosed(t *testing.T) { assert.Contains(t, result.Failed[0].Error, "daily cap check failed") } +// TestProcessAutoExchange_UnparseablePaymentDue_FailsClosed is the regression +// test for COR-04: an unparseable or empty paymentDueStr must abort the +// exchange and persist a failed record, never count as $0 toward the +// MaxPaymentDailyUSD cap. RunAutoExchange always builds a parseable string +// today, so processAutoExchange is exercised directly with the bad inputs. +func TestProcessAutoExchange_UnparseablePaymentDue_FailsClosed(t *testing.T) { + t.Parallel() + for _, paymentDueStr := range []string{"not-a-number", ""} { + t.Run(fmt.Sprintf("paymentDue=%q", paymentDueStr), func(t *testing.T) { + t.Parallel() + store := &mockExchangeStore{dailySpend: "0"} + client := &mockExchangeClient{executeResult: "exch-should-not-happen"} + 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 := processAutoExchange(context.Background(), params, rec, "offering-123", paymentDueStr, perExchangeCap) + + assert.Contains(t, outcome.Error, "failed to parse payment due") + assert.Empty(t, outcome.ExchangeID) + assert.Zero(t, client.executeCalls, "exchange must not execute on unparseable payment due") + + // A failed record must be persisted for the audit trail, + // mirroring the dailySpent parse-failure branch. + require.Len(t, store.savedRecords, 1) + assert.Equal(t, "failed", store.savedRecords[0].Status) + assert.Contains(t, store.savedRecords[0].Error, "failed to parse payment due") + }) + } +} + func TestRunAutoExchange_AutoMode_ExecutionFails(t *testing.T) { t.Parallel() store := &mockExchangeStore{dailySpend: "0"}