diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index 560df22f..8c6e766c 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -770,6 +770,40 @@ describe('Plans Module', () => { await expect(loadPlans()).resolves.not.toThrow(); }); + test('renders notified purchase with run, pause and disable buttons', async () => { + (api.getPlans as jest.Mock).mockResolvedValue({ plans: [] }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ + purchases: [ + { + id: 'purchase-1', + plan_id: 'plan-1', + plan_name: 'Test Plan', + scheduled_date: '2024-02-20', + provider: 'aws', + service: 'ec2', + resource_type: 't3.medium', + region: 'us-east-1', + count: 5, + term: 1, + payment: 'all-upfront', + upfront_cost: 1000, + estimated_savings: 200, + status: 'notified', + step_number: 1, + total_steps: 4 + } + ] + }); + + await loadPlans(); + + const container = document.getElementById('planned-purchases-list'); + expect(container?.innerHTML).toContain('data-action="run"'); + expect(container?.innerHTML).toContain('data-action="pause"'); + expect(container?.innerHTML).toContain('data-action="disable"'); + expect(container?.innerHTML).not.toContain('data-action="resume"'); + }); + test('renders paused purchase with correct buttons', async () => { (api.getPlans as jest.Mock).mockResolvedValue({ plans: [] }); (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index c111b915..73cea304 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -485,7 +485,7 @@ export interface PlannedPurchase { // upfront_cost and estimated_savings are then placeholders, not data // (platform#609). has_recommendations: boolean; - status: 'pending' | 'paused' | 'running' | 'completed' | 'failed'; + status: 'pending' | 'notified' | 'paused' | 'running' | 'completed' | 'failed'; step_number: number; total_steps: number; // created_by_user_id is the UUID of the user who scheduled the purchase. diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 9b9f5f3e..ebe2cd14 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -685,7 +685,7 @@ function canManageScheduledPurchase(purchase: PlannedPurchase): boolean { function renderPlannedPurchaseRow(purchase: PlannedPurchase): string { const statusClass = getPlannedPurchaseStatusClass(purchase.status); const isPaused = purchase.status === 'paused'; - const isPending = purchase.status === 'pending'; + const isPending = purchase.status === 'pending' || purchase.status === 'notified'; const canRun = isPending || isPaused; const termCell = purchase.term > 0 ? `${purchase.term}yr ${escapeHtml((purchase.payment ?? '').replace('-', ' '))}` @@ -767,7 +767,8 @@ function renderPlannedPurchaseRow(purchase: PlannedPurchase): string { */ function getPlannedPurchaseStatusClass(status: string): string { switch (status) { - case 'pending': return 'status-pending'; + case 'pending': + case 'notified': return 'status-pending'; case 'paused': return 'status-paused'; case 'running': return 'status-running'; case 'completed': return 'status-completed'; diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 09ef08fb..d79860d9 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -307,6 +307,10 @@ func (m *mockConfigStore) TransitionExecutionStatusPinned(ctx context.Context, e return nil, nil } +func (m *mockConfigStore) RescheduleAwaitingExecution(_ context.Context, _ string, _ time.Time) (bool, error) { + return false, nil +} + func (m *mockConfigStore) SetCancelledBy(_ context.Context, _ string, _ string) error { return nil } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index e65e8025..76428d54 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -205,7 +205,7 @@ func (h *Handler) pausePlannedPurchase(ctx context.Context, req *events.LambdaFu } // Atomically transition to paused - if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending"}, "paused", resolveCreatorUserID(session)); err != nil { + if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "notified"}, "paused", resolveCreatorUserID(session)); err != nil { return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be paused: %v", executionID, err)) } @@ -229,13 +229,41 @@ func (h *Handler) resumePlannedPurchase(ctx context.Context, req *events.LambdaF } // Atomically transition from paused back to pending - if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"paused"}, "pending", resolveCreatorUserID(session)); err != nil { - return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be resumed: %v", executionID, err)) + resumed, transitionErr := h.config.TransitionExecutionStatus(ctx, executionID, []string{"paused"}, "pending", resolveCreatorUserID(session)) + if transitionErr != nil { + return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be resumed: %v", executionID, transitionErr)) + } + if pushErr := h.pushPastScheduledDate(ctx, resumed, time.Now()); pushErr != nil { + return nil, pushErr } return &StatusResponse{Status: "resumed"}, 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 +// veto window has run. The row is pending, which the scheduler does not +// execute while it carries recommendations, so the gap between the resume +// and this write cannot buy. Rows without recommendations keep their date +// (the scheduler fails them). +func (h *Handler) pushPastScheduledDate(ctx context.Context, row *config.PurchaseExecution, now time.Time) error { + 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 to reschedule resumed execution %s: %w", row.PlanID, row.ExecutionID, err) + } + // 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). + if _, err := h.config.RescheduleAwaitingExecution(ctx, row.ExecutionID, now.AddDate(0, 0, plan.NotificationDaysBefore)); err != nil { + return fmt.Errorf("failed to reschedule resumed execution %s: %w", row.ExecutionID, err) + } + return nil +} + // runPlannedPurchase forces a pending or paused scheduled purchase to // execute immediately (the "Run now" button) instead of waiting for its // scheduled date. It delegates to purchase.Manager.RunPlannedPurchaseNow, @@ -454,14 +482,21 @@ func (h *Handler) disablePlanIfInScope(ctx context.Context, session *Session, pl } // cancelOrRecoverExecution transitions the execution to "canceled" if it is -// still in {pending, paused}. If a prior attempt already canceled it +// still in {pending, notified, paused}. If a prior attempt already canceled it // (ErrExecutionNotInExpectedStatus), it fetches the row instead so the caller // can still drive the plan-disable side-effect, keeping the operation // idempotent across retries. // actor is the UUID of the user initiating the cancel (nil for system-initiated paths). func (h *Handler) cancelOrRecoverExecution(ctx context.Context, executionID string, actor *string) (*config.PurchaseExecution, error) { - canceled, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "canceled", actor) + canceled, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "notified", "paused"}, "canceled", actor) if err == nil { + // A recommendation-bearing row owns suppressions (written with its + // recommendations); release them like the other cancel paths. The + // delete is idempotent and also runs on the recovery path below, so a + // retry heals a failure here. + if delErr := h.config.DeleteSuppressionsByExecution(ctx, executionID); delErr != nil { + return nil, fmt.Errorf("cancel execution %s: failed to release suppressions: %w", executionID, delErr) + } return canceled, nil } if !errors.Is(err, config.ErrExecutionNotInExpectedStatus) { @@ -500,6 +535,9 @@ func (h *Handler) cancelOrRecoverExecution(ctx context.Context, executionID stri if existing.Status == config.LegacyStatusCanceled { existing.Status = config.StatusCanceled } + if delErr := h.config.DeleteSuppressionsByExecution(ctx, executionID); delErr != nil { + return nil, fmt.Errorf("cancel execution %s: failed to release suppressions: %w", executionID, delErr) + } return existing, nil } diff --git a/internal/api/handler_purchases_notified_integration_test.go b/internal/api/handler_purchases_notified_integration_test.go new file mode 100644 index 00000000..1eb8997a --- /dev/null +++ b/internal/api/handler_purchases_notified_integration_test.go @@ -0,0 +1,87 @@ +//go:build integration + +package api + +import ( + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real PostgreSQL through the handlers. The rec-bearing row is a hand-built +// fixture (nothing attaches recommendations to a plan step yet). + +func (f *pauseClaimFixture) setRow(t *testing.T, status string, date time.Time) { + t.Helper() + _, err := f.pool.Exec(f.ctx, `UPDATE purchase_executions SET status=$2, scheduled_date=$3 WHERE execution_id=$1`, f.execution.ExecutionID, status, date) + require.NoError(t, err) +} + +func (f *pauseClaimFixture) scheduledDate(t *testing.T) time.Time { + t.Helper() + row, err := f.store.GetExecutionByID(f.ctx, f.execution.ExecutionID) + require.NoError(t, err) + return row.ScheduledDate +} + +func (f *pauseClaimFixture) suppressionCount(t *testing.T) int { + t.Helper() + var n int + require.NoError(t, f.pool.QueryRow(f.ctx, `SELECT count(*) FROM purchase_suppressions WHERE execution_id=$1`, f.execution.ExecutionID).Scan(&n)) + return n +} + +func TestNotifiedRow_PauseResumeHandlersMoveDatePastWindow(t *testing.T) { + f := newPauseClaimFixture(t) + f.setRow(t, "notified", time.Now().Add(-48*time.Hour)) + require.NoError(t, f.pause()) + require.Equal(t, "paused", f.state(t).Status) + + before := time.Now() + require.NoError(t, f.resume()) + assert.Equal(t, "pending", f.state(t).Status) + plan, err := f.store.GetPurchasePlan(f.ctx, f.execution.PlanID) + require.NoError(t, err) + assert.WithinDuration(t, before.AddDate(0, 0, plan.NotificationDaysBefore), f.scheduledDate(t), time.Minute) +} + +// F1: a cancel landing between the paused->pending CAS and the reschedule is +// not overwritten by the reschedule (replayed with the stale resume snapshot). +func TestNotifiedRow_RescheduleDoesNotOverwriteConcurrentCancel(t *testing.T) { + f := newPauseClaimFixture(t) + past := time.Now().Add(-48 * time.Hour) + f.setRow(t, "paused", past) + resumed, err := f.store.TransitionExecutionStatus(f.ctx, f.execution.ExecutionID, []string{"paused"}, "pending", nil) + require.NoError(t, err) + _, err = f.store.TransitionExecutionStatus(f.ctx, f.execution.ExecutionID, []string{"pending", "notified", "paused"}, "canceled", nil) + require.NoError(t, err) + dateBefore := f.scheduledDate(t) + + require.NoError(t, f.handler.pushPastScheduledDate(f.ctx, resumed, time.Now())) + + assert.Equal(t, "canceled", f.state(t).Status) + assert.WithinDuration(t, dateBefore, f.scheduledDate(t), time.Second) +} + +// F2: canceling or deleting a notified rec-bearing row releases its suppressions. +func TestNotifiedRow_DeleteReleasesSuppressions(t *testing.T) { + f := newPauseClaimFixture(t) + f.setRow(t, "notified", time.Now().Add(time.Hour)) + global, err := f.store.GetGlobalConfig(f.ctx) + require.NoError(t, err) + sups := config.BuildSuppressions(f.execution.Recommendations, f.execution.ExecutionID, global, time.Now()) + require.NotEmpty(t, sups) + for i := range sups { + require.NoError(t, f.store.CreateSuppression(f.ctx, &sups[i])) + } + require.Positive(t, f.suppressionCount(t)) + + _, err = f.handler.deletePlannedPurchase(f.ctx, f.request, f.execution.ExecutionID) + require.NoError(t, err) + + assert.Equal(t, "canceled", f.state(t).Status) + assert.Zero(t, f.suppressionCount(t)) +} diff --git a/internal/api/handler_purchases_notified_test.go b/internal/api/handler_purchases_notified_test.go new file mode 100644 index 00000000..98db138c --- /dev/null +++ b/internal/api/handler_purchases_notified_test.go @@ -0,0 +1,89 @@ +package api + +import ( + "context" + "testing" + "time" + + "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" +) + +// Fixture rows (mock store): nothing attaches recommendations to a plan step +// yet, so rec-bearing rows are built by hand. + +const notifiedExecID = "11111111-1111-1111-1111-111111111111" + +func notifiedAdminHandler(ctx context.Context) (*Handler, *MockConfigStore, *events.LambdaFunctionURLRequest) { + 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() + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}} + return &Handler{config: store, auth: auth}, store, req +} + +func TestPausePlannedPurchase_AcceptsNotifiedRow(t *testing.T) { + ctx := context.Background() + handler, store, req := notifiedAdminHandler(ctx) + store.On("TransitionExecutionStatus", ctx, notifiedExecID, []string{"pending", "notified"}, "paused", mock.Anything). + Return(&config.PurchaseExecution{ExecutionID: notifiedExecID, Status: "paused"}, nil) + + res, err := handler.pausePlannedPurchase(ctx, req, notifiedExecID) + require.NoError(t, err) + assert.Equal(t, "paused", res.Status) +} + +func TestResumePlannedPurchase_PastScheduledDate(t *testing.T) { + ctx := context.Background() + recs := []config.RecommendationRecord{{ID: "rec-1", Count: 1}} + cases := []struct { + name string + recs []config.RecommendationRecord + date time.Time + wantMoved bool + }{ + {"rec-bearing past date is moved", recs, time.Now().Add(-48 * time.Hour), true}, + {"rec-bearing future date is kept", recs, time.Now().Add(72 * time.Hour), false}, + {"empty row past date is kept", nil, time.Now().Add(-48 * time.Hour), false}, + } + for _, tc := range cases { + 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("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 + store.On("RescheduleAwaitingExecution", ctx, notifiedExecID, mock.Anything).Run(func(a mock.Arguments) { + d := a.Get(2).(time.Time) + movedTo = &d + }).Return(true, nil).Maybe() + + before := time.Now() + _, err := handler.resumePlannedPurchase(ctx, req, notifiedExecID) + require.NoError(t, err) + if !tc.wantMoved { + assert.Nil(t, movedTo) + return + } + require.NotNil(t, movedTo) + assert.WithinDuration(t, before.AddDate(0, 0, 3), *movedTo, time.Minute) + store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + }) + } +} + +func TestCancelOrRecoverExecution_ReleasesSuppressionsOfNotifiedRow(t *testing.T) { + ctx := context.Background() + handler, store, _ := notifiedAdminHandler(ctx) + store.On("TransitionExecutionStatus", ctx, notifiedExecID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything). + Return(&config.PurchaseExecution{ExecutionID: notifiedExecID, Status: "canceled"}, nil) + store.On("DeleteSuppressionsByExecution", ctx, notifiedExecID).Return(nil) + + _, err := handler.cancelOrRecoverExecution(ctx, notifiedExecID, nil) + require.NoError(t, err) + store.AssertCalled(t, "DeleteSuppressionsByExecution", ctx, notifiedExecID) +} diff --git a/internal/api/handler_purchases_pause_claim_integration_test.go b/internal/api/handler_purchases_pause_claim_integration_test.go index c5d3ff00..2619b391 100644 --- a/internal/api/handler_purchases_pause_claim_integration_test.go +++ b/internal/api/handler_purchases_pause_claim_integration_test.go @@ -254,7 +254,7 @@ func TestPauseClaim_PausedQueueResumesExactlyOnce(t *testing.T) { func TestPauseClaim_RejectsNonPendingStates(t *testing.T) { f := newPauseClaimFixture(t) - for _, status := range []string{"running", "completed", "canceled", "failed", "approved", "notified", "scheduled", "paused"} { + for _, status := range []string{"running", "completed", "canceled", "failed", "approved", "scheduled", "paused"} { t.Run(status, func(t *testing.T) { f.execution.Status = status require.NoError(t, f.store.SavePurchaseExecution(f.ctx, f.execution)) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index fef11890..b169cbbc 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1511,7 +1511,7 @@ func TestHandler_pausePlannedPurchase(t *testing.T) { paused := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "paused"} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending"}, "paused", mock.Anything).Return(paused, nil) + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "notified"}, "paused", mock.Anything).Return(paused, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1538,7 +1538,7 @@ func TestHandler_pausePlannedPurchase_NotFound(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending"}, "paused", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) + mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "notified"}, "paused", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1718,7 +1718,7 @@ func TestHandler_deletePlannedPurchase(t *testing.T) { canceled := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "canceled"} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "paused"}, "canceled", mock.Anything).Return(canceled, nil) + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(canceled, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1763,7 +1763,7 @@ func TestHandler_deletePlannedPurchase_DisablesPlan(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything).Return(canceled, nil) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(canceled, nil) mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) // Assert that UpdatePurchasePlan is called with enabled=false. mockStore.On("UpdatePurchasePlan", ctx, mock.MatchedBy(func(p *config.PurchasePlan) bool { @@ -1815,7 +1815,7 @@ func TestHandler_deletePlannedPurchase_AlreadyDisabledPlan(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything).Return(canceled, nil) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(canceled, nil) mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1866,7 +1866,7 @@ func TestHandler_deletePlannedPurchase_ConflictRetryDisablesPlan(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything).Return(nil, conflictErr) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(nil, conflictErr) mockStore.On("GetExecutionByID", ctx, execID).Return(existingExec, nil) mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) mockStore.On("UpdatePurchasePlan", ctx, mock.MatchedBy(func(p *config.PurchasePlan) bool { @@ -1917,7 +1917,7 @@ func TestHandler_deletePlannedPurchase_ConflictRetryAlreadyDisabled(t *testing.T mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything).Return(nil, conflictErr) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(nil, conflictErr) mockStore.On("GetExecutionByID", ctx, execID).Return(existingExec, nil) mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) // UpdatePurchasePlan is intentionally NOT registered; AssertExpectations @@ -1963,7 +1963,7 @@ func TestHandler_deletePlannedPurchase_ConflictRetryRunningReturns409(t *testing mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything).Return(nil, conflictErr) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(nil, conflictErr) mockStore.On("GetExecutionByID", ctx, execID).Return(runningExec, nil) // GetPurchasePlan must NOT be called — AssertExpectations verifies this. @@ -2024,7 +2024,7 @@ func TestHandler_deletePlannedPurchase_ConflictRetryLegacyCanceledNormalizes(t * mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything).Return(nil, conflictErr) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(nil, conflictErr) mockStore.On("GetExecutionByID", ctx, execID).Return(existingExec, nil) mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil) mockStore.On("UpdatePurchasePlan", ctx, mock.MatchedBy(func(p *config.PurchasePlan) bool { @@ -2070,7 +2070,7 @@ func TestHandler_deletePlannedPurchase_BackendErrorReturns5xx(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything).Return(nil, dbErr) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(nil, dbErr) // On a real backend error we must NOT fall into the recovery path: // GetExecutionByID must not be called. AssertExpectations verifies this. @@ -2100,7 +2100,7 @@ func TestHandler_pausePlannedPurchase_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{"pending"}, "paused", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) + mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "notified"}, "paused", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) handler := &Handler{config: mockStore, auth: mockAuth} @@ -2137,7 +2137,7 @@ func TestHandler_pausePlannedPurchase_IneligibleStatus(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending"}, "paused", mock.Anything). + mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "notified"}, "paused", mock.Anything). Return(nil, fmt.Errorf("%w: execution 11111111-1111-1111-1111-111111111111 cannot transition from %q to %q", config.ErrExecutionNotInExpectedStatus, status, "paused")) @@ -2260,7 +2260,7 @@ func TestHandler_deletePlannedPurchase_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{"pending", "paused"}, "canceled", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) + mockStore.On("TransitionExecutionStatus", ctx, "99999999-9999-9999-9999-999999999999", []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(nil, fmt.Errorf("execution not found: 99999999-9999-9999-9999-999999999999")) handler := &Handler{config: mockStore, auth: mockAuth} @@ -2293,7 +2293,7 @@ func TestHandler_pausePlannedPurchase_ActorStamped(t *testing.T) { paused := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "paused"} // Actor must equal the session UserID (pointer value comparison via reflect.DeepEqual). mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", - []string{"pending"}, "paused", + []string{"pending", "notified"}, "paused", mock.MatchedBy(func(a *string) bool { return a != nil && *a == actorID }), ).Return(paused, nil) @@ -5272,7 +5272,7 @@ func TestHandler_deletePlannedPurchase_NonOwner_Rejected(t *testing.T) { func TestHandler_deletePlannedPurchase_CancelOwn_Creator_Allowed(t *testing.T) { handler, mockConfig, _ := buildCancelOwnHandler(ownUserA, ownUserA) canceled := &config.PurchaseExecution{ExecutionID: ownExecID, PlanID: "", Status: "canceled"} - mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "paused"}, "canceled", mock.Anything). + mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything). Return(canceled, nil) result, err := handler.deletePlannedPurchase(context.Background(), manageReq(), ownExecID) @@ -5329,7 +5329,7 @@ func TestHandler_deletePlannedPurchase_UserAPIKey_Allowed(t *testing.T) { mockStore.On("GetExecutionByID", ctx, execID).Return(execution, nil) canceled := &config.PurchaseExecution{ExecutionID: execID, Status: "canceled"} - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything). + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything). Return(canceled, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -5356,20 +5356,20 @@ func TestHandler_runPlannedPurchase_NonOwner_Rejected(t *testing.T) { // TestHandler_pausePlannedPurchase_Owner_Allowed: user A manages their OWN P1. func TestHandler_pausePlannedPurchase_Owner_Allowed(t *testing.T) { handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserA, false) - mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending"}, "paused", mock.Anything). + mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "notified"}, "paused", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: ownExecID, Status: "paused"}, nil) res, err := handler.pausePlannedPurchase(context.Background(), manageReq(), ownExecID) require.NoError(t, err) assert.Equal(t, "paused", res.Status) - mockConfig.AssertCalled(t, "TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending"}, "paused", mock.Anything) + mockConfig.AssertCalled(t, "TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "notified"}, "paused", mock.Anything) } // TestHandler_pausePlannedPurchase_UpdateAny_AllowsAny: a privileged user with // update-any:purchases manages P2 created by user B. func TestHandler_pausePlannedPurchase_UpdateAny_AllowsAny(t *testing.T) { handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserB, true) - mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending"}, "paused", mock.Anything). + mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "notified"}, "paused", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: ownExecID, Status: "paused"}, nil) res, err := handler.pausePlannedPurchase(context.Background(), manageReq(), ownExecID) diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 561928d9..4e75b3e2 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -1214,7 +1214,7 @@ func TestHandler_HandleRequest_PausePlannedPurchase(t *testing.T) { mockAuth.On("ValidateCSRFToken", ctx, mock.Anything, mock.Anything).Return(nil) paused := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "paused"} - mockStore.On("TransitionExecutionStatus", mock.Anything, "11111111-1111-1111-1111-111111111111", []string{"pending"}, "paused", mock.Anything).Return(paused, nil) + mockStore.On("TransitionExecutionStatus", mock.Anything, "11111111-1111-1111-1111-111111111111", []string{"pending", "notified"}, "paused", mock.Anything).Return(paused, nil) handler := &Handler{config: mockStore, auth: mockAuth, corsAllowedOrigin: "*", apiKey: "test-key"} @@ -1330,7 +1330,7 @@ func TestHandler_HandleRequest_DeletePlannedPurchase(t *testing.T) { mockAuth.On("ValidateCSRFToken", ctx, mock.Anything, mock.Anything).Return(nil) canceled := &config.PurchaseExecution{ExecutionID: "11111111-1111-1111-1111-111111111111", Status: "canceled"} - mockStore.On("TransitionExecutionStatus", mock.Anything, "11111111-1111-1111-1111-111111111111", []string{"pending", "paused"}, "canceled", mock.Anything).Return(canceled, nil) + mockStore.On("TransitionExecutionStatus", mock.Anything, "11111111-1111-1111-1111-111111111111", []string{"pending", "notified", "paused"}, "canceled", mock.Anything).Return(canceled, nil) handler := &Handler{config: mockStore, auth: mockAuth, corsAllowedOrigin: "*", apiKey: "test-key"} diff --git a/internal/api/router_660_permission_flips_test.go b/internal/api/router_660_permission_flips_test.go index 0971e058..fc23c0fe 100644 --- a/internal/api/router_660_permission_flips_test.go +++ b/internal/api/router_660_permission_flips_test.go @@ -262,7 +262,7 @@ func TestPausePlannedPurchase_PermissionGate(t *testing.T) { mockStore.On("GetExecutionByID", ctx, execID). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &creator}, nil) // TransitionExecutionStatus is called next; stub it. - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending"}, "paused", mock.Anything). + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified"}, "paused", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil) h := &Handler{auth: mockAuth, config: mockStore} @@ -300,7 +300,7 @@ func TestPausePlannedPurchase_PermissionGate(t *testing.T) { // authorizeExecutionManagement; only requireExecutionAccess fetches. mockStore.On("GetExecutionByID", ctx, execID). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil) - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending"}, "paused", mock.Anything). + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified"}, "paused", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil) h := &Handler{auth: mockAuth, config: mockStore} @@ -321,7 +321,7 @@ func TestPausePlannedPurchase_PermissionGate(t *testing.T) { // immediately without calling GetAllowedAccountsAPI or GetExecutionByID). mockAuth := authForAdmin(ctx, t) mockStore := new(MockConfigStore) - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending"}, "paused", mock.Anything). + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified"}, "paused", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil) h := &Handler{auth: mockAuth, config: mockStore} @@ -350,7 +350,7 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { mockStore := new(MockConfigStore) mockStore.On("GetExecutionByID", ctx, execID). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &creator}, nil) - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything). + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "canceled"}, nil) h := &Handler{auth: mockAuth, config: mockStore} @@ -403,7 +403,7 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { mockStore := new(MockConfigStore) mockStore.On("GetExecutionByID", ctx, execID). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &creator}, nil) - mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "canceled", mock.Anything). + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified", "paused"}, "canceled", mock.Anything). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "canceled"}, nil) h := &Handler{auth: mockAuth, config: mockStore} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index d5fd9f35..3ff9b53a 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -191,6 +191,11 @@ type StoreInterface interface { // row whose recommendations were attached after the caller's gate check. A // lost pin returns ErrExecutionNotInExpectedStatus like any lost CAS. TransitionExecutionStatusPinned(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string, sawEmptyRecommendations bool) (*PurchaseExecution, error) + // RescheduleAwaitingExecution sets scheduled_date on an execution that is + // still pending or notified and touches no other column. It returns false + // (and writes nothing) when the row has left those statuses, so a cancel or + // approve that lands first is never overwritten. + RescheduleAwaitingExecution(ctx context.Context, executionID string, scheduledDate time.Time) (bool, error) // SetCancelledBy stamps canceled_by and the legacy cancelled_by column on // an execution without overwriting any other columns. Used after // TransitionExecutionStatus to fold the actor attribution into the same diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index f4709516..8ccb29de 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1213,6 +1213,18 @@ func (s *PostgresStore) TransitionExecutionStatusPinned(ctx context.Context, exe return s.transitionExecutionStatus(ctx, executionID, fromStatuses, toStatus, actor, &sawEmptyRecommendations) } +// RescheduleAwaitingExecution is a conditional single-column update: the +// status predicate makes a lost race a benign no-op instead of an overwrite. +func (s *PostgresStore) RescheduleAwaitingExecution(ctx context.Context, executionID string, scheduledDate time.Time) (bool, error) { + tag, err := s.db.Exec(ctx, ` + UPDATE purchase_executions SET scheduled_date = $2, updated_at = NOW() + WHERE execution_id = $1 AND status IN ('pending', 'notified')`, executionID, scheduledDate) + if err != nil { + return false, fmt.Errorf("reschedule execution %s: %w", executionID, err) + } + return tag.RowsAffected() == 1, nil +} + func (s *PostgresStore) transitionExecutionStatus(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string, sawEmpty *bool) (*PurchaseExecution, error) { var cancelActor *string if toStatus == StatusCanceled { diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index e72e4bfa..4c13b32a 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -419,6 +419,12 @@ func (m *MockConfigStore) TransitionExecutionStatusPinned(ctx context.Context, e return v, args.Error(1) } +// RescheduleAwaitingExecution mocks the conditional scheduled_date update. +func (m *MockConfigStore) RescheduleAwaitingExecution(ctx context.Context, executionID string, scheduledDate time.Time) (bool, error) { + args := m.Called(ctx, executionID, scheduledDate) + return args.Bool(0), args.Error(1) +} + // SetCancelledBy mocks the SetCancelledBy targeted-update operation. func (m *MockConfigStore) SetCancelledBy(ctx context.Context, executionID, cancelledBy string) error { m.record("SetCancelledBy", ctx, executionID, cancelledBy) diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 9c8cb9d5..c574d1cb 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -352,7 +352,7 @@ func (m *Manager) DirectExecute(ctx context.Context, executionID, actor string, // The returned row carries the run-now audit stamp and its final status, also // on an execution error; it is nil when the run never started. func (m *Manager) RunPlannedPurchaseNow(ctx context.Context, executionID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { - return m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "paused"}, runNowSkipReason) + return m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "notified", "paused"}, runNowSkipReason) } // transitionApproveAndExecute is the shared body behind ApproveAndExecute diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index a4cb793f..82292375 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -360,7 +360,7 @@ func TestManager_RunPlannedPurchaseNow_ExecutesFromPaused(t *testing.T) { StepNumber: 1, Recommendations: approvalTestRecs(), } - store.On("TransitionExecutionStatus", ctx, "exec-run-now", []string{"pending", "paused"}, "approved", (*string)(nil)).Return(updated, nil) + store.On("TransitionExecutionStatus", ctx, "exec-run-now", []string{"pending", "notified", "paused"}, "approved", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-run-now") final, revocationToken, err := manager.RunPlannedPurchaseNow(ctx, "exec-run-now", "operator@example.com", nil) @@ -387,7 +387,7 @@ func TestManager_RunPlannedPurchaseNow_LostCASReturnsError(t *testing.T) { ctx := context.Background() manager, store, _ := newApproveManager(t) - store.On("TransitionExecutionStatus", ctx, "exec-race", []string{"pending", "paused"}, "approved", (*string)(nil)). + store.On("TransitionExecutionStatus", ctx, "exec-race", []string{"pending", "notified", "paused"}, "approved", (*string)(nil)). Return(nil, config.ErrExecutionNotInExpectedStatus) final, _, err := manager.RunPlannedPurchaseNow(ctx, "exec-race", "operator@example.com", nil) diff --git a/internal/purchase/notified_run_now_test.go b/internal/purchase/notified_run_now_test.go new file mode 100644 index 00000000..23b08007 --- /dev/null +++ b/internal/purchase/notified_run_now_test.go @@ -0,0 +1,83 @@ +package purchase + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Fixture rows (mock store and mock provider): nothing attaches +// recommendations to a plan step yet, so notified rows are built by hand. +// +// Run-now on a notified row is a human action standing in for the approval, +// so it reaches executeAndFinalize and its guards exactly like approve does. +// The spend cap and the kill switch are not re-checked there (same as approve). + +var runNowFrom = []string{"pending", "notified", "paused"} + +func TestRunNow_NotifiedRowReachesArmedRedriveGuard(t *testing.T) { + exec := armedExecution("azure", "savingsplans", 1, "notified") + mgr, store, rec := armedHarness(t, common.ServiceSavingsPlansAll) + store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() + expectClaim(store, exec, runNowFrom, "approved") + + _, _, err := mgr.RunPlannedPurchaseNow(context.Background(), exec.ExecutionID, "operator@example.com", nil) + + assert.Zero(t, rec.count()) + require.Error(t, err) + assert.Contains(t, err.Error(), "refusing to execute retry attempt 1") +} + +func TestRunNow_NotifiedRowReachesStaleAzurePricingRefusal(t *testing.T) { + exec := armedExecution("azure", "compute", 0, "notified") + exec.Recommendations[0].ID = "azure-current" + mgr, store, rec := armedHarness(t, common.ServiceCompute) + repriced := exec.Recommendations[0] + repriced.UpfrontCost++ + store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}). + Return([]config.RecommendationRecord{repriced}, nil).Maybe() + store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() + expectClaim(store, exec, runNowFrom, "approved") + + _, _, err := mgr.RunPlannedPurchaseNow(context.Background(), exec.ExecutionID, "operator@example.com", nil) + + assert.Zero(t, rec.count()) + require.ErrorIs(t, err, ErrStaleAzurePricing) +} + +// The scheduler honors scheduled_date: a notified row whose date was pushed +// into the future is not claimed; once the date passes it is. +func TestScheduler_NotifiedRowRespectsPushedScheduledDate(t *testing.T) { + ctx := context.Background() + for _, tc := range []struct { + name string + date time.Time + wantClaim bool + }{ + {"inside the veto window", time.Now().Add(48 * time.Hour), false}, + {"after the window", time.Now().Add(-time.Minute), true}, + } { + t.Run(tc.name, func(t *testing.T) { + store, factory := new(MockConfigStore), new(MockProviderFactory) + exec := dueRecExec("notified", upfrontRec(10)) + exec.ScheduledDate = tc.date + manager := newPinManager(ctx, store, factory, exec, autoPlan(f64(1000)), autoPlan(f64(1000))) + store.On("TransitionExecutionStatusPinned", ctx, "exec-gate", mock.Anything, "running", (*string)(nil), false). + Return(nil, config.ErrExecutionNotInExpectedStatus).Maybe() + + _, err := manager.ProcessScheduledPurchases(ctx) + require.NoError(t, err) + if tc.wantClaim { + store.AssertCalled(t, "TransitionExecutionStatusPinned", ctx, "exec-gate", mock.Anything, "running", (*string)(nil), false) + } else { + store.AssertNotCalled(t, "TransitionExecutionStatusPinned", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + } + }) + } +} diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index ff395196..75e72562 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -158,6 +158,10 @@ func (m *mockConfigStoreForHealth) TransitionExecutionStatusPinned(ctx context.C return nil, nil } +func (m *mockConfigStoreForHealth) RescheduleAwaitingExecution(_ context.Context, _ string, _ time.Time) (bool, error) { + return false, nil +} + func (m *mockConfigStoreForHealth) SetCancelledBy(_ context.Context, _ string, _ string) error { return nil }