fix(plans): scope Savings Plans out of multi-step ramps

internal/api/handler_plans_sp_multistep_test.go

@@ -0,0 +1,95 @@
+package api
+
+import (
+	"context"
+	"testing"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/aws/aws-lambda-go/events"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// Savings Plans are scoped out of multi-step ramps (platform#631 Q7): create,
+// PUT and PATCH (arm) refuse an armed multi-step plan with an SP service with
+// a 400, and leave single-step SP and multi-step non-SP plans alone.
+
+const spPlanID = "11111111-1111-1111-1111-111111111111"
+
+func spHandler(t *testing.T) (*Handler, *MockConfigStore) {
+	t.Helper()
+	ctx := context.Background()
+	store := new(MockConfigStore)
+	auth := new(MockAuthService)
+	auth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil)
+	auth.grantAdmin()
+	return &Handler{config: store, auth: auth}, store
+}
+
+func spReq(body string) *events.LambdaFunctionURLRequest {
+	return &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}, Body: body}
+}
+
+func requireSP400(t *testing.T, err error) {
+	t.Helper()
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "want ClientError, got %T: %v", err, err)
+	assert.Equal(t, 400, ce.code)
+	assert.Contains(t, ce.Error(), "multi-step ramps do not support Savings Plans")
+}
+
+func TestCreatePlan_RejectsArmedMultiStepSavingsPlan(t *testing.T) {
+	h, _ := spHandler(t)
+	body := `{"name":"P","provider":"aws","service":"savingsplans","ramp_schedule":"weekly-25pct","enabled":true,"auto_purchase":true,"target_accounts":["bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"]}`
+	_, err := h.createPlan(context.Background(), spReq(body))
+	requireSP400(t, err)
+}
+
+func TestMergeUpdatedPlan_RejectsArmedMultiStepSavingsPlan(t *testing.T) {
+	existing := &config.PurchasePlan{Name: "p", RampSchedule: config.RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1}}
+	_, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", Provider: "aws", Service: "savingsplans",
+		RampSchedule: "weekly-25pct", Enabled: true, AutoPurchase: true}, "id")
+	requireSP400(t, err)
+}
+
+func patchSP(t *testing.T, existing *config.PurchasePlan, body string) (any, error) {
+	t.Helper()
+	ctx := context.Background()
+	h, store := spHandler(t)
+	store.On("GetPurchasePlan", ctx, spPlanID).Return(existing, nil)
+	store.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil)
+	return h.patchPlan(ctx, spReq(body), spPlanID)
+}
+
+func spPlan(service string, ramp config.RampSchedule) *config.PurchasePlan {
+	return &config.PurchasePlan{ID: spPlanID, Name: "p", NotificationDaysBefore: 3, RampSchedule: ramp,
+		Services: map[string]config.ServiceConfig{"aws/" + service: {Provider: "aws", Service: service, Enabled: true, Term: 3, Payment: "no-upfront", Coverage: 80}}}
+}
+
+func TestPatchPlan_ArmingMultiStepSavingsPlanIs400(t *testing.T) {
+	weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4}
+	_, err := patchSP(t, spPlan("savingsplans", weekly), `{"enabled":true,"auto_purchase":true}`)
+	requireSP400(t, err)
+}
+
+func TestPatchPlan_SingleStepSavingsPlanAndMultiStepNonSPArmFine(t *testing.T) {
+	immediate := config.RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1}
+	weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4}
+	for name, plan := range map[string]*config.PurchasePlan{
+		"single-step SP":          spPlan("savingsplans", immediate),
+		"multi-step RDS":          spPlan("rds", weekly),
+		"single custom SP (100%)": spPlan("savingsplans", config.RampSchedule{Type: "custom", PercentPerStep: 100, StepIntervalDays: 7, TotalSteps: 1}),
+	} {
+		t.Run(name, func(t *testing.T) {
+			_, err := patchSP(t, plan, `{"enabled":true,"auto_purchase":true}`)
+			require.NoError(t, err)
+		})
+	}
+}
+
+func TestPatchPlan_UnarmedMultiStepSavingsPlanIsAllowed(t *testing.T) {
+	weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4}
+	_, err := patchSP(t, spPlan("savingsplans", weekly), `{"name":"renamed"}`)
+	require.NoError(t, err)
+}

internal/config/validation.go

@@ -3,13 +3,15 @@ package config
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"math"
 	"net/mail"
 	"sort"
 	"strconv"
 	"strings"
 
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
 	"github.com/LeanerCloud/cloud-commitments-go/pkg/ladder"
 )
 
@@ -597,6 +599,29 @@ func (p *PurchasePlan) Validate() error {
 		}
 	}
 
