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 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) {