From 2256488931f8894b3330622e3b163f57a41821e2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 8 Sep 2026 08:38:33 +0200 Subject: [PATCH 1/2] fix(exchange): refuse a quote with no PaymentDue instead of treating it as $0 An ExchangeQuoteSummary whose PaymentDueUSD is nil means the AWS response carried no PaymentDue at all; a zero-cost exchange arrives as an explicit "0.000000" and parses to a non-nil zero. Every cap layer collapsed the two: getValidatedQuote skipped the per-exchange cap, processRecommendation recorded the exchange as costing "0" (so the daily cap added nothing and a manual pending record carried "0"), and resolvePaymentDue substituted a zero inside Execute so both the initial and the pre-accept re-quote checks passed. An unpriced exchange could reach AcceptReservedInstancesExchangeQuote with no effective ceiling. Fail closed at every consumer: - getValidatedQuote skips the recommendation with "quote reported no PaymentDue" before the cap compare; processRecommendation no longer defaults the amount to "0". - requirePaymentDue replaces resolvePaymentDue; checkInitialQuote and checkReQuote return its error before Accept is called. - acceptedAmountFromQuote falls back to the initial quoted amount, never "0", when a fresh quote carries no amount. Regression tests drive RunAutoExchange (auto and manual) and the real executeWithAPI with an unpriced quote and assert nothing executes and nothing is recorded; an explicit zero still proceeds. Closes #1964 Refs #1448 (A09-004) Co-Authored-By: claude-flow Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC --- pkg/exchange/auto.go | 48 +++++++------- pkg/exchange/auto_test.go | 84 +++++++++++++++++++++++ pkg/exchange/exchange.go | 37 +++++++---- pkg/exchange/fail_loud_test.go | 117 +++++++++++++++++++++++++++++++++ 4 files changed, 248 insertions(+), 38 deletions(-) diff --git a/pkg/exchange/auto.go b/pkg/exchange/auto.go index 6698b66e4..c85c2a778 100644 --- a/pkg/exchange/auto.go +++ b/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) diff --git a/pkg/exchange/auto_test.go b/pkg/exchange/auto_test.go index f76eb74b9..788005133 100644 --- a/pkg/exchange/auto_test.go +++ b/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"} diff --git a/pkg/exchange/exchange.go b/pkg/exchange/exchange.go index e8208497b..60d8a0163 100644 --- a/pkg/exchange/exchange.go +++ b/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)", diff --git a/pkg/exchange/fail_loud_test.go b/pkg/exchange/fail_loud_test.go index c4d54fa48..c2591babe 100644 --- a/pkg/exchange/fail_loud_test.go +++ b/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) { From 72bc37492f6c322a3365dcb8c5a04fb94449cc7d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 8 Sep 2026 08:51:52 +0200 Subject: [PATCH 2/2] refactor(api): drop the unreachable zero-substitution in handlerAcceptedAmount The helper returned "0" when a fresh Execute quote carried no payment amount, and its comment explained that as a zero-cost exchange where AWS returned nil. This commit's own change disproves that premise: a genuine zero arrives as an explicit "0.000000" and parses to a non-nil zero, while an absent PaymentDue is now refused by checkInitialQuote and checkReQuote before Accept runs. Execute can therefore no longer return successfully with an empty amount, so the branch is unreachable and its comment asserts something untrue. A future reader would take it as evidence that an empty amount is normal. Now mirrors exchange.acceptedAmountFromQuote and returns the caller's fallback instead of fabricating a figure. Found by adversarial review of the parent commit. Co-Authored-By: claude-flow Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC --- internal/api/handler_ri_exchange.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 5bc838799..d0f2d3c1a 100644 --- a/internal/api/handler_ri_exchange.go +++ b/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