+	return p.CheckNoMultiStepSavingsPlans()
+}
+
+// ErrMultiStepSavingsPlans marks an armed multi-step plan with an enabled
+// Savings Plans service. Step 1 suppresses the whole SP recommendation for 7
+// days, so later steps find nothing and the ramp strands (platform#631, Q7).
+// Read-only query for already-armed hits: runbooks/armed-multi-step-savings-plans-plans.md.
+var ErrMultiStepSavingsPlans = errors.New("multi-step ramps do not support Savings Plans: use a single-step (immediate) ramp or remove the Savings Plans service")
+
+// CheckNoMultiStepSavingsPlans refuses an armed (enabled and auto-purchase)
+// plan that has more than one ramp step and an enabled Savings Plans service.
+// It is the one definition, shared by plan validation (create, update, arm) and
+// the step precheck at attach. Single-step plans are unaffected.
+func (p *PurchasePlan) CheckNoMultiStepSavingsPlans() error {
+	if !p.Enabled || !p.AutoPurchase || p.RampSchedule.TotalSteps <= 1 {
+		return nil
+	}
+	for key := range p.Services {
+		svc := p.Services[key]
+		if svc.Enabled && common.IsSavingsPlan(common.ServiceType(svc.Service)) {
+			return fmt.Errorf("%w (service %q, total_steps %d)", ErrMultiStepSavingsPlans, key, p.RampSchedule.TotalSteps)
+		}
+	}
 	return nil
 }
 

internal/config/validation_test.go

@@ -1109,3 +1109,39 @@ func TestLadderCapMicros(t *testing.T) {
 		assert.Error(t, err, in)
 	}
 }
+
+func TestPurchasePlan_CheckNoMultiStepSavingsPlans(t *testing.T) {
+	svc := func(name string, enabled bool) map[string]ServiceConfig {
+		return map[string]ServiceConfig{"aws/" + name: {Provider: "aws", Service: name, Enabled: enabled}}
+	}
+	cases := []struct {
+		name    string
+		enabled bool
+		auto    bool
+		steps   int
+		svcs    map[string]ServiceConfig
+		wantErr bool
+	}{
+		{"armed 4-step SP", true, true, 4, svc("savingsplans", true), true},
+		{"armed 2-step SP (boundary)", true, true, 2, svc("savingsplans", true), true},
+		{"armed 1-step SP", true, true, 1, svc("savingsplans", true), false},
+		{"armed 0-step SP", true, true, 0, svc("savingsplans", true), false},
+		{"armed 4-step compute SP", true, true, 4, svc("savings-plans-compute", true), true},
+		{"armed 4-step disabled SP service", true, true, 4, svc("savingsplans", false), false},
+		{"armed 4-step non-SP", true, true, 4, svc("rds", true), false},
+		{"not auto-purchase", true, false, 4, svc("savingsplans", true), false},
+		{"disabled", false, true, 4, svc("savingsplans", true), false},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			p := &PurchasePlan{Enabled: tc.enabled, AutoPurchase: tc.auto, Services: tc.svcs,
+				RampSchedule: RampSchedule{TotalSteps: tc.steps}}
+			err := p.CheckNoMultiStepSavingsPlans()
+			if tc.wantErr {
+				require.ErrorIs(t, err, ErrMultiStepSavingsPlans)
+				return
+			}
+			require.NoError(t, err)
+		})
+	}
+}

internal/planstep/precheck.go

@@ -17,7 +17,8 @@ var ErrUnsupportedPlan = errors.New("plan settings are not supported by recommen
 // Precheck refuses, with a plan-specific error, the plan settings Resolve does
 // not apply: any Include*/Exclude*/MinCount filter on a service config (the
 // plan-step request never sets them, so honoring them would buy something other
-// than what the plan says), and an immediate ramp whose PercentPerStep is not
+// than what the plan says), an armed multi-step plan with a Savings Plans
+// service (platform#631 Q7), and an immediate ramp whose PercentPerStep is not
 // 100 (an immediate ramp ignores that field when validated but Resolve sizes
 // with it).
 func Precheck(plan *config.PurchasePlan) error {
@@ -36,6 +37,9 @@ func Precheck(plan *config.PurchasePlan) error {
 		return fmt.Errorf("%w: plan %s has an immediate ramp with percent_per_step %v (must be 100)",
 			ErrUnsupportedPlan, plan.ID, plan.RampSchedule.PercentPerStep)
 	}
+	if err := plan.CheckNoMultiStepSavingsPlans(); err != nil {
+		return fmt.Errorf("%w: plan %s: %w", ErrUnsupportedPlan, plan.ID, err)
+	}
 	return nil
 }
 

internal/planstep/precheck_test.go

@@ -17,6 +17,9 @@ func planWith(svc config.ServiceConfig, ramp config.RampSchedule) *config.Purcha
 func TestPrecheck(t *testing.T) {
 	ok := config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: true}
 	weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25}
+	sp := config.ServiceConfig{Provider: "aws", Service: "savingsplans", Enabled: true}
+	weekly4 := config.RampSchedule{Type: "weekly", PercentPerStep: 25, TotalSteps: 4}
+	armed := func(p *config.PurchasePlan) *config.PurchasePlan { p.Enabled, p.AutoPurchase = true, true; return p }
 	cases := []struct {
 		name string
 		plan *config.PurchasePlan
@@ -35,6 +38,9 @@ func TestPrecheck(t *testing.T) {
 		{"exclude types", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.ExcludeTypes = []string{"m5.large"} }), weekly), "exclude_types"},
 		{"min count", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.MinCount = 1 }), weekly), "min_count"},
 		{"negative min count", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.MinCount = -1 }), weekly), "min_count"},
+		{"armed multi-step savings plan", armed(planWith(sp, weekly4)), "multi-step ramps do not support Savings Plans"},
+		{"armed single-step savings plan", armed(planWith(sp, config.RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1})), ""},
+		{"armed multi-step non-SP", armed(planWith(ok, weekly4)), ""},
 		{"filter on a disabled service still refuses", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.Enabled = false; s.MinCount = 2 }), weekly), "min_count"},
 	}
 	for _, tc := range cases {

internal/purchase/plan_step_attach.go

@@ -153,6 +153,9 @@ func (m *Manager) attachStep(ctx context.Context, tx pgx.Tx, locked *config.Purc
 	if errors.Is(err, planstep.ErrNoMatch) {
 		return m.noMatch(ctx, tx, locked, claim, root)
 	}
+	if errors.Is(err, planstep.ErrUnsupportedPlan) {
+		return m.failStep(ctx, tx, locked, claim, root, err.Error())
+	}
 	if err != nil {
 		return stepClaim{}, fmt.Errorf("plan %s step %d: %w", locked.ID, claim.number, err)
 	}

internal/purchase/plan_step_attach_test.go

@@ -237,3 +237,25 @@ func TestPlanUnchanged_ComparesDatesAtMicrosecondPrecision(t *testing.T) {
 	locked.NextExecutionDate = &moved
 	assert.False(t, planUnchanged(listed, locked, providers), "a microsecond is a change")
 }
+
+// An already-armed multi-step Savings Plans plan (platform#631 Q7) must produce
+// one visible failed row with the reason, not an error on every tick.
+func TestAttach_UnsupportedPlanFailsTheStepVisibly(t *testing.T) {
+	listed := attachPlan(time.Now().Add(48*time.Hour), 1e6)
+	locked := attachPlan(*listed.NextExecutionDate, 1e6)
+	for _, p := range []*config.PurchasePlan{listed, locked} {
+		p.Services = map[string]config.ServiceConfig{"aws/savingsplans": {Enabled: true, Provider: "aws", Service: "savingsplans"}}
+		p.RampSchedule = config.RampSchedule{Type: "weekly", PercentPerStep: 25, TotalSteps: 4}
+	}
+	res := &fakeResolver{err: planstep.Precheck(locked)}
+	require.ErrorIs(t, res.err, planstep.ErrUnsupportedPlan)
+	m, store := attachManager(t, listed, locked, res)
+	store.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil)
+
+	_, _, _, err := m.getOrCreateExecution(context.Background(), listed)
+	require.ErrorIs(t, err, errExecutionNotNotifiable)
+	saved := savedRows(store)
+	require.Len(t, saved, 1)
+	assert.Equal(t, "failed", saved[0].Status)
+	assert.Contains(t, saved[0].Error, "multi-step ramps do not support Savings Plans")
+}

runbooks/armed-multi-step-savings-plans-plans.md

@@ -0,0 +1,29 @@
+# Runbook: armed multi-step plans that include Savings Plans
+
+Multi-step ramps do not support Savings Plans (platform#631). Step 1 suppresses
+the whole SP recommendation for 7 days, so later steps find nothing and the ramp
+strands. Plan create, update and arm now return HTTP 400 for an armed plan
+(enabled and auto_purchase) with `total_steps > 1` and an enabled Savings Plans
+service, and an already-armed one fails its next step with a visible failed row.
+
+## Before deploying this change (read-only, run on DEV and prod)
+
+Lists the armed plans that would start failing. Decide each hit first: re-create
+it as a single-step (immediate) plan, remove the SP service, or disarm it.
+
+```sql
+SELECT p.id, p.name, p.ramp_schedule->>'type' AS ramp_type,
+       (p.ramp_schedule->>'total_steps')::int AS total_steps,
+       (p.ramp_schedule->>'current_step')::int AS current_step,
+       p.next_execution_date,
+       array_agg(s.key) AS sp_services
+FROM purchase_plans p
+CROSS JOIN LATERAL jsonb_each(p.services) AS s(key, val)
+WHERE p.enabled AND p.auto_purchase
+  AND COALESCE((p.ramp_schedule->>'total_steps')::int, 0) > 1
+  AND COALESCE((s.val->>'enabled')::boolean, false)
+  AND (s.val->>'service' = 'savingsplans' OR s.val->>'service' LIKE 'savings-plans-%')
+GROUP BY p.id, p.name, p.ramp_schedule, p.next_execution_date;
+```
+
+Zero rows means nothing changes for existing plans.