From 1c5c7c8af882ff1095dcb6f8f00f1128af87005d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 18:45:00 +0200 Subject: [PATCH] fix(aws/ladder): reject nil reshape payment caps instead of treating them as unlimited ReshapeBuffer always runs in auto mode, and a nil MaxPaymentPerExchangeUSD or MaxPaymentDailyUSD was mapped to math.MaxFloat64, so an omitted field meant unlimited spend. Both caps are now required: requireCap (replacing capOrUnlimited) returns a loud error for nil and the unlimitedCapUSD constant is removed. No production caller wires the write side yet, so nothing changes at runtime today. The stale "nil means no cap" doc in pkg/ladder/capability.go is left for a follow-up to avoid conflicting with the open #441, which edits that file. Part of #83 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 +++ providers/aws/ladder/reshape.go | 41 ++++++++++------------------ providers/aws/ladder/reshape_test.go | 36 ++++++++++++++++-------- 3 files changed, 43 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab16788b..a6fd1f6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -208,6 +208,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- **`ReshapeBuffer` rejects nil payment caps** instead of mapping them to + `math.MaxFloat64`: `MaxPaymentPerExchangeUSD` and `MaxPaymentDailyUSD` + are now required, so an omitted cap can no longer mean unlimited spend on + the auto-exchange path (#83). - **Azure savings plan recommendations use a 730 hour month** (was 730.5), matching `common.HoursPerMonth` and the 8760 hour year; monthly recurring cost, on-demand cost and savings shift by about 0.07% (#583). diff --git a/providers/aws/ladder/reshape.go b/providers/aws/ladder/reshape.go index d427a677..3e9e050b 100644 --- a/providers/aws/ladder/reshape.go +++ b/providers/aws/ladder/reshape.go @@ -10,19 +10,6 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/ladder" ) -// unlimitedCapUSD is the explicit "no cap" value passed to -// exchange.RIExchangeConfig when a BufferReshapeConfig cap is nil. -// -// Why not something cleaner: RIExchangeConfig has no nil/absent -// representation for its float64 caps, and 0 there is maximally RESTRICTIVE -// (RunAutoExchange skips any exchange whose payment exceeds the cap, so a -// zero cap blocks everything) — mapping nil to 0 would silently invert the -// caller's intent. +Inf is not usable either: big.Rat.SetFloat64(+Inf) -// returns nil and the comparison in RunAutoExchange would panic. -// math.MaxFloat64 is finite (exactly representable in big.Rat) and exceeds -// any real exchange payment, making it a faithful "no cap". -const unlimitedCapUSD = math.MaxFloat64 - // ReshapeBuffer runs the automated RI exchange flow over the buffer layer // (convertible RIs), delegating to the injected exchangeRunner. AWSLadder // only maps the configuration and the outcome; the runner owns the exchange @@ -49,11 +36,12 @@ const unlimitedCapUSD = math.MaxFloat64 // // Config mapping (BufferReshapeConfig -> exchange.RIExchangeConfig): // -// - MaxPaymentPerExchangeUSD / MaxPaymentDailyUSD: nil means no cap and maps -// to unlimitedCapUSD (see that constant for why); non-nil values must be -// finite and > 0 — zero is rejected loudly because RunAutoExchange treats -// the cap as a skip threshold and a zero cap would silently block every -// exchange (almost certainly a config bug, not an intent). +// - MaxPaymentPerExchangeUSD / MaxPaymentDailyUSD: both are required. A nil +// cap is rejected loudly (Mode is always auto, so an omitted cap must +// never mean unlimited spend), and values must be finite and > 0 — zero +// is rejected because RunAutoExchange treats the cap as a skip threshold +// and a zero cap would silently block every exchange (almost certainly a +// config bug, not an intent). // - UtilizationThresholdPct must be in (0, 100]; LookbackDays must be > 0. // - Mode is always exchange.ExchangeModeAuto: exchanges execute immediately, // subject to the per-exchange and daily caps. @@ -109,11 +97,11 @@ func (a *AWSLadder) ReshapeBuffer(ctx context.Context, scope ladder.Scope, cfg l // exchange package's runtime configuration. See ReshapeBuffer's godoc for the // full mapping rationale. func buildRIExchangeConfig(cfg ladder.BufferReshapeConfig) (exchange.RIExchangeConfig, error) { - perExchangeCap, err := capOrUnlimited("MaxPaymentPerExchangeUSD", cfg.MaxPaymentPerExchangeUSD) + perExchangeCap, err := requireCap("MaxPaymentPerExchangeUSD", cfg.MaxPaymentPerExchangeUSD) if err != nil { return exchange.RIExchangeConfig{}, err } - dailyCap, err := capOrUnlimited("MaxPaymentDailyUSD", cfg.MaxPaymentDailyUSD) + dailyCap, err := requireCap("MaxPaymentDailyUSD", cfg.MaxPaymentDailyUSD) if err != nil { return exchange.RIExchangeConfig{}, err } @@ -138,19 +126,20 @@ func buildRIExchangeConfig(cfg ladder.BufferReshapeConfig) (exchange.RIExchangeC }, nil } -// capOrUnlimited maps an optional money cap to the float64 the exchange -// config requires: nil -> unlimitedCapUSD (no cap); non-nil values must be -// finite and > 0 (see the unlimitedCapUSD comment for why zero is rejected). -func capOrUnlimited(name string, capUSD *float64) (float64, error) { +// requireCap validates a required money cap and returns it as the float64 the +// exchange config requires. nil is rejected: the caller must set an explicit +// cap, so an omitted field can never mean unlimited spend. Values must be +// finite and > 0 (a zero cap would block every exchange). +func requireCap(name string, capUSD *float64) (float64, error) { if capUSD == nil { - return unlimitedCapUSD, nil + return 0, fmt.Errorf("%s is required: set an explicit cap (a nil cap is not treated as unlimited)", name) } v := *capUSD if math.IsNaN(v) || math.IsInf(v, 0) { return 0, fmt.Errorf("%s must be finite, got %g", name, v) } if v <= 0 { - return 0, fmt.Errorf("%s must be > 0 when set (a zero cap would block every exchange; use nil for no cap), got %g", name, v) + return 0, fmt.Errorf("%s must be > 0 (a zero cap would block every exchange), got %g", name, v) } return v, nil } diff --git a/providers/aws/ladder/reshape_test.go b/providers/aws/ladder/reshape_test.go index db7a3886..7e9f5307 100644 --- a/providers/aws/ladder/reshape_test.go +++ b/providers/aws/ladder/reshape_test.go @@ -66,20 +66,32 @@ func TestReshapeBuffer_CapMapping_SetValuesPassedVerbatim(t *testing.T) { assert.Equal(t, 30, ex.gotCfg.LookbackDays) } -func TestReshapeBuffer_CapMapping_NilMeansUnlimited(t *testing.T) { +func TestReshapeBuffer_CapMapping_NilRejected(t *testing.T) { t.Parallel() - ex := &fakeExchangeRunner{} - a := newWiredLadder(t, &fakePurchaser{}, &fakePurchaser{}, ex) + tests := []struct { + name string + mutate func(*ladder.BufferReshapeConfig) + wantErr string + }{ + {"nil per-exchange cap", func(c *ladder.BufferReshapeConfig) { c.MaxPaymentPerExchangeUSD = nil }, + "MaxPaymentPerExchangeUSD is required"}, + {"nil daily cap", func(c *ladder.BufferReshapeConfig) { c.MaxPaymentDailyUSD = nil }, + "MaxPaymentDailyUSD is required"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ex := &fakeExchangeRunner{} + a := newWiredLadder(t, &fakePurchaser{}, &fakePurchaser{}, ex) - cfg := validReshapeCfg() - cfg.MaxPaymentPerExchangeUSD = nil - cfg.MaxPaymentDailyUSD = nil - _, err := a.ReshapeBuffer(context.Background(), testScope(), cfg) - require.NoError(t, err) - assert.Equal(t, unlimitedCapUSD, ex.gotCfg.MaxPaymentPerExchangeUSD, - "nil per-exchange cap must map to the explicit unlimited constant, never to 0 (0 blocks every exchange)") - assert.Equal(t, unlimitedCapUSD, ex.gotCfg.MaxPaymentDailyUSD, - "nil daily cap must map to the explicit unlimited constant, never to 0") + cfg := validReshapeCfg() + tt.mutate(&cfg) + _, err := a.ReshapeBuffer(context.Background(), testScope(), cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + assert.Equal(t, 0, ex.calls, "a missing cap must never reach the exchange runner") + }) + } } func TestReshapeBuffer_CapValidation_ZeroAndBadValuesRejected(t *testing.T) {