From 88b943efc013336255a353e943d9deb383a7e667 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 10:38:53 +0200 Subject: [PATCH 1/2] fix(api): refuse a ramp shape change on a plan with completed steps updatePlan rebuilds the ramp schedule from the request. When the shape (type, percent, interval, step count) differed from the stored one, the plan restarted at step 0 and forgot the percentage already bought; a PUT that omitted ramp_schedule did the same by defaulting to immediate. Return 409 when the stored plan has CurrentStep > 0 and the requested shape differs. The message tells the operator to resend the current ramp_schedule or create a new plan. Same-shape edits and shape changes at step 0 are unchanged. Documented the 409 on PUT /api/plans/{id}. The request-to-plan merge moves into mergeUpdatedPlan to keep updatePlan under the cyclomatic limit. Replaces TestHandler_updatePlan_ChangingRampScheduleStartsAtStepZero, which asserted the restart behavior. Refs #386 --- internal/api/handler_plans.go | 49 ++++++++--- internal/api/handler_plans_ramp_shape_test.go | 83 +++++++++++++++++++ internal/api/handler_plans_test.go | 55 ------------ internal/api/openapi.yaml | 6 ++ 4 files changed, 125 insertions(+), 68 deletions(-) create mode 100644 internal/api/handler_plans_ramp_shape_test.go diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 96b59236c..2eca0fe7c 100644 --- a/internal/api/handler_plans.go +++ b/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,11 +348,6 @@ 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 } @@ -342,13 +355,15 @@ func (h *Handler) updatePlan(ctx context.Context, httpReq *events.LambdaFunction // 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 @@ -356,7 +371,15 @@ func preserveRampProgress(existingPlan, plan *config.PurchasePlan) { 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 diff --git a/internal/api/handler_plans_ramp_shape_test.go b/internal/api/handler_plans_ramp_shape_test.go new file mode 100644 index 000000000..b2c23a5ac --- /dev/null +++ b/internal/api/handler_plans_ramp_shape_test.go @@ -0,0 +1,83 @@ +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] + + tests := []struct { + name string + step int + body string + wantStatus int + wantStep int + wantType string + }{ + {"in-progress ramp, different shape is refused", 3, `{"name":"p","ramp_schedule":"monthly-10pct"}`, 409, 0, ""}, + {"in-progress ramp, omitted ramp_schedule is refused", 3, `{"name":"renamed"}`, 409, 0, ""}, + {"in-progress ramp, different percent is refused", 3, `{"name":"p","ramp_schedule":"custom","custom_step_percent":20,"custom_interval_days":7}`, 409, 0, ""}, + {"in-progress ramp, same shape keeps its step", 3, `{"name":"renamed","ramp_schedule":"weekly-25pct"}`, 200, 3, "weekly"}, + {"no completed step, different shape is allowed", 0, `{"name":"p","ramp_schedule":"monthly-10pct"}`, 200, 0, "monthly"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + rs := weekly + 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) + }) + } +} diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go index 2acd3d595..083124d70 100644 --- a/internal/api/handler_plans_test.go +++ b/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) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 7cef4b596..d8f21b21e 100644 --- a/internal/api/openapi.yaml +++ b/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] From 86de032dcc11ea7e97f67288f2a5c2166954bcc0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 10:47:33 +0200 Subject: [PATCH 2/2] test(api): pin the first completed step and the percent-only ramp change Two mutants survived the review of the ramp shape check: moving the boundary from CurrentStep > 0 to > 1, and dropping PercentPerStep from the shape comparison. Add a CurrentStep=1 row and a row whose stored and requested schedules differ only in percent (custom 30% vs 33%, both 3 steps), so each mutant now fails. Refs #386 --- internal/api/handler_plans_ramp_shape_test.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/api/handler_plans_ramp_shape_test.go b/internal/api/handler_plans_ramp_shape_test.go index b2c23a5ac..499cf8404 100644 --- a/internal/api/handler_plans_ramp_shape_test.go +++ b/internal/api/handler_plans_ramp_shape_test.go @@ -46,24 +46,28 @@ func putPlan(t *testing.T, existing *config.PurchasePlan, body string) (*events. 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", 3, `{"name":"p","ramp_schedule":"monthly-10pct"}`, 409, 0, ""}, - {"in-progress ramp, omitted ramp_schedule is refused", 3, `{"name":"renamed"}`, 409, 0, ""}, - {"in-progress ramp, different percent is refused", 3, `{"name":"p","ramp_schedule":"custom","custom_step_percent":20,"custom_interval_days":7}`, 409, 0, ""}, - {"in-progress ramp, same shape keeps its step", 3, `{"name":"renamed","ramp_schedule":"weekly-25pct"}`, 200, 3, "weekly"}, - {"no completed step, different shape is allowed", 0, `{"name":"p","ramp_schedule":"monthly-10pct"}`, 200, 0, "monthly"}, + {"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 := weekly + rs := tc.stored rs.CurrentStep = tc.step existing := &config.PurchasePlan{ID: planID, Name: "p", Enabled: true, RampSchedule: rs}