fix(api): refuse a ramp shape change on a plan with completed steps

internal/api/handler_plans.go

@@ -306,6 +306,22 @@ func (h *Handler) updatePlan(ctx context.Context, httpReq *events.LambdaFunction
 			"updatePlan: GetPurchasePlan failed")
 	}
 
+	plan, err := mergeUpdatedPlan(existingPlan, &req, planID)
+	if err != nil {
+		return nil, err
+	}
+
+	if err := h.config.UpdatePurchasePlan(ctx, plan); err != nil {
+		return nil, mapCreatePlanStorageError(err, "plan not found", "failed to update plan", "updatePlan: UpdatePurchasePlan failed")
+	}
+
+	return plan, nil
+}
+
+// mergeUpdatedPlan builds the plan an update request describes, carrying over
+// the state the request cannot express (timestamps, ramp progress, services)
+// from the stored plan, and validates it.
+func mergeUpdatedPlan(existingPlan *config.PurchasePlan, req *PlanRequest, planID string) (*config.PurchasePlan, error) {
 	// Create new plan from request
 	plan := req.toPurchasePlan()
 	plan.ID = planID
@@ -319,7 +335,9 @@ func (h *Handler) updatePlan(ctx context.Context, httpReq *events.LambdaFunction
 	// since it has no way to see the existing plan. Left alone, every PUT
 	// -- including a bare rename -- silently restarts an in-progress ramp,
 	// so the scheduler re-buys steps that already executed (issue #219).
-	preserveRampProgress(existingPlan, plan)
+	if err := preserveRampProgress(existingPlan, plan); err != nil {
+		return nil, err
+	}
 
 	// If no services were created from request, preserve existing services
 	if len(plan.Services) == 0 && len(existingPlan.Services) > 0 {
@@ -330,33 +348,38 @@ func (h *Handler) updatePlan(ctx context.Context, httpReq *events.LambdaFunction
 	if err := plan.Validate(); err != nil {
 		return nil, NewClientError(400, fmt.Sprintf("validation error: %s", err))
 	}
-
-	if err := h.config.UpdatePurchasePlan(ctx, plan); err != nil {
-		return nil, mapCreatePlanStorageError(err, "plan not found", "failed to update plan", "updatePlan: UpdatePurchasePlan failed")
-	}
-
 	return plan, nil
 }
 
 // preserveRampProgress carries ramp-in-progress state from the existing
 // plan onto a freshly rebuilt one (issue #219). LastExecutionDate and
 // LastNotificationSent record real purchase history, so they are always
 // carried over; a rebuild has no way to know either and would otherwise
-// wipe them. CurrentStep, StartDate, and NextExecutionDate are only
-// carried over when the requested ramp schedule has the same shape as
-// the existing one (same type/percent/interval/step-count) -- an
-// unrelated field edit (rename, target coverage, ...) resubmits the same
-// schedule and must not restart it, while a deliberate change to a
-// different schedule starts that schedule's progress at step zero.
-func preserveRampProgress(existingPlan, plan *config.PurchasePlan) {
+// wipe them. CurrentStep, StartDate, and NextExecutionDate are carried
+// over when the requested ramp schedule has the same shape as the existing
+// one (same type/percent/interval/step-count): an unrelated field edit
+// (rename, target coverage, ...) resubmits the same schedule and must not
+// restart it. A different shape is refused once a step has completed,
+// because the rebuilt schedule would restart at step zero and forget the
+// percentage already bought (issue #386); before the first step it is a
+// plain schedule change.
+func preserveRampProgress(existingPlan, plan *config.PurchasePlan) error {
 	plan.LastExecutionDate = existingPlan.LastExecutionDate
 	plan.LastNotificationSent = existingPlan.LastNotificationSent
 
 	if rampScheduleShapeUnchanged(existingPlan.RampSchedule, plan.RampSchedule) {
 		plan.RampSchedule.CurrentStep = existingPlan.RampSchedule.CurrentStep
 		plan.RampSchedule.StartDate = existingPlan.RampSchedule.StartDate
 		plan.NextExecutionDate = existingPlan.NextExecutionDate
+		return nil
 	}
+	if existingPlan.RampSchedule.CurrentStep > 0 {
+		return NewClientError(http.StatusConflict, fmt.Sprintf(
+			"cannot change the ramp schedule of a plan with %d completed step(s); "+
+				"resend the current ramp_schedule to keep it, or create a new plan",
+			existingPlan.RampSchedule.CurrentStep))
+	}
+	return nil
 }
 
 // rampScheduleShapeUnchanged compares the schedule's shape, deliberately

internal/api/handler_plans_ramp_shape_test.go

@@ -0,0 +1,87 @@
+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"
+)
+
+// putPlan drives PUT /api/plans/{id} through the router against an existing
+// plan and returns the response plus the plan handed to UpdatePurchasePlan
+// (nil when the update was refused).
+func putPlan(t *testing.T, existing *config.PurchasePlan, body string) (*events.LambdaFunctionURLResponse, *config.PurchasePlan) {
+	t.Helper()
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", ctx, "test-token").Return(&Session{UserID: "admin-id", Email: "admin@example.com"}, nil)
+	mockAuth.grantAdmin()
+	mockAuth.On("ValidateCSRFToken", ctx, mock.Anything, mock.Anything).Return(nil)
+
+	var saved *config.PurchasePlan
+	mockStore.On("GetPurchasePlan", mock.Anything, existing.ID).Return(existing, nil)
+	mockStore.On("UpdatePurchasePlan", mock.Anything, mock.AnythingOfType("*config.PurchasePlan")).
+		Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchasePlan) }).Return(nil)
+
+	handler := &Handler{config: mockStore, auth: mockAuth, apiKey: "test-key"}
+	resp, err := handler.HandleRequest(ctx, &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{
+			"X-API-Key": "test-key", "Authorization": "Bearer test-token",
+			"X-CSRF-Token": "test-csrf", "Content-Type": "application/json",
+		},
+		Body: body,
+		RequestContext: events.LambdaFunctionURLRequestContext{
+			HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "PUT", Path: "/api/plans/" + existing.ID},
+		},
+	})
+	require.NoError(t, err)
+	return resp, saved
+}
+
+func TestUpdatePlan_RampShapeChange(t *testing.T) {
+	const planID = "12345678-1234-1234-1234-123456789abc"
+	weekly := config.PresetRampSchedules[config.RampWeekly25Pct]
+	custom30 := config.RampSchedule{Type: "custom", PercentPerStep: 30, StepIntervalDays: 7, TotalSteps: 3}
+
+	tests := []struct {
+		name       string
+		stored     config.RampSchedule
+		step       int
+		body       string
+		wantStatus int
+		wantStep   int
+		wantType   string
+	}{
+		{"in-progress ramp, different shape is refused", weekly, 3, `{"name":"p","ramp_schedule":"monthly-10pct"}`, 409, 0, ""},
+		{"in-progress ramp, omitted ramp_schedule is refused", weekly, 3, `{"name":"renamed"}`, 409, 0, ""},
+		{"first completed step, different shape is refused", weekly, 1, `{"name":"p","ramp_schedule":"monthly-10pct"}`, 409, 0, ""},
+		// 30% and 33% both give 3 steps: only PercentPerStep differs.
+		{"in-progress ramp, only the percent differs, is refused", custom30, 2, `{"name":"p","ramp_schedule":"custom","custom_step_percent":33,"custom_interval_days":7}`, 409, 0, ""},
+		{"in-progress ramp, same shape keeps its step", weekly, 3, `{"name":"renamed","ramp_schedule":"weekly-25pct"}`, 200, 3, "weekly"},
+		{"no completed step, different shape is allowed", weekly, 0, `{"name":"p","ramp_schedule":"monthly-10pct"}`, 200, 0, "monthly"},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			rs := tc.stored
+			rs.CurrentStep = tc.step
+			existing := &config.PurchasePlan{ID: planID, Name: "p", Enabled: true, RampSchedule: rs}
+
+			resp, saved := putPlan(t, existing, tc.body)
+
+			assert.Equal(t, tc.wantStatus, resp.StatusCode)
+			if tc.wantStatus == 409 {
+				assert.Nil(t, saved, "a refused shape change must not reach the store")
+				assert.Contains(t, resp.Body, "create a new plan")
+				return
+			}
+			require.NotNil(t, saved)
+			assert.Equal(t, tc.wantStep, saved.RampSchedule.CurrentStep)
+			assert.Equal(t, tc.wantType, saved.RampSchedule.Type)
+		})
+	}
+}

internal/api/handler_plans_test.go

@@ -664,61 +664,6 @@ func TestHandler_updatePlan_PreservesCompletedRampSteps(t *testing.T) {
 	assert.True(t, plan.LastNotificationSent.Equal(lastNotif))
 }
 
-// TestHandler_updatePlan_ChangingRampScheduleStartsAtStepZero documents
-// the complementary case: when the update genuinely picks a different
-// ramp schedule, its progress legitimately starts fresh rather than
-// carrying over a step count that has no meaning under the new schedule.
-func TestHandler_updatePlan_ChangingRampScheduleStartsAtStepZero(t *testing.T) {
-	ctx := context.Background()
-	mockStore := new(MockConfigStore)
-	mockAuth := new(MockAuthService)
-
-	adminSession := &Session{
-		UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa",
-		Email:  "admin@example.com",
-	}
-
-	rampStart := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)
-	existingPlan := &config.PurchasePlan{
-		ID:      "12345678-1234-1234-1234-123456789abc",
-		Name:    "Weekly ramp plan",
-		Enabled: true,
-		Services: map[string]config.ServiceConfig{
-			"aws/ec2": {Provider: "aws", Service: "ec2", Enabled: true, Term: 1, Payment: "no-upfront", Coverage: 80},
-		},
-		RampSchedule: config.RampSchedule{
-			Type:             "weekly",
-			PercentPerStep:   25,
-			StepIntervalDays: 7,
-			CurrentStep:      3,
-			TotalSteps:       4,
-			StartDate:        rampStart,
-		},
-	}
-
-	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
-	mockAuth.grantAdmin()
-	mockStore.On("GetPurchasePlan", ctx, "12345678-1234-1234-1234-123456789abc").Return(existingPlan, nil)
-	mockStore.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil)
-
-	handler := &Handler{config: mockStore, auth: mockAuth}
-
-	body := `{"name": "Weekly ramp plan", "enabled": true, "ramp_schedule": "monthly-10pct"}`
-	req := &events.LambdaFunctionURLRequest{
-		Headers: map[string]string{
-			"Authorization": "Bearer admin-token",
-		},
-		Body: body,
-	}
-	result, err := handler.updatePlan(ctx, req, "12345678-1234-1234-1234-123456789abc")
-	require.NoError(t, err)
-
-	plan := result.(*config.PurchasePlan)
-	assert.Equal(t, "monthly", plan.RampSchedule.Type)
-	assert.Equal(t, 0, plan.RampSchedule.CurrentStep,
-		"a deliberately different schedule starts its own progress at step 0")
-}
-
 func TestHandler_deletePlan(t *testing.T) {
 	ctx := context.Background()
 	mockStore := new(MockConfigStore)

internal/api/openapi.yaml

@@ -361,6 +361,12 @@ paths:
           $ref: '#/components/responses/Forbidden'
         '404':
           $ref: '#/components/responses/NotFound'
+        '409':
+          description: The plan changed concurrently, or the request changes the ramp schedule of a plan that has already completed a step. Keep the schedule, or create a new plan.
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'
     patch:
       operationId: patchPlan
       tags: [Plans]