diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go index 82132a4a..4842539d 100644 --- a/internal/api/execution_scope_test.go +++ b/internal/api/execution_scope_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "testing" + "time" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" @@ -160,6 +161,7 @@ func scopeTestExecution(k scopeTestKind, status string) *config.PurchaseExecutio CloudAccountID: k.cloudAccountID, Recommendations: k.recs, CreatedByUserID: &creator, + ScheduledDate: time.Now().Add(time.Hour), // not due, so resume needs no reschedule } } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 76428d54..f5669e40 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -228,6 +228,13 @@ func (h *Handler) resumePlannedPurchase(ctx context.Context, req *events.LambdaF return nil, err } + // A recommendation-bearing row that is past its date is moved to + // now + NotificationDaysBefore on resume; a plan without a veto window + // would move it to "now", so refuse before changing anything. + if checkErr := h.requireResumeVetoWindow(ctx, executionID, time.Now()); checkErr != nil { + return nil, checkErr + } + // Atomically transition from paused back to pending resumed, transitionErr := h.config.TransitionExecutionStatus(ctx, executionID, []string{"paused"}, "pending", resolveCreatorUserID(session)) if transitionErr != nil { @@ -240,6 +247,26 @@ func (h *Handler) resumePlannedPurchase(ctx context.Context, req *events.LambdaF return &StatusResponse{Status: "resumed"}, nil } +// requireResumeVetoWindow refuses to resume a recommendation-bearing row whose +// scheduled_date has passed when its plan has no veto window (S13). +func (h *Handler) requireResumeVetoWindow(ctx context.Context, executionID string, now time.Time) error { + row, err := h.config.GetExecutionByID(ctx, executionID) + if err != nil { + return fmt.Errorf("failed to read execution %s before resume: %w", executionID, err) + } + if len(row.Recommendations) == 0 || !row.ScheduledDate.Before(now) { + return nil + } + plan, err := h.config.GetPurchasePlan(ctx, row.PlanID) + if err != nil { + return fmt.Errorf("failed to read plan %s before resuming execution %s: %w", row.PlanID, executionID, err) + } + if vetoErr := plan.RequireVetoWindow(); vetoErr != nil { + return NewClientError(409, fmt.Sprintf("execution %s cannot be resumed: %v", executionID, vetoErr)) + } + return nil +} + // pushPastScheduledDate moves a resumed recommendation-bearing row whose // scheduled_date has already passed to now + the plan's NotificationDaysBefore // days, so that once it is re-notified the scheduler cannot buy it before the @@ -255,6 +282,9 @@ func (h *Handler) pushPastScheduledDate(ctx context.Context, row *config.Purchas if err != nil { return fmt.Errorf("failed to read plan %s to reschedule resumed execution %s: %w", row.PlanID, row.ExecutionID, err) } + if vetoErr := plan.RequireVetoWindow(); vetoErr != nil { + return NewClientError(409, fmt.Sprintf("execution %s was resumed but cannot be rescheduled: %v", row.ExecutionID, vetoErr)) + } // Conditional single-column update, not a full-row save: a cancel or // approve landing after the resume must not be overwritten. A lost // condition is a benign no-op (the row is no longer awaiting approval). diff --git a/internal/api/handler_purchases_notified_integration_test.go b/internal/api/handler_purchases_notified_integration_test.go index 1eb8997a..5369c4dc 100644 --- a/internal/api/handler_purchases_notified_integration_test.go +++ b/internal/api/handler_purchases_notified_integration_test.go @@ -85,3 +85,16 @@ func TestNotifiedRow_DeleteReleasesSuppressions(t *testing.T) { assert.Equal(t, "canceled", f.state(t).Status) assert.Zero(t, f.suppressionCount(t)) } + +// S13 on resume: without a veto window a due recommendation-bearing row stays paused. +func TestNotifiedRow_ResumeRefusedWithoutVetoWindow(t *testing.T) { + f := newPauseClaimFixture(t) + plan, err := f.store.GetPurchasePlan(f.ctx, f.execution.PlanID) + require.NoError(t, err) + plan.NotificationDaysBefore = 0 + require.NoError(t, f.store.UpdatePurchasePlan(f.ctx, plan)) + f.setRow(t, "paused", time.Now().Add(-48*time.Hour)) + + assertPauseConflict(t, f.resume()) + assert.Equal(t, "paused", f.state(t).Status) +} diff --git a/internal/api/handler_purchases_notified_test.go b/internal/api/handler_purchases_notified_test.go index 98db138c..1f672293 100644 --- a/internal/api/handler_purchases_notified_test.go +++ b/internal/api/handler_purchases_notified_test.go @@ -54,6 +54,7 @@ func TestResumePlannedPurchase_PastScheduledDate(t *testing.T) { t.Run(tc.name, func(t *testing.T) { handler, store, req := notifiedAdminHandler(ctx) row := &config.PurchaseExecution{ExecutionID: notifiedExecID, PlanID: "plan-1", Status: "pending", Recommendations: tc.recs, ScheduledDate: tc.date} + store.On("GetExecutionByID", ctx, notifiedExecID).Return(row, nil).Maybe() store.On("TransitionExecutionStatus", ctx, notifiedExecID, []string{"paused"}, "pending", mock.Anything).Return(row, nil) store.On("GetPurchasePlan", ctx, "plan-1").Return(&config.PurchasePlan{ID: "plan-1", NotificationDaysBefore: 3}, nil).Maybe() var movedTo *time.Time @@ -87,3 +88,39 @@ func TestCancelOrRecoverExecution_ReleasesSuppressionsOfNotifiedRow(t *testing.T require.NoError(t, err) store.AssertCalled(t, "DeleteSuppressionsByExecution", ctx, notifiedExecID) } + +// S13 on the resume path: a plan without a veto window must not have a due +// recommendation-bearing row moved to "now"; nothing is changed. +func TestResumePlannedPurchase_RefusesWithoutVetoWindow(t *testing.T) { + ctx := context.Background() + recs := []config.RecommendationRecord{{ID: "rec-1", Count: 1}} + for _, tc := range []struct { + name string + recs []config.RecommendationRecord + date time.Time + refused bool + }{ + {"rec-bearing past date, 0 days", recs, time.Now().Add(-time.Hour), true}, + {"rec-bearing future date, 0 days", recs, time.Now().Add(time.Hour), false}, + {"empty row past date, 0 days", nil, time.Now().Add(-time.Hour), false}, + } { + t.Run(tc.name, func(t *testing.T) { + handler, store, req := notifiedAdminHandler(ctx) + row := &config.PurchaseExecution{ExecutionID: notifiedExecID, PlanID: "plan-1", Status: "paused", Recommendations: tc.recs, ScheduledDate: tc.date} + store.On("GetExecutionByID", ctx, notifiedExecID).Return(row, nil) + store.On("GetPurchasePlan", ctx, "plan-1").Return(&config.PurchasePlan{ID: "plan-1", NotificationDaysBefore: 0}, nil).Maybe() + store.On("TransitionExecutionStatus", ctx, notifiedExecID, []string{"paused"}, "pending", mock.Anything).Return(row, nil).Maybe() + store.On("RescheduleAwaitingExecution", ctx, notifiedExecID, mock.Anything).Return(true, nil).Maybe() + + _, err := handler.resumePlannedPurchase(ctx, req, notifiedExecID) + if !tc.refused { + require.NoError(t, err) + return + } + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 409, ce.code) + store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + } +} diff --git a/internal/api/handler_purchases_pause_claim_integration_test.go b/internal/api/handler_purchases_pause_claim_integration_test.go index 2619b391..cd3df35a 100644 --- a/internal/api/handler_purchases_pause_claim_integration_test.go +++ b/internal/api/handler_purchases_pause_claim_integration_test.go @@ -88,6 +88,7 @@ func newPauseClaimFixture(t *testing.T) *pauseClaimFixture { plan, err := f.store.GetPurchasePlan(ctx, f.planID) require.NoError(t, err) plan.AutoPurchase = true + plan.NotificationDaysBefore = 3 plan.MaxStepCommitment = &pauseClaimCap require.NoError(t, f.store.UpdatePurchasePlan(ctx, plan)) globalCfg, err := f.store.GetGlobalConfig(ctx) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index b169cbbc..169206e7 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1566,6 +1566,7 @@ func TestHandler_resumePlannedPurchase(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"paused"}, "pending", mock.Anything).Return(resumed, nil) + mockStore.On("GetExecutionByID", mock.Anything, mock.Anything).Return(resumed, nil).Maybe() handler := &Handler{config: mockStore, auth: mockAuth} @@ -2172,6 +2173,7 @@ func TestHandler_resumePlannedPurchase_NilExecution(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"paused"}, "pending", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) + mockStore.On("GetExecutionByID", ctx, "99999999-9999-9999-9999-999999999999").Return(&config.PurchaseExecution{}, nil).Maybe() handler := &Handler{config: mockStore, auth: mockAuth} diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 4e75b3e2..6b4dd45b 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -1250,6 +1250,7 @@ func TestHandler_HandleRequest_ResumePlannedPurchase(t *testing.T) { resumed := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "pending"} mockStore.On("TransitionExecutionStatus", mock.Anything, "11111111-1111-1111-1111-111111111111", []string{"paused"}, "pending", mock.Anything).Return(resumed, nil) + mockStore.On("GetExecutionByID", mock.Anything, mock.Anything).Return(resumed, nil).Maybe() handler := &Handler{config: mockStore, auth: mockAuth, corsAllowedOrigin: "*", apiKey: "test-key"} diff --git a/internal/config/plan_veto_window.go b/internal/config/plan_veto_window.go new file mode 100644 index 00000000..898bd98f --- /dev/null +++ b/internal/config/plan_veto_window.go @@ -0,0 +1,21 @@ +package config + +import ( + "errors" + "fmt" +) + +// ErrNoVetoWindow marks a plan whose notification window is empty: with +// NotificationDaysBefore < 1 the email goes out when the purchase is already +// due, so the human veto the notified status stands for has no time to happen. +var ErrNoVetoWindow = errors.New("plan has no veto window") + +// RequireVetoWindow refuses a plan that cannot give an emailed purchase at +// least one day before it runs. Attaching recommendations to a plan step and +// re-scheduling a resumed recommendation-bearing row both call it. +func (p *PurchasePlan) RequireVetoWindow() error { + if p.NotificationDaysBefore < 1 { + return fmt.Errorf("%w: plan %s has notification_days_before=%d, need at least 1", ErrNoVetoWindow, p.ID, p.NotificationDaysBefore) + } + return nil +} diff --git a/internal/config/plan_veto_window_test.go b/internal/config/plan_veto_window_test.go new file mode 100644 index 00000000..f1c22503 --- /dev/null +++ b/internal/config/plan_veto_window_test.go @@ -0,0 +1,32 @@ +package config + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRequireVetoWindow(t *testing.T) { + for days, wantErr := range map[int]bool{-1: true, 0: true, 1: false, 7: false} { + err := (&PurchasePlan{ID: "p", NotificationDaysBefore: days}).RequireVetoWindow() + if wantErr { + assert.ErrorIs(t, err, ErrNoVetoWindow, "days=%d", days) + } else { + assert.NoError(t, err, "days=%d", days) + } + } +} + +func TestPlanStepLockKeys_SortedDedupedValidated(t *testing.T) { + keys, err := planStepLockKeys([]string{"gcp", "aws", "azure", "aws"}) + require.NoError(t, err) + assert.Equal(t, []string{"cudly:planstep:aws", "cudly:planstep:azure", "cudly:planstep:gcp"}, keys) + + _, err = planStepLockKeys(nil) + assert.Error(t, err) + _, err = planStepLockKeys([]string{"aws", ""}) + assert.Error(t, err) + assert.Error(t, (&PostgresStore{}).LockPlanProvidersTx(context.Background(), nil, nil)) +} diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index dcc08af9..9e8c5dd6 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1680,28 +1680,37 @@ func (s *PostgresStore) GetPendingExecutionsTx(ctx context.Context, tx pgx.Tx) ( // planStepProviderLockPrefix namespaces the per-provider plan-step advisory lock. const planStepProviderLockPrefix = "cudly:planstep:" -// LockPlanProvidersTx implements StoreInterface.LockPlanProvidersTx. Sorted -// acquisition keeps two callers with overlapping provider sets from -// deadlocking on each other. -func (s *PostgresStore) LockPlanProvidersTx(ctx context.Context, tx pgx.Tx, providers []string) error { +// planStepLockKeys returns the advisory-lock keys for providers: validated, +// de-duplicated and sorted. Sorted acquisition keeps two callers with +// overlapping provider sets from deadlocking on each other. +func planStepLockKeys(providers []string) ([]string, error) { if len(providers) == 0 { - return errors.New("lock plan providers: no providers given") + return nil, errors.New("lock plan providers: no providers given") } - sorted := make([]string, 0, len(providers)) + keys := make([]string, 0, len(providers)) seen := make(map[string]struct{}, len(providers)) for _, p := range providers { if p == "" { - return errors.New("lock plan providers: empty provider name") + return nil, errors.New("lock plan providers: empty provider name") } if _, dup := seen[p]; !dup { seen[p] = struct{}{} - sorted = append(sorted, p) + keys = append(keys, planStepProviderLockPrefix+p) } } - sort.Strings(sorted) - for _, p := range sorted { - if _, err := tx.Exec(ctx, "SELECT pg_advisory_xact_lock(hashtextextended($1, 0))", planStepProviderLockPrefix+p); err != nil { - return fmt.Errorf("lock plan provider %s: %w", p, err) + sort.Strings(keys) + return keys, nil +} + +// LockPlanProvidersTx implements StoreInterface.LockPlanProvidersTx. +func (s *PostgresStore) LockPlanProvidersTx(ctx context.Context, tx pgx.Tx, providers []string) error { + keys, err := planStepLockKeys(providers) + if err != nil { + return err + } + for _, key := range keys { + if _, err := tx.Exec(ctx, "SELECT pg_advisory_xact_lock(hashtextextended($1, 0))", key); err != nil { + return fmt.Errorf("lock plan step key %s: %w", key, err) } } return nil diff --git a/internal/email/sender.go b/internal/email/sender.go index 03a9d488..6fdab5f6 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -578,6 +578,14 @@ type NotificationData struct { DaysUntilPurchase int TotalUpfrontCost float64 TotalSavings float64 + // TotalCommitment, TotalRecurring, TotalUnits and StepCap describe what a + // scheduled plan step will buy. TotalCommitment is upfront plus the + // recurring charges over each term; StepCap is the plan's per-step cap, 0 + // when none is shown. A zero TotalCommitment omits the block. + TotalCommitment float64 + TotalRecurring float64 + TotalUnits int + StepCap float64 // RevocationToken is the one-time token embedded in the revocation link // of a post-execution notification email. When non-empty, the template // renders a "Revoke this purchase" CTA that hits @@ -607,6 +615,15 @@ type RecommendationSummary struct { MonthlySavings float64 Term int UpfrontCost float64 + // MonthlyCost, Commitment, AvailabilityZone, Platform and Tenancy are the + // per-row detail of a scheduled plan step. Commitment is upfront plus + // monthly over the term, so a no-upfront (Azure monthly) row does not read + // as $0. + MonthlyCost float64 + Commitment float64 + AvailabilityZone string + Platform string + Tenancy string } // RIExchangeNotificationData holds data for RI exchange email templates. diff --git a/internal/email/templates.go b/internal/email/templates.go index 5e76c456..2b6cb211 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -37,10 +37,13 @@ const scheduledPurchaseTemplate = `CUDly - Scheduled Purchase in {{.DaysUntilPur ========================================= Based on your Purchase Plan "{{.PlanName}}", the following commitments will be purchased on {{.PurchaseDate}}: - +{{if gt .TotalCommitment 0.0}} +Will buy {{len .Recommendations}} recommendation(s) ({{.TotalUnits}} units) for ${{printf "%.2f" .TotalCommitment}} total commitment (upfront ${{printf "%.2f" .TotalUpfrontCost}} + ${{printf "%.2f" .TotalRecurring}} recurring over the terms){{if gt .StepCap 0.0}}, cap ${{printf "%.2f" .StepCap}}{{end}}, on {{.PurchaseDate}} unless canceled. +{{end}} {{range .Recommendations}} -- {{.Count}}x {{.ResourceType}}{{if .Engine}} ({{.Engine}}){{end}} in {{.Region}} - Service: {{.Service}} | Est. Savings: ${{printf "%.2f" .MonthlySavings}}/month +- {{.Count}}x {{.ResourceType}}{{if .Engine}} ({{.Engine}}){{end}} in {{.Region}}{{if .AvailabilityZone}} ({{.AvailabilityZone}}){{end}}{{if .Platform}} | {{.Platform}}{{end}}{{if .Tenancy}} | {{.Tenancy}}{{end}} + Service: {{.Service}} | Est. Savings: ${{printf "%.2f" .MonthlySavings}}/month{{if gt .Commitment 0.0}} + Term: {{.Term}} yr {{.Payment}} | Upfront ${{printf "%.2f" .UpfrontCost}} + ${{printf "%.2f" .MonthlyCost}}/month | Commitment ${{printf "%.2f" .Commitment}}{{end}} {{end}} Summary: diff --git a/internal/purchase/notification_data_test.go b/internal/purchase/notification_data_test.go new file mode 100644 index 00000000..5f3493b7 --- /dev/null +++ b/internal/purchase/notification_data_test.go @@ -0,0 +1,48 @@ +package purchase + +import ( + "encoding/json" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/email" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Fixture rows: nothing attaches recommendations to a plan step yet. +func TestBuildNotificationData_ShowsTotalCommitmentAndCap(t *testing.T) { + monthly := 100.0 + capAmount := 5000.0 + plan := config.PurchasePlan{ID: "plan-1", Name: "Azure ramp", MaxStepCommitment: &capAmount} + exec := &config.PurchaseExecution{ + ExecutionID: "exec-1", PlanID: "plan-1", ScheduledDate: time.Date(2026, 11, 3, 0, 0, 0, 0, time.UTC), + TotalUpfrontCost: 300, + Recommendations: []config.RecommendationRecord{ + // Azure monthly billing: no upfront, the commitment is all recurring. + {Service: "compute", ResourceType: "Standard_D2s_v3", Region: "westeurope", Count: 2, Term: 1, Payment: "monthly", MonthlyCost: &monthly}, + {Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", AvailabilityZone: "us-east-1a", Count: 3, Term: 3, Payment: "partial-upfront", UpfrontCost: 300, + Details: json.RawMessage(`{"platform":"Linux/UNIX","tenancy":"default"}`)}, + }, + } + m := &Manager{} + + data := m.buildNotificationData(plan, exec, "tok", 3, "ops@example.invalid") + + assert.InDelta(t, 1500.0, data.TotalCommitment, 1e-9) + assert.InDelta(t, 1200.0, data.TotalRecurring, 1e-9) + assert.Equal(t, 5, data.TotalUnits) + assert.InDelta(t, 5000.0, data.StepCap, 1e-9) + require.Len(t, data.Recommendations, 2) + assert.InDelta(t, 1200.0, data.Recommendations[0].Commitment, 1e-9, "an Azure monthly row is not $0") + assert.Equal(t, "us-east-1a", data.Recommendations[1].AvailabilityZone) + assert.Equal(t, "Linux/UNIX", data.Recommendations[1].Platform) + assert.Equal(t, "default", data.Recommendations[1].Tenancy) + + body, err := email.RenderScheduledPurchaseEmail(data) + require.NoError(t, err) + assert.Contains(t, body, "Will buy 2 recommendation(s) (5 units) for $1500.00 total commitment (upfront $300.00 + $1200.00 recurring over the terms), cap $5000.00, on November 3, 2026 unless canceled.") + assert.Contains(t, body, "Term: 1 yr monthly | Upfront $0.00 + $100.00/month | Commitment $1200.00") + assert.Contains(t, body, "in us-east-1 (us-east-1a) | Linux/UNIX | default") +} diff --git a/internal/purchase/notifications.go b/internal/purchase/notifications.go index 6572b4b1..baccac5d 100644 --- a/internal/purchase/notifications.go +++ b/internal/purchase/notifications.go @@ -316,16 +316,41 @@ func (m *Manager) buildNotificationData(plan config.PurchasePlan, exec *config.P RecipientEmail: notifyEmail, } + if plan.MaxStepCommitment != nil { + data.StepCap = *plan.MaxStepCommitment + } for _rvc := range exec.Recommendations { rec := exec.Recommendations[_rvc] - data.Recommendations = append(data.Recommendations, email.RecommendationSummary{ - Service: rec.Service, - ResourceType: rec.ResourceType, - Engine: rec.Engine, - Region: rec.Region, - Count: rec.Count, - MonthlySavings: rec.Savings, - }) + summary := email.RecommendationSummary{ + Service: rec.Service, + ResourceType: rec.ResourceType, + Engine: rec.Engine, + Region: rec.Region, + Payment: rec.Payment, + Term: rec.Term, + Count: rec.Count, + MonthlySavings: rec.Savings, + UpfrontCost: rec.UpfrontCost, + Commitment: rec.TotalCommitment(), + AvailabilityZone: rec.AvailabilityZone, + } + var recurring float64 + if rec.MonthlyCost != nil { + summary.MonthlyCost = *rec.MonthlyCost + recurring = *rec.MonthlyCost * float64(rec.Term*12) + } + // Platform and tenancy are display only; a Details blob that does not + // decode is logged and shown without them (the cap and purchase paths + // decode it themselves and fail loud). + if ci, err := rec.ComputeIdentity(); err != nil { + logging.Warnf("purchase[%s]: notification row without platform/tenancy: %v", exec.ExecutionID, err) + } else { + summary.Platform, summary.Tenancy = ci.Platform, ci.Tenancy + } + data.TotalCommitment += summary.Commitment + data.TotalRecurring += recurring + data.TotalUnits += rec.Count + data.Recommendations = append(data.Recommendations, summary) } return data