fix(ladder): hold the buffer purchase when its layer has a pending reshape

CHANGELOG.md

@@ -223,6 +223,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 ### Fixed
 
 - **`gcp`:** the Cloud SQL, Memorystore and Cloud Storage price extractors keep 1-year and 3-year commitment SKUs in separate slots and return the price for the requested term, so a catalog that lists both terms no longer fails as ambiguous (#254). Two SKUs of the same term at different prices still fail closed, and a recommendation term other than 1yr or 3yr (for example "2yr") now errors instead of being priced as one year.
+- **`ladder`:** a run no longer emits a buffer-layer purchase for a layer it also recommends reshaping. The buffer allocation becomes an informational Hold naming the pending reshape; the reshape and all other allocations are unchanged. A layer that also carries the base or flex role (merged Azure reservation) is not held (#30).
+- **`ladder`:** a run no longer emits a buffer-layer purchase for a layer it also recommends reshaping. The buffer allocation becomes an informational Hold naming the pending reshape; the reshape stays. Other allocations are not edited, but with the held buffer removed a binding `MaxHourlyCommitPerRun` can give base/flex a larger share of the cap (total spend never higher), and a freed max-actions slot can let a deferred purchase through. A layer that also carries the base role (merged Azure reservation) is not held (#30).
 - **`azure`:** recommendations from the Consumption path (compute, database, cache, cosmosdb, managedredis, synapse) and the Savings Plan benefit path now set `CommitmentCurrency` to `USD`, because their commitment amounts come from sources already required to be USD. The Advisor and legacy paths carry no checked commitment amounts and stay empty (#623).
 - **AWS/OpenSearch:** the STS account-ID lookup used for post-purchase tagging caches only a successful result, so one throttled or timed-out `GetCallerIdentity` no longer disables tagging for the client's lifetime (#53).
 - **AWS/recommendations:** `normalizeRegionName` maps the current `Europe (Ireland|Frankfurt|London|Paris|Stockholm)` Cost Explorer display names to region codes, so coverage lookups keyed by region code match those recommendations (#53).

pkg/ladder/allocate.go

@@ -541,15 +541,17 @@ func buildResult(in *AllocationInput, lowWater, target, existingTotal, gap *big.
 	if err != nil {
 		return nil, err
 	}
+	allocs, reshapeHolds := holdReshapedBufferAllocations(allocs, reshapes, in.Layers, in.DataSources)
 	allocs, rateHolds := convertAllocations(allocs, rates, in.DataSources)
 	allocs, err = applyCapGuardrail(allocs, in.Config.MaxHourlyCommitPerRun)
 	if err != nil {
 		return nil, err
 	}
 	allocs, skipHolds := dropSubMinimumAllocations(allocs, in.DataSources)
 	allocs, reshapes, truncHolds := truncateToMaxActions(allocs, reshapes, in.Config.MaxActionsPerRun, in.LayerStates, in.DataSources)
-	holds := make([]PlannedAction, 0, len(maintHolds)+len(rateHolds)+len(skipHolds)+len(truncHolds))
+	holds := make([]PlannedAction, 0, len(maintHolds)+len(reshapeHolds)+len(rateHolds)+len(skipHolds)+len(truncHolds))
 	holds = append(holds, maintHolds...)
+	holds = append(holds, reshapeHolds...)
 	holds = append(holds, rateHolds...)
 	holds = append(holds, skipHolds...)
 	holds = append(holds, truncHolds...)
@@ -560,6 +562,43 @@ func buildResult(in *AllocationInput, lowWater, target, existingTotal, gap *big.
 	}, nil
 }
 
+// holdReshapedBufferAllocations turns the purchase for a buffer layer into an
+// informational Hold when the same run recommends reshaping that layer:
+// buying more of an under-utilized layer contradicts the reshape. Only
+// buffer-only layers are held; a layer that also carries the base role
+// (the merged Azure reservation) keeps its allocation because it is not purely
+// a buffer purchase. Must run before convertAllocations, while only
+// OnDemandEquivalentUSDPerHour is populated.
+func holdReshapedBufferAllocations(allocs []Allocation, reshapes []PlannedAction, layers []LayerSpec, dataSources []string) ([]Allocation, []PlannedAction) {
+	if len(reshapes) == 0 {
+		return allocs, nil
+	}
+	bufferOnly := make(map[LayerType]bool, len(layers))
+	for _, ls := range layers {
+		bufferOnly[ls.Type] = slices.Contains(ls.Roles, RoleBuffer) && !slices.Contains(ls.Roles, RoleBase)
+	}
+	reshaped := make(map[LayerType]bool, len(reshapes))
+	for _, r := range reshapes {
+		reshaped[r.Layer] = bufferOnly[r.Layer]
+	}
+	var kept []Allocation
+	var holds []PlannedAction
+	for _, a := range allocs {
+		if !reshaped[a.Layer] {
+			kept = append(kept, a)
+			continue
+		}
+		holds = append(holds, PlannedAction{
+			Action: ActionHold,
+			Layer:  a.Layer,
+			Rationale: fmt.Sprintf("buffer purchase of %s on layer %s withheld: the layer has a pending reshape recommendation, so buying more of an under-utilized layer is deferred",
+				fmtRatUSD(a.OnDemandEquivalentUSDPerHour), a.Layer),
+			DataSources: dataSources,
+		})
+	}
+	return kept, holds
+}
+
 // dropSubMinimumAllocations removes allocations whose amount fell below the
 // minimum allocatable threshold after splitting and cap scaling, emitting an
 // informational Hold for each so the skipped amount is never silently lost.

pkg/ladder/allocate_test.go

@@ -750,9 +750,10 @@ func TestAllocate_MaxActions_ReshapeRetained(t *testing.T) {
 		t.Errorf("surviving purchase must be the largest-gap layer EC2InstanceSP, got %s",
 			result.Allocations[0].Layer)
 	}
-	// Exactly the two smaller purchases were dropped with holds.
+	// ComputeSP was dropped by truncation; the ConvertibleRI buffer purchase
+	// never reached truncation because its layer has the pending reshape.
 	held := truncatedHoldLayers(result.Holds)
-	wantHeld := map[LayerType]bool{LayerComputeSP: true, LayerConvertibleRI: true}
+	wantHeld := map[LayerType]bool{LayerComputeSP: true}
 	if !maps.Equal(held, wantHeld) {
 		t.Errorf("dropped-purchase hold layers = %v, want %v", held, wantHeld)
 	}
@@ -765,8 +766,9 @@ func TestAllocate_MaxActions_ReshapeRetained(t *testing.T) {
 // its layer + amount in a max_actions_per_run hold.
 //
 // Fixture matches TestAllocate_MaxActions_ReshapeRetained but with cap=1: the
-// single reshape (ConvertibleRI) consumes the only slot, so all three purchases
-// (EC2InstanceSP $4/hr, ComputeSP $0.5/hr, ConvertibleRI $0.5/hr) are dropped.
+// single reshape (ConvertibleRI) consumes the only slot, so the remaining
+// purchases (EC2InstanceSP $4/hr, ComputeSP $0.5/hr) are dropped; the
+// ConvertibleRI buffer purchase is held earlier because of the reshape.
 func TestAllocate_MaxActions_ReshapesFillCap(t *testing.T) {
 	t.Parallel()
 	cfg := validConfigAWS()
@@ -805,7 +807,6 @@ func TestAllocate_MaxActions_ReshapesFillCap(t *testing.T) {
 	wantHeld := map[LayerType]bool{
 		LayerEC2InstanceSP: true,
 		LayerComputeSP:     true,
-		LayerConvertibleRI: true,
 	}
 	if !maps.Equal(held, wantHeld) {
 		t.Errorf("dropped-purchase hold layers = %v, want %v", held, wantHeld)
@@ -827,8 +828,9 @@ func TestAllocate_MaxActions_ReshapesFillCap(t *testing.T) {
 			t.Errorf("hold rationale must name the dropped amount: %q", h.Rationale)
 		}
 	}
-	if truncHolds != 3 {
-		t.Errorf("want 3 truncation holds (one per dropped purchase), got %d", truncHolds)
+	// The buffer purchase is held earlier (pending reshape), not truncated.
+	if truncHolds != 2 {
+		t.Errorf("want 2 truncation holds (one per dropped purchase), got %d", truncHolds)
 	}
 }
 
@@ -1917,3 +1919,88 @@ func TestAllocate_NilInFlightRejected(t *testing.T) {
 		t.Errorf("error %q does not name InFlight", err)
 	}
 }
+
+func reshapeHoldInput(bufferUtil float64) *AllocationInput {
+	layers := awsLayers()
+	states := withBufferUtilization(withExisting(zeroStates(layers), LayerConvertibleRI, 2.0), bufferUtil)
+	return &AllocationInput{
+		Config:      validConfigAWS(),
+		Layers:      layers,
+		Baseline:    UsageBaseline{LowWaterUSDPerHour: ptr(10.0), StableUSDPerHour: ptr(4.0)},
+		LayerStates: states,
+		Now:         nowFixed(),
+		InFlight:    noInFlight(), CommitmentRates: unitRates(),
+	}
+}
+
+func hasAllocOn(r *AllocateResult, l LayerType) bool {
+	for _, a := range r.Allocations {
+		if a.Layer == l {
+			return true
+		}
+	}
+	return false
+}
+
+func TestAllocate_BufferHeldWhenLayerHasReshape(t *testing.T) {
+	t.Parallel()
+	result, err := Allocate(reshapeHoldInput(70.0))
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if hasAllocOn(result, LayerConvertibleRI) {
+		t.Errorf("buffer purchase must be held, got %+v", result.Allocations)
+	}
+	if !hasAllocOn(result, LayerComputeSP) || !hasAllocOn(result, LayerEC2InstanceSP) {
+		t.Errorf("non-buffer allocations must be unchanged, got %+v", result.Allocations)
+	}
+	if len(result.Reshapes) != 1 || result.Reshapes[0].Layer != LayerConvertibleRI {
+		t.Fatalf("reshape must still appear, got %+v", result.Reshapes)
+	}
+	var held bool
+	for _, h := range result.Holds {
+		if h.Action == ActionHold && h.Layer == LayerConvertibleRI && strings.Contains(h.Rationale, "pending reshape") {
+			held = true
+		}
+	}
+	if !held {
+		t.Errorf("want a buffer Hold naming the pending reshape, got %+v", result.Holds)
+	}
+}
+
+func TestAllocate_BufferBoughtWhenNoReshape(t *testing.T) {
+	t.Parallel()
+	result, err := Allocate(reshapeHoldInput(95.0))
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(result.Reshapes) != 0 {
+		t.Fatalf("want no reshape, got %+v", result.Reshapes)
+	}
+	if !hasAllocOn(result, LayerConvertibleRI) {
+		t.Errorf("buffer must still be bought, got %+v", result.Allocations)
+	}
+}
+
+// A reshape on another layer must not suppress the buffer purchase. Valid
+// topologies allow one buffer layer, so this exercises the helper directly.
+func TestHoldReshapedBufferAllocations_OtherLayerUnaffected(t *testing.T) {
+	t.Parallel()
+	allocs := []Allocation{{Layer: LayerConvertibleRI, OnDemandEquivalentUSDPerHour: big.NewRat(1, 1)}}
+	reshapes := []PlannedAction{{Action: ActionReshape, Layer: LayerComputeSP}}
+	kept, holds := holdReshapedBufferAllocations(allocs, reshapes, awsLayers(), nil)
+	if len(kept) != 1 || len(holds) != 0 {
+		t.Errorf("want buffer kept and no holds, got kept=%+v holds=%+v", kept, holds)
+	}
+}
+
+// The merged Azure reservation also carries the base role; it is not held.
+func TestHoldReshapedBufferAllocations_MergedBaseBufferKept(t *testing.T) {
+	t.Parallel()
+	allocs := []Allocation{{Layer: LayerAzureReservation, OnDemandEquivalentUSDPerHour: big.NewRat(1, 1)}}
+	reshapes := []PlannedAction{{Action: ActionReshape, Layer: LayerAzureReservation}}
+	kept, holds := holdReshapedBufferAllocations(allocs, reshapes, azureLayers(), nil)
+	if len(kept) != 1 || len(holds) != 0 {
+		t.Errorf("want merged allocation kept, got kept=%+v holds=%+v", kept, holds)
+	}
+}