diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 79dec1cc..8722ffc8 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -388,6 +388,10 @@ func preserveRampProgress(existingPlan, plan *config.PurchasePlan) error { if rampScheduleShapeUnchanged(existingPlan.RampSchedule, plan.RampSchedule) { plan.RampSchedule.CurrentStep = existingPlan.RampSchedule.CurrentStep plan.RampSchedule.StartDate = existingPlan.RampSchedule.StartDate + // Editor-owned markers the request cannot express; dropping them hands + // the plan back to the interval formula. + plan.RampSchedule.DatesManaged = existingPlan.RampSchedule.DatesManaged + plan.RampSchedule.AllocationMode = existingPlan.RampSchedule.AllocationMode plan.NextExecutionDate = existingPlan.NextExecutionDate return nil } diff --git a/internal/api/handler_plans_managed_rename_integration_test.go b/internal/api/handler_plans_managed_rename_integration_test.go new file mode 100644 index 00000000..21821a57 --- /dev/null +++ b/internal/api/handler_plans_managed_rename_integration_test.go @@ -0,0 +1,36 @@ +//go:build integration + +package api + +import ( + "context" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/require" +) + +func TestUpdatePlanRenameKeepsManagedMarkers(t *testing.T) { + ctx := context.Background() + f := newCreateConcurrencyFixture(ctx, t) + _, err := f.pool.Exec(ctx, `UPDATE purchase_plans SET next_execution_date = now() + interval '3 days', + ramp_schedule = ramp_schedule || '{"dates_managed":true,"allocation_mode":"equal_count"}' WHERE id = $1`, f.planID) + require.NoError(t, err) + const snapshot = `SELECT jsonb_build_object('managed', ramp_schedule->'dates_managed', 'mode', ramp_schedule->'allocation_mode', + 'step', ramp_schedule->'current_step', 'start', ramp_schedule->'start_date', 'next', next_execution_date)::text + FROM purchase_plans WHERE id = $1` + var before, after, name string + require.NoError(t, f.pool.QueryRow(ctx, snapshot, f.planID).Scan(&before)) + + _, err = f.handler.updatePlan(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token", "Content-Type": "application/json"}, + Body: `{"name":"renamed","enabled":true,"notification_days_before":2,"ramp_schedule":"weekly-25pct"}`, + }, f.planID) + require.NoError(t, err) + + require.NoError(t, f.pool.QueryRow(ctx, snapshot, f.planID).Scan(&after)) + require.NoError(t, f.pool.QueryRow(ctx, `SELECT name FROM purchase_plans WHERE id = $1`, f.planID).Scan(&name)) + require.Equal(t, "renamed", name) + require.JSONEq(t, before, after, "a rename must not touch the editor-owned markers, clock or progress") + require.Contains(t, after, `"managed": true`) +} diff --git a/internal/config/errors.go b/internal/config/errors.go index 0831dcbc..ac8f63e2 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -15,6 +15,11 @@ var ErrEmptyPurchaseID = errors.New("purchase history record has no purchase id" // ErrPurchasePlanConflict means a plan changed after the caller read it. var ErrPurchasePlanConflict = errors.New("purchase plan changed") +// ErrEditorReconciliationRequired means the plan is editor-managed and a legacy +// writer tried to change what only the schedule editor may. Permanent for the +// request: callers map it to 409; a background caller would skip, not retry. +var ErrEditorReconciliationRequired = errors.New("plan is managed by the schedule editor") + // ErrExecutionNotInExpectedStatus is returned by TransitionExecutionStatus // when the target execution exists but its current status is not in the // allowed `fromStatuses` set -- i.e. the atomic CAS rejected because some diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 76b556b3..37647e4f 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -753,7 +753,7 @@ func (s *PostgresStore) UpdatePurchasePlan(ctx context.Context, plan *PurchasePl if err != nil { return err } - if updErr := s.UpdatePurchasePlanTx(ctx, tx, &pending); updErr != nil { + if updErr := s.updatePurchasePlanGuardedTx(ctx, tx, old, &pending); updErr != nil { return updErr } if old == nil || !rampBaselineStale(old, &pending) { @@ -775,8 +775,24 @@ func (s *PostgresStore) UpdatePurchasePlan(ctx context.Context, plan *PurchasePl return nil } -// UpdatePurchasePlanTx rejects snapshots that lost a race with another plan writer. +// UpdatePurchasePlanTx rejects snapshots that lost a race with another plan +// writer and refuses changes to what the editor owns on a managed plan. It +// re-locks the row, which every caller already holds, to judge the guard on +// the locked version. func (s *PostgresStore) UpdatePurchasePlanTx(ctx context.Context, tx pgx.Tx, plan *PurchasePlan) error { + old, err := s.LockPurchasePlanTx(ctx, tx, plan.ID) + if err != nil { + return err + } + return s.updatePurchasePlanGuardedTx(ctx, tx, old, plan) +} + +// updatePurchasePlanGuardedTx runs the managed-plan guard against old, the row +// locked by the caller's transaction, and then the optimistic update. +func (s *PostgresStore) updatePurchasePlanGuardedTx(ctx context.Context, tx pgx.Tx, old, plan *PurchasePlan) error { + if err := requireManagedPlanUpdateTx(old, plan); err != nil { + return err + } // Marshal services and ramp_schedule to JSONB servicesJSON, err := json.Marshal(plan.Services) if err != nil { diff --git a/internal/config/store_postgres_complete_step_test.go b/internal/config/store_postgres_complete_step_test.go index 4cac70b0..eec97f7c 100644 --- a/internal/config/store_postgres_complete_step_test.go +++ b/internal/config/store_postgres_complete_step_test.go @@ -163,6 +163,9 @@ func TestPGXMock_CompletePlanStep_LocksAndAdvances(t *testing.T) { WithArgs("plan-123"). WillReturnRows(rampPlanRows(t, "plan-123", ramp, now, stale, sql.NullTime{Valid: false})) expectFanOut(mock, "plan-123", 2, 3, 0) + mock.ExpectQuery(`SELECT[\s\S]*FROM purchase_plans[\s\S]*WHERE id = \$1 FOR UPDATE`). + WithArgs("plan-123"). + WillReturnRows(rampPlanRows(t, "plan-123", ramp, now, stale, sql.NullTime{Valid: false})) mock.ExpectQuery(`UPDATE purchase_plans`). WithArgs(completeStepUpdateArgs(2, stale, false)...). WillReturnRows(pgxmock.NewRows([]string{"updated_at"}).AddRow(now)) @@ -353,6 +356,10 @@ func TestPGXMock_CompletePlanStep_CompletedRampClearsNextDate(t *testing.T) { ramp := RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, CurrentStep: 4, TotalSteps: 4, StartDate: now} mock.ExpectBegin() + mock.ExpectQuery(`SELECT[\s\S]*FROM purchase_plans[\s\S]*WHERE id = \$1 FOR UPDATE`). + WithArgs("plan-done"). + WillReturnRows(rampPlanRows(t, "plan-done", ramp, now, stale, sql.NullTime{Valid: true, Time: now})) + // The managed-plan guard re-locks the row before the update. mock.ExpectQuery(`SELECT[\s\S]*FROM purchase_plans[\s\S]*WHERE id = \$1 FOR UPDATE`). WithArgs("plan-done"). WillReturnRows(rampPlanRows(t, "plan-done", ramp, now, stale, sql.NullTime{Valid: true, Time: now})) diff --git a/internal/config/store_postgres_managed_guard.go b/internal/config/store_postgres_managed_guard.go new file mode 100644 index 00000000..1c3b22f5 --- /dev/null +++ b/internal/config/store_postgres_managed_guard.go @@ -0,0 +1,60 @@ +package config + +import ( + "bytes" + "encoding/json" + "fmt" + "time" +) + +// requireManagedPlanUpdateTx refuses a legacy plan write that would change what +// the schedule editor owns on an editor-managed plan. old is the row locked by +// the caller's transaction; judging it, never a pre-lock copy, is what lets a +// writer that waited behind the editor see the committed marker. Only the +// editor command sets or clears dates_managed (it is part of ramp_schedule, so +// adopting or stripping it is refused as a ramp change); it does not come +// through here. +func requireManagedPlanUpdateTx(old, next *PurchasePlan) error { + if old == nil || (!old.RampSchedule.DatesManaged && !next.RampSchedule.DatesManaged) { + return nil + } + if field := managedPlanSensitiveChange(old, next); field != "" { + return fmt.Errorf("%w: %s changed on plan %s", ErrEditorReconciliationRequired, field, old.ID) + } + return nil +} + +// managedPlanSensitiveChange names the first editor-owned field whose +// persisted value differs, or "" when only name, enabled, disarming, the cap +// and the bookkeeping timestamps changed. +func managedPlanSensitiveChange(old, next *PurchasePlan) string { + switch { + case !sameJSON(old.RampSchedule, next.RampSchedule): + return "ramp_schedule" + case !sameJSON(old.Services, next.Services): + return "services" + case !sameDate(old.NextExecutionDate, next.NextExecutionDate): + return "next_execution_date" + case old.NotificationDaysBefore != next.NotificationDaysBefore: + return "notification_days_before" + case !old.AutoPurchase && next.AutoPurchase: + return "auto_purchase" + } + return "" +} + +// sameJSON compares what would be persisted: nil and empty slices marshal +// equal under omitempty, so a rebuilt but identical value is not a change. +func sameJSON(a, b any) bool { + aj, aErr := json.Marshal(a) + bj, bErr := json.Marshal(b) + return aErr == nil && bErr == nil && bytes.Equal(aj, bj) +} + +// sameDate compares instants; both sides come from the database or a copy of it. +func sameDate(a, b *time.Time) bool { + if a == nil || b == nil { + return a == b + } + return a.Equal(*b) +} diff --git a/internal/config/store_postgres_managed_guard_integration_test.go b/internal/config/store_postgres_managed_guard_integration_test.go new file mode 100644 index 00000000..b9bcce73 --- /dev/null +++ b/internal/config/store_postgres_managed_guard_integration_test.go @@ -0,0 +1,109 @@ +//go:build integration +// +build integration + +package config + +import ( + "context" + "testing" + "time" + + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/require" +) + +func storedPlanJSON(ctx context.Context, t *testing.T, f *rampFixture) string { + t.Helper() + var stored string + require.NoError(t, f.store.db.QueryRow(ctx, `SELECT to_jsonb(p)::text FROM purchase_plans p WHERE id = $1`, f.planID).Scan(&stored)) + return stored +} + +func TestManagedPlanUpdateGuard(t *testing.T) { + ctx := context.Background() + f := managedClockFixture(ctx, t, 1) + soon := time.Now().Add(48 * time.Hour) + refused := map[string]func(p *PurchasePlan){ + "marker strip": func(p *PurchasePlan) { p.RampSchedule.DatesManaged = false }, + "allocation mode": func(p *PurchasePlan) { p.RampSchedule.AllocationMode = RampAllocationEqualCount }, + "ramp percent": func(p *PurchasePlan) { p.RampSchedule.PercentPerStep = 50 }, + "current step": func(p *PurchasePlan) { p.RampSchedule.CurrentStep = 2 }, + "service key": func(p *PurchasePlan) { p.Services["ec2"] = ServiceConfig{Provider: "aws", Service: "ec2"} }, + "service content": func(p *PurchasePlan) { p.Services["future_service"] = ServiceConfig{Term: 3} }, + "next date": func(p *PurchasePlan) { p.NextExecutionDate = &soon }, + "notice": func(p *PurchasePlan) { p.NotificationDaysBefore += 3 }, + "arming": func(p *PurchasePlan) { p.AutoPurchase = true }, + "arming with rename": func(p *PurchasePlan) { p.AutoPurchase, p.Name = true, "armed" }, + "notice with disable": func(p *PurchasePlan) { p.NotificationDaysBefore, p.Enabled = 9, false }, + } + for name, mutate := range refused { + t.Run("refuses "+name, func(t *testing.T) { + plan, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + before := storedPlanJSON(ctx, t, f) + mutate(plan) + require.ErrorIs(t, f.store.UpdatePurchasePlan(ctx, plan), ErrEditorReconciliationRequired) + require.Equal(t, before, storedPlanJSON(ctx, t, f)) + }) + } + + t.Run("allows disarm, rename, cap and bookkeeping", func(t *testing.T) { + _, err := f.store.db.Exec(ctx, `UPDATE purchase_plans SET auto_purchase = true WHERE id = $1`, f.planID) + require.NoError(t, err) + plan, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + cap, now := 5.0, time.Now().Truncate(time.Microsecond) + plan.Name, plan.Enabled, plan.AutoPurchase = "renamed", false, false + plan.MaxStepCommitment, plan.LastExecutionDate, plan.LastNotificationSent = &cap, &now, &now + require.NoError(t, f.store.UpdatePurchasePlan(ctx, plan)) + var name string + var managed bool + require.NoError(t, f.store.db.QueryRow(ctx, `SELECT name, ramp_schedule->>'dates_managed' = 'true' FROM purchase_plans WHERE id = $1`, f.planID).Scan(&name, &managed)) + require.Equal(t, "renamed", name) + require.True(t, managed) + }) + + t.Run("allows a rebuilt identical plan", func(t *testing.T) { + plan, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + require.NoError(t, f.store.UpdatePurchasePlan(ctx, plan)) + }) +} + +func TestUnmanagedPlanUpdateGuard(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 0, 1, 3) + plan, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + plan.Name, plan.NotificationDaysBefore, plan.AutoPurchase = "free", 7, true + plan.RampSchedule.PercentPerStep = 40 + require.NoError(t, f.store.UpdatePurchasePlan(ctx, plan)) + + t.Run("refuses marker adoption", func(t *testing.T) { + plan.RampSchedule.DatesManaged = true + before := storedPlanJSON(ctx, t, f) + require.ErrorIs(t, f.store.UpdatePurchasePlan(ctx, plan), ErrEditorReconciliationRequired) + require.Equal(t, before, storedPlanJSON(ctx, t, f)) + }) + + t.Run("judges the locked row, not the caller's stale copy", func(t *testing.T) { + stale, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + _, err = f.store.db.Exec(ctx, `UPDATE purchase_plans SET ramp_schedule = ramp_schedule || '{"dates_managed":true}' WHERE id = $1`, f.planID) + require.NoError(t, err) + stale.Name = "stale rename" + require.ErrorIs(t, f.store.UpdatePurchasePlan(ctx, stale), ErrEditorReconciliationRequired) + }) +} + +func TestUpdatePurchasePlanTxGuardsDirectCallers(t *testing.T) { + ctx := context.Background() + f := managedClockFixture(ctx, t, 1) + plan, err := f.store.GetPurchasePlan(ctx, f.planID) + require.NoError(t, err) + before := storedPlanJSON(ctx, t, f) + plan.NotificationDaysBefore += 3 + err = f.store.WithTx(ctx, func(tx pgx.Tx) error { return f.store.UpdatePurchasePlanTx(ctx, tx, plan) }) + require.ErrorIs(t, err, ErrEditorReconciliationRequired) + require.Equal(t, before, storedPlanJSON(ctx, t, f)) +} diff --git a/known_issues/resolved/34_legacy_rename_strips_dates_managed.md b/known_issues/resolved/34_legacy_rename_strips_dates_managed.md new file mode 100644 index 00000000..0f6a3578 --- /dev/null +++ b/known_issues/resolved/34_legacy_rename_strips_dates_managed.md @@ -0,0 +1,15 @@ +# Known Issues: legacy plan rename strips editor markers + +> **Audit status (2026-10-16):** `0 needs triage ยท 1 resolved` + +## HIGH: PUT rename dropped dates_managed and allocation_mode + +**Files:** `internal/api/handler_plans.go` (`preserveRampProgress`), `internal/config/store_postgres.go` + +**Description:** A legacy `PUT /plans/{id}` rebuilds the ramp schedule from the request, and `preserveRampProgress` carried over only the step, start date and next date. A bare rename of an editor-managed plan therefore wrote the plan back without `dates_managed` and `allocation_mode`, returning it to the interval formula (#631). + +**Why deferred:** Not deferred; found while planning the legacy-writer guards and fixed in the same change. + +**Issue:** #631 + +**Status:** Resolved in the G1a guard PR. `preserveRampProgress` keeps both markers, and `UpdatePurchasePlanTx` refuses legacy writes that change editor-owned fields of a managed plan. Regression test: `TestUpdatePlanRenameKeepsManagedMarkers`.