From fcb2fabb8dcdae97b06698dc42829e38222b56d1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 15:59:54 +0200 Subject: [PATCH 1/3] fix(planned-purchases): Pause is visible + reversible (badge, toast, Resume) After #779/#772 added the paused DB status, clicking Pause made the scheduled purchase silently disappear from the list with no feedback. The planned-purchases list read GetPendingExecutions, whose status set (pending, notified) the scheduler relies on to decide what to FIRE and which excludes paused, so paused rows dropped out. Switch the list handler to GetExecutionsByStatuses with [pending, notified, paused] so paused executions stay listed (with the existing Paused badge and Resume button) while the scheduler keeps using the narrower GetPendingExecutions and never fires paused rows. Restore the soonest-first ordering. Add success toasts on Pause and Resume. Pause stays distinct from Disable plan (#774, whole plan) and Cancel (terminal): it is reversible and scoped to a single execution; the plan stays enabled. Resume (paused -> pending) and the 409 on ineligible transitions already existed from #772. Tests: backend asserts the paused status set is requested, paused rows stay returned, and ordering is ascending; frontend asserts the paused row renders visibly with a badge and that Pause/Resume fire success toasts. --- frontend/src/__tests__/plans.test.ts | 11 ++++ frontend/src/plans.ts | 5 ++ internal/api/handler_purchases.go | 18 +++++- internal/api/handler_purchases_test.go | 82 ++++++++++++++++++++++++-- internal/api/handler_test.go | 2 +- 5 files changed, 111 insertions(+), 7 deletions(-) diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index 188017121..ee3c0f7fb 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -640,6 +640,11 @@ describe('Plans Module', () => { const container = document.getElementById('planned-purchases-list'); expect(container?.innerHTML).toContain('data-action="resume"'); expect(container?.innerHTML).toContain('data-action="run"'); + // Paused rows stay visible with a Paused badge and are NOT replaced by + // the empty-state message. + expect(container?.innerHTML).toContain('status-paused'); + expect(container?.innerHTML).toContain('>paused<'); + expect(container?.innerHTML).not.toContain('No planned purchases'); }); test('renders running purchase without pause/resume buttons', async () => { @@ -800,6 +805,9 @@ describe('Plans Module', () => { await new Promise(resolve => setTimeout(resolve, 50)); expect(api.pausePlannedPurchase).toHaveBeenCalledWith('purchase-1'); + // Pause must give visible feedback via a success toast. + expect(mockShowToast).toHaveBeenCalledWith( + expect.objectContaining({ message: 'Purchase paused', kind: 'success' })); }); test('disable action deletes planned purchase with confirmation', async () => { @@ -932,6 +940,9 @@ describe('Plans Module', () => { await new Promise(resolve => setTimeout(resolve, 50)); expect(api.resumePlannedPurchase).toHaveBeenCalledWith('purchase-1'); + // Resume must give visible feedback via a success toast. + expect(mockShowToast).toHaveBeenCalledWith( + expect.objectContaining({ message: 'Purchase resumed', kind: 'success' })); }); }); diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 45d11f855..5e2d0353f 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -232,10 +232,15 @@ async function handlePlannedPurchaseAction(action: string, purchaseId: string, p } break; case 'pause': + // Pause is reversible and scoped to a single execution: the plan stays + // enabled (unlike Disable plan) and the row stays listed with a Paused + // badge (unlike the old silent-removal behaviour). await api.pausePlannedPurchase(purchaseId); + showToast({ message: 'Purchase paused', kind: 'success', timeout: 5_000 }); break; case 'resume': await api.resumePlannedPurchase(purchaseId); + showToast({ message: 'Purchase resumed', kind: 'success', timeout: 5_000 }); break; case 'edit': // Open edit modal for the parent plan using plan_id, not the purchase id. diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index f7be43519..12d804d35 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -84,16 +84,30 @@ func buildSuppressions(recs []config.RecommendationRecord, executionID string, c return out } +// plannedListStatuses are the execution statuses surfaced in the Scheduled +// (Planned) Purchases list. It deliberately includes "paused" so a paused +// execution stays VISIBLE with its badge instead of silently dropping out of +// the list. It does NOT use GetPendingExecutions, which the +// scheduler relies on to decide what to FIRE: paused rows must be listed but +// never fired, so the two concerns use different status sets. +var plannedListStatuses = []string{"pending", "notified", "paused"} + func (h *Handler) getPlannedPurchases(ctx context.Context, req *events.LambdaFunctionURLRequest) (*PlannedPurchasesResponse, error) { session, err := h.requirePermission(ctx, req, "view", "purchases") if err != nil { return nil, err } - executions, err := h.config.GetPendingExecutions(ctx) + executions, err := h.config.GetExecutionsByStatuses(ctx, plannedListStatuses, config.MaxListLimit) if err != nil { - return nil, fmt.Errorf("failed to get pending executions: %w", err) + return nil, fmt.Errorf("failed to get planned executions: %w", err) } + // GetExecutionsByStatuses orders scheduled_date DESC; the planned list + // reads soonest-first, so restore the ascending order GetPendingExecutions + // used before paused rows were folded in. + sort.SliceStable(executions, func(i, j int) bool { + return executions[i].ScheduledDate.Before(executions[j].ScheduledDate) + }) plans, err := h.config.ListPurchasePlans(ctx, config.PurchasePlanFilter{}) if err != nil { diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index e7681a438..dc348b95c 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -751,7 +751,12 @@ func TestHandler_getPlannedPurchases(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockStore.On("GetPendingExecutions", ctx).Return(executions, nil) + // The planned list must request paused executions alongside pending/notified + // so a paused row stays VISIBLE. Assert the status set + // explicitly rather than mock.Anything to lock the invariant. + mockStore.On("GetExecutionsByStatuses", ctx, + []string{"pending", "notified", "paused"}, config.MaxListLimit). + Return(executions, nil) mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -763,6 +768,7 @@ func TestHandler_getPlannedPurchases(t *testing.T) { } result, err := handler.getPlannedPurchases(ctx, req) require.NoError(t, err) + mockStore.AssertExpectations(t) assert.Len(t, result.Purchases, 1) assert.Equal(t, "11111111-1111-1111-1111-111111111111", result.Purchases[0].ID) @@ -791,7 +797,8 @@ func TestHandler_getPlannedPurchases_ErrorGettingExecutions(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockStore.On("GetPendingExecutions", ctx).Return(nil, errors.New("database error")) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). + Return(nil, errors.New("database error")) handler := &Handler{config: mockStore, auth: mockAuth} @@ -803,7 +810,74 @@ func TestHandler_getPlannedPurchases_ErrorGettingExecutions(t *testing.T) { result, err := handler.getPlannedPurchases(ctx, req) assert.Error(t, err) assert.Nil(t, result) - assert.Contains(t, err.Error(), "failed to get pending executions") + assert.Contains(t, err.Error(), "failed to get planned executions") +} + +// TestHandler_getPlannedPurchases_PausedStaysVisible is a regression guard: +// a paused execution must remain in the list (not silently disappear), and +// rows stay ordered soonest-first regardless of the store's native DESC ordering. +func TestHandler_getPlannedPurchases_PausedStaysVisible(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + Role: "admin", + } + + soon := time.Now().AddDate(0, 0, 3) + later := time.Now().AddDate(0, 0, 10) + // Returned newest-first by the store (DESC); the handler must re-sort ASC. + executions := []config.PurchaseExecution{ + { + ExecutionID: "22222222-2222-2222-2222-222222222222", + PlanID: "11111111-1111-1111-1111-111111111111", + Status: "paused", + ScheduledDate: later, + StepNumber: 2, + }, + { + ExecutionID: "33333333-3333-3333-3333-333333333333", + PlanID: "11111111-1111-1111-1111-111111111111", + Status: "pending", + ScheduledDate: soon, + StepNumber: 1, + }, + } + plans := []config.PurchasePlan{ + { + ID: "11111111-1111-1111-1111-111111111111", + Name: "Test Plan", + Services: map[string]config.ServiceConfig{"aws/rds": {Provider: "aws", Service: "rds", Term: 3, Payment: "no-upfront"}}, + RampSchedule: config.RampSchedule{TotalSteps: 5}, + }, + } + + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockStore.On("GetExecutionsByStatuses", ctx, + []string{"pending", "notified", "paused"}, config.MaxListLimit). + Return(executions, nil) + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}} + + result, err := handler.getPlannedPurchases(ctx, req) + require.NoError(t, err) + mockStore.AssertExpectations(t) + + require.Len(t, result.Purchases, 2) + // Soonest-first ordering. + assert.Equal(t, "pending", result.Purchases[0].Status) + assert.Equal(t, "paused", result.Purchases[1].Status) + // The paused row is present, not dropped. + var statuses []string + for _, p := range result.Purchases { + statuses = append(statuses, p.Status) + } + assert.Contains(t, statuses, "paused") } func TestHandler_pausePlannedPurchase(t *testing.T) { @@ -1311,7 +1385,7 @@ func TestHandler_getPlannedPurchases_ErrorGettingPlans(t *testing.T) { executions := []config.PurchaseExecution{{ExecutionID: "11111111-1111-1111-1111-111111111111", PlanID: "11111111-1111-1111-1111-111111111111"}} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockStore.On("GetPendingExecutions", ctx).Return(executions, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(executions, nil) mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(nil, errors.New("database error")) handler := &Handler{config: mockStore, auth: mockAuth} diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 3eedae9ba..8d7c8cb08 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -1021,7 +1021,7 @@ func TestHandler_HandleRequest_GetPlannedPurchases(t *testing.T) { {ID: "11111111-1111-1111-1111-111111111111", Name: "Test Plan", Services: map[string]config.ServiceConfig{"aws/rds": {Provider: "aws", Service: "rds"}}}, } - mockStore.On("GetPendingExecutions", ctx).Return(executions, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(executions, nil) mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) handler := &Handler{config: mockStore, auth: mockAuth, corsAllowedOrigin: "*", apiKey: "test-key"} From 027eea011e38137e293ce82229e55cc8af9c8187 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 18:05:37 +0200 Subject: [PATCH 2/3] feat(config/store): add GetPlannedExecutions with ASC ordering GetExecutionsByStatuses uses ORDER BY scheduled_date DESC + LIMIT, which is correct for History (newest-first) but truncates the soonest rows when used for the Planned Purchases list and total pending/notified/paused rows exceed MaxListLimit. An in-memory ASC re-sort of the already-truncated subset cannot recover what LIMIT dropped at the DB. Add a dedicated GetPlannedExecutions method that mirrors the GetExecutionsByStatuses shape (same status filter, same limit clamping, same scan logic via queryExecutions) but flips the ORDER BY to scheduled_date ASC NULLS LAST, id ASC. The secondary id ASC sort keeps the ordering stable when multiple rows share a scheduled_date; NULLS LAST is defensive against a future schema relaxation (today scheduled_date is NOT NULL). GetExecutionsByStatuses is left untouched so its other callers (History queries that want newest-first) keep their semantics. pgxmock regression coverage anchors the SQL contract: - TestPGXMock_GetPlannedExecutions_UsesASCOrdering asserts the query has ASC + NULLS LAST + id ASC + LIMIT $2 via a strict regex matcher, so a regression to DESC or a dropped secondary sort fails the test. - TestPGXMock_GetPlannedExecutions_EmptyStatuses guards the short-circuit. - TestPGXMock_GetPlannedExecutions_LimitClamping covers the negative -> DefaultListLimit and over-MaxListLimit -> MaxListLimit clamps. All store mocks (api, purchase, scheduler, analytics, server health) gain a GetPlannedExecutions method so the StoreInterface contract is satisfied across the codebase. Refs CR on #904. --- internal/analytics/collector_test.go | 4 + internal/api/mocks_test.go | 8 ++ internal/config/interfaces.go | 9 ++ internal/config/store_postgres.go | 39 ++++++++ .../config/store_postgres_pgxmock_test.go | 89 +++++++++++++++++++ internal/purchase/mocks_test.go | 8 ++ internal/scheduler/scheduler_test.go | 8 ++ internal/server/test_helpers_test.go | 4 + 8 files changed, 169 insertions(+) diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 8ed9b29ae..9068a43f1 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -169,6 +169,10 @@ func (m *mockConfigStore) GetExecutionsByStatuses(ctx context.Context, statuses return nil, nil } +func (m *mockConfigStore) GetPlannedExecutions(ctx context.Context, statuses []string, limit int) ([]config.PurchaseExecution, error) { + return nil, nil +} + func (m *mockConfigStore) GetStaleApprovedExecutions(ctx context.Context, olderThan time.Duration) ([]config.PurchaseExecution, error) { return nil, nil } diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index 8fb7a1249..c759b08fe 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -167,6 +167,14 @@ func (m *MockConfigStore) GetExecutionsByStatuses(ctx context.Context, statuses return args.Get(0).([]config.PurchaseExecution), args.Error(1) } +func (m *MockConfigStore) GetPlannedExecutions(ctx context.Context, statuses []string, limit int) ([]config.PurchaseExecution, error) { + args := m.Called(ctx, statuses, limit) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]config.PurchaseExecution), args.Error(1) +} + func (m *MockConfigStore) GetStaleApprovedExecutions(ctx context.Context, olderThan time.Duration) ([]config.PurchaseExecution, error) { args := m.Called(ctx, olderThan) if args.Get(0) == nil { diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 457eeef19..72c179e56 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -41,6 +41,15 @@ type StoreInterface interface { // this method's status filter) to avoid accidental double-processing of // failed / expired rows. GetExecutionsByStatuses(ctx context.Context, statuses []string, limit int) ([]PurchaseExecution, error) + // GetPlannedExecutions returns executions in any of the given states + // ordered by scheduled_date ASC (soonest first), the order the Planned + // Purchases UI lists rows so the user acts on imminent purchases first. + // Distinct from GetExecutionsByStatuses (which is DESC for History's + // "newest first" semantics): when the result set exceeds `limit`, an + // ORDER-BY-DESC + LIMIT in SQL truncates away the soonest rows, exactly + // the rows this list must surface. Secondary sort by id ASC stabilises + // ordering when multiple rows share a scheduled_date. + GetPlannedExecutions(ctx context.Context, statuses []string, limit int) ([]PurchaseExecution, error) // GetStaleApprovedExecutions returns executions stuck in the "approved" // status with updated_at older than olderThan — strands left behind when a // synchronous purchase run was interrupted before finalizing (issue #632). diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 6905f4105..0df9e89df 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -922,6 +922,45 @@ func (s *PostgresStore) GetExecutionsByStatuses(ctx context.Context, statuses [] return s.queryExecutions(ctx, query, statuses, limit) } +// GetPlannedExecutions returns executions whose Status is any of the supplied +// values, ordered by scheduled_date ASC (soonest first), capped at `limit`. +// Used by the Planned Purchases handler where the user expects to act on +// imminent rows first. +// +// Distinct from GetExecutionsByStatuses (DESC + LIMIT for History's +// newest-first semantics): when total rows exceed `limit`, a DESC truncation +// drops the soonest rows, exactly the ones this list must surface. Sorting +// the already-truncated subset in-memory cannot recover them. +// +// Secondary sort by id ASC keeps ordering stable when multiple rows share a +// scheduled_date. NULLS LAST is defensive: the schema makes scheduled_date +// NOT NULL today, but the clause guards against a future relaxation silently +// hiding rows at the top of the list. +func (s *PostgresStore) GetPlannedExecutions(ctx context.Context, statuses []string, limit int) ([]PurchaseExecution, error) { + if len(statuses) == 0 { + return nil, nil + } + if limit <= 0 { + limit = DefaultListLimit + } + if limit > MaxListLimit { + limit = MaxListLimit + } + query := ` + SELECT plan_id, execution_id, status, step_number, scheduled_date, + notification_sent, approval_token, recommendations, + total_upfront_cost, estimated_savings, completed_at, error, expires_at, + cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + created_by_user_id, retry_execution_id, retry_attempt_n, + approval_token_expires_at + FROM purchase_executions + WHERE status = ANY($1) + ORDER BY scheduled_date ASC NULLS LAST, id ASC + LIMIT $2 + ` + return s.queryExecutions(ctx, query, statuses, limit) +} + // GetStaleApprovedExecutions returns executions stuck in the "approved" status // whose last update is older than olderThan. These are executions that were // flipped to "approved" by ApproveAndExecute but whose synchronous purchase run diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index f48076627..c9ccc8d63 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -1553,6 +1553,95 @@ func TestPGXMock_ListPendingExecutionIDsForAccount_Empty(t *testing.T) { require.NoError(t, mock.ExpectationsWereMet()) } +// ─── GetPlannedExecutions ──────────────────────────────────────────────────── + +// TestPGXMock_GetPlannedExecutions_UsesASCOrdering is the regression guard for +// the planned-purchases list truncation bug. GetExecutionsByStatuses uses +// ORDER BY scheduled_date DESC + LIMIT, which drops the SOONEST rows when the +// pending/notified/paused set exceeds MaxListLimit, exactly the rows the UI +// must surface. GetPlannedExecutions must order ASC at the DB level so LIMIT +// keeps the soonest rows. pgxmock's regexp matcher fails the test if the SQL +// uses DESC, regresses to the GetExecutionsByStatuses query, or drops the +// stable secondary sort. +func TestPGXMock_GetPlannedExecutions_UsesASCOrdering(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + now := time.Now().Truncate(time.Second) + soon := now.Add(2 * time.Hour) + later := now.Add(48 * time.Hour) + rows := pgxmock.NewRows(stuckExecCols()). + AddRow(stuckExecRow("exec-soon", "pending", soon)...). + AddRow(stuckExecRow("exec-later", "paused", later)...) + // Strict regex anchors: + // 1. ASC ordering on scheduled_date (NOT DESC), + // 2. NULLS LAST guard so a future-relaxed schema can't hide rows, + // 3. id ASC secondary sort for stable ordering at equal scheduled_date, + // 4. LIMIT $2 so callers can bound result size. + // If a future refactor regresses to DESC or drops the secondary sort, the + // expectation goes unmet and the test fails. + mock.ExpectQuery(`(?s)SELECT.*FROM purchase_executions.*status = ANY\(\$1\).*ORDER BY scheduled_date ASC NULLS LAST, id ASC.*LIMIT \$2`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()). + WillReturnRows(rows) + + execs, err := store.GetPlannedExecutions(ctx, []string{"pending", "notified", "paused"}, 100) + require.NoError(t, err) + require.Len(t, execs, 2) + assert.Equal(t, "exec-soon", execs[0].ExecutionID) + assert.Equal(t, "exec-later", execs[1].ExecutionID) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_GetPlannedExecutions_EmptyStatuses guards the short-circuit: +// nil/empty status list returns nil with no SQL roundtrip (pgxmock fails on +// any unexpected query since none is registered here). +func TestPGXMock_GetPlannedExecutions_EmptyStatuses(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + execs, err := store.GetPlannedExecutions(ctx, nil, 100) + require.NoError(t, err) + assert.Nil(t, execs) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_GetPlannedExecutions_LimitClamping asserts limit <= 0 falls +// back to DefaultListLimit and limit > MaxListLimit is clamped to MaxListLimit. +// Mirrors GetExecutionsByStatuses' clamping so callers can pass user-supplied +// values without sanitizing upstream. +func TestPGXMock_GetPlannedExecutions_LimitClamping(t *testing.T) { + t.Run("negative falls back to DefaultListLimit", func(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + rows := pgxmock.NewRows(stuckExecCols()) + mock.ExpectQuery(`ORDER BY scheduled_date ASC`). + WithArgs(pgxmock.AnyArg(), DefaultListLimit). + WillReturnRows(rows) + + _, err := store.GetPlannedExecutions(ctx, []string{"pending"}, -1) + require.NoError(t, err) + assert.NoError(t, mock.ExpectationsWereMet()) + }) + t.Run("over-max clamped to MaxListLimit", func(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + rows := pgxmock.NewRows(stuckExecCols()) + mock.ExpectQuery(`ORDER BY scheduled_date ASC`). + WithArgs(pgxmock.AnyArg(), MaxListLimit). + WillReturnRows(rows) + + _, err := store.GetPlannedExecutions(ctx, []string{"pending"}, MaxListLimit+5000) + require.NoError(t, err) + assert.NoError(t, mock.ExpectationsWereMet()) + }) +} + // ─── ListStuckExecutions ───────────────────────────────────────────────────── // stuckExecRow builds a pgxmock row that matches the queryExecutions scan diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index e39bf8f0c..b34ebfcdb 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -265,6 +265,14 @@ func (m *MockConfigStore) GetExecutionsByStatuses(ctx context.Context, statuses return args.Get(0).([]config.PurchaseExecution), args.Error(1) } +func (m *MockConfigStore) GetPlannedExecutions(ctx context.Context, statuses []string, limit int) ([]config.PurchaseExecution, error) { + args := m.Called(ctx, statuses, limit) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]config.PurchaseExecution), args.Error(1) +} + func (m *MockConfigStore) GetStaleApprovedExecutions(ctx context.Context, olderThan time.Duration) ([]config.PurchaseExecution, error) { args := m.Called(ctx, olderThan) if args.Get(0) == nil { diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index bb827162f..a292ad516 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -140,6 +140,14 @@ func (m *MockConfigStore) GetExecutionsByStatuses(ctx context.Context, statuses return args.Get(0).([]config.PurchaseExecution), args.Error(1) } +func (m *MockConfigStore) GetPlannedExecutions(ctx context.Context, statuses []string, limit int) ([]config.PurchaseExecution, error) { + args := m.Called(ctx, statuses, limit) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]config.PurchaseExecution), args.Error(1) +} + func (m *MockConfigStore) GetStaleApprovedExecutions(ctx context.Context, olderThan time.Duration) ([]config.PurchaseExecution, error) { args := m.Called(ctx, olderThan) if args.Get(0) == nil { diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 913476803..ddd51abe7 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -71,6 +71,10 @@ func (m *mockConfigStoreForHealth) GetExecutionsByStatuses(ctx context.Context, return nil, nil } +func (m *mockConfigStoreForHealth) GetPlannedExecutions(ctx context.Context, statuses []string, limit int) ([]config.PurchaseExecution, error) { + return nil, nil +} + func (m *mockConfigStoreForHealth) GetStaleApprovedExecutions(ctx context.Context, olderThan time.Duration) ([]config.PurchaseExecution, error) { return nil, nil } From d5aef84182f3e38299d6005639e9aab500d05956 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 18:05:47 +0200 Subject: [PATCH 3/3] fix(api/purchases): use GetPlannedExecutions to avoid truncating soonest rows Handler.getPlannedPurchases was calling GetExecutionsByStatuses (ORDER BY scheduled_date DESC + LIMIT $2) and then re-sorting ASC in-memory. When the planned set (pending + notified + paused) exceeds MaxListLimit, the DB returns only the LATEST rows; the in-memory sort just re-orders that already-truncated subset. The soonest rows, exactly the ones the user has to act on, can be omitted from the response entirely. Switch the handler to the new GetPlannedExecutions store method (ASC at the SQL level so LIMIT keeps the soonest rows), and drop the post-fetch sort.SliceStable since the DB now returns rows in the correct order. The "sort" import stays (still used for tuple normalisation elsewhere in the file). End-to-end regression coverage: - TestHandler_getPlannedPurchases_SoonestRowsNotTruncated seeds 5 rows in ASC order, asserts all 5 reach the response in ASC order, and uses AssertNotCalled to guard against any future refactor re-introducing a parallel GetExecutionsByStatuses call on this code path. - TestHandler_getPlannedPurchases_PausedStaysVisible updated to reflect the new ASC-from-store contract (was returning DESC then re-sorting). Closes CR finding on #904. --- internal/api/handler_purchases.go | 13 ++- internal/api/handler_purchases_test.go | 109 +++++++++++++++++++++---- internal/api/handler_test.go | 2 +- 3 files changed, 102 insertions(+), 22 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 12d804d35..3e6f88058 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -98,16 +98,15 @@ func (h *Handler) getPlannedPurchases(ctx context.Context, req *events.LambdaFun return nil, err } - executions, err := h.config.GetExecutionsByStatuses(ctx, plannedListStatuses, config.MaxListLimit) + // GetPlannedExecutions orders scheduled_date ASC at the DB level so the + // soonest-first list isn't truncated when total rows exceed MaxListLimit. + // (GetExecutionsByStatuses uses DESC + LIMIT for History; mixing them here + // drops the genuinely-soonest rows, exactly the rows this list must show. + // An in-memory re-sort cannot recover what LIMIT already discarded.) + executions, err := h.config.GetPlannedExecutions(ctx, plannedListStatuses, config.MaxListLimit) if err != nil { return nil, fmt.Errorf("failed to get planned executions: %w", err) } - // GetExecutionsByStatuses orders scheduled_date DESC; the planned list - // reads soonest-first, so restore the ascending order GetPendingExecutions - // used before paused rows were folded in. - sort.SliceStable(executions, func(i, j int) bool { - return executions[i].ScheduledDate.Before(executions[j].ScheduledDate) - }) plans, err := h.config.ListPurchasePlans(ctx, config.PurchasePlanFilter{}) if err != nil { diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index dc348b95c..09e61a918 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -754,7 +754,7 @@ func TestHandler_getPlannedPurchases(t *testing.T) { // The planned list must request paused executions alongside pending/notified // so a paused row stays VISIBLE. Assert the status set // explicitly rather than mock.Anything to lock the invariant. - mockStore.On("GetExecutionsByStatuses", ctx, + mockStore.On("GetPlannedExecutions", ctx, []string{"pending", "notified", "paused"}, config.MaxListLimit). Return(executions, nil) mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) @@ -797,7 +797,7 @@ func TestHandler_getPlannedPurchases_ErrorGettingExecutions(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). + mockStore.On("GetPlannedExecutions", ctx, mock.Anything, mock.Anything). Return(nil, errors.New("database error")) handler := &Handler{config: mockStore, auth: mockAuth} @@ -814,8 +814,8 @@ func TestHandler_getPlannedPurchases_ErrorGettingExecutions(t *testing.T) { } // TestHandler_getPlannedPurchases_PausedStaysVisible is a regression guard: -// a paused execution must remain in the list (not silently disappear), and -// rows stay ordered soonest-first regardless of the store's native DESC ordering. +// a paused execution must remain in the list (not silently disappear) and +// rows must be ordered soonest-first end-to-end. func TestHandler_getPlannedPurchases_PausedStaysVisible(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -829,15 +829,9 @@ func TestHandler_getPlannedPurchases_PausedStaysVisible(t *testing.T) { soon := time.Now().AddDate(0, 0, 3) later := time.Now().AddDate(0, 0, 10) - // Returned newest-first by the store (DESC); the handler must re-sort ASC. + // GetPlannedExecutions returns rows soonest-first (ASC); the handler + // preserves that order with no in-memory re-sort. executions := []config.PurchaseExecution{ - { - ExecutionID: "22222222-2222-2222-2222-222222222222", - PlanID: "11111111-1111-1111-1111-111111111111", - Status: "paused", - ScheduledDate: later, - StepNumber: 2, - }, { ExecutionID: "33333333-3333-3333-3333-333333333333", PlanID: "11111111-1111-1111-1111-111111111111", @@ -845,6 +839,13 @@ func TestHandler_getPlannedPurchases_PausedStaysVisible(t *testing.T) { ScheduledDate: soon, StepNumber: 1, }, + { + ExecutionID: "22222222-2222-2222-2222-222222222222", + PlanID: "11111111-1111-1111-1111-111111111111", + Status: "paused", + ScheduledDate: later, + StepNumber: 2, + }, } plans := []config.PurchasePlan{ { @@ -856,7 +857,7 @@ func TestHandler_getPlannedPurchases_PausedStaysVisible(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockStore.On("GetExecutionsByStatuses", ctx, + mockStore.On("GetPlannedExecutions", ctx, []string{"pending", "notified", "paused"}, config.MaxListLimit). Return(executions, nil) mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) @@ -880,6 +881,86 @@ func TestHandler_getPlannedPurchases_PausedStaysVisible(t *testing.T) { assert.Contains(t, statuses, "paused") } +// TestHandler_getPlannedPurchases_SoonestRowsNotTruncated is the end-to-end +// regression guard for CodeRabbit's truncation finding on PR #904: when total +// pending/notified/paused rows exceed MaxListLimit, the store's DESC + LIMIT +// drops the soonest rows entirely, and an in-memory ASC re-sort of the +// already-truncated subset cannot recover them. +// +// The fix routes the handler through GetPlannedExecutions (ASC + LIMIT) and +// removes the post-fetch in-memory sort. This test: +// - registers a mock expectation on GetPlannedExecutions and asserts +// GetExecutionsByStatuses is NOT called (regressing to the DESC path +// would surface here), +// - returns rows in ASC order (mimicking what the real SQL would produce) +// and asserts the handler preserves that order without re-shuffling, +// - asserts all 5 rows survive (none are dropped by the handler itself). +func TestHandler_getPlannedPurchases_SoonestRowsNotTruncated(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + Role: "admin", + } + + now := time.Now() + // 5 rows, soonest first, the order the store will return them when + // ORDER BY scheduled_date ASC is used (the fix). Pre-fix code called + // GetExecutionsByStatuses (DESC) and re-sorted in-memory, but the + // regression scenario is that the DB has truncated the soonest rows + // away before the in-memory sort sees them. + soonest := []config.PurchaseExecution{ + {ExecutionID: "11111111-1111-1111-1111-111111111111", PlanID: "00000000-0000-0000-0000-000000000001", Status: "pending", ScheduledDate: now.AddDate(0, 0, 1), StepNumber: 1}, + {ExecutionID: "22222222-2222-2222-2222-222222222222", PlanID: "00000000-0000-0000-0000-000000000001", Status: "notified", ScheduledDate: now.AddDate(0, 0, 2), StepNumber: 2}, + {ExecutionID: "33333333-3333-3333-3333-333333333333", PlanID: "00000000-0000-0000-0000-000000000001", Status: "paused", ScheduledDate: now.AddDate(0, 0, 3), StepNumber: 3}, + {ExecutionID: "44444444-4444-4444-4444-444444444444", PlanID: "00000000-0000-0000-0000-000000000001", Status: "pending", ScheduledDate: now.AddDate(0, 0, 4), StepNumber: 4}, + {ExecutionID: "55555555-5555-5555-5555-555555555555", PlanID: "00000000-0000-0000-0000-000000000001", Status: "pending", ScheduledDate: now.AddDate(0, 0, 5), StepNumber: 5}, + } + plans := []config.PurchasePlan{ + { + ID: "00000000-0000-0000-0000-000000000001", + Name: "Test Plan", + Services: map[string]config.ServiceConfig{"aws/rds": {Provider: "aws", Service: "rds", Term: 3, Payment: "no-upfront"}}, + RampSchedule: config.RampSchedule{TotalSteps: 5}, + }, + } + + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + // Strict args lock the contract: planned statuses + MaxListLimit (so the + // DB receives the same cap the handler intends, no off-by-one budget). + mockStore.On("GetPlannedExecutions", ctx, + []string{"pending", "notified", "paused"}, config.MaxListLimit). + Return(soonest, nil) + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}} + + result, err := handler.getPlannedPurchases(ctx, req) + require.NoError(t, err) + mockStore.AssertExpectations(t) + // Regression anchor: the handler must NOT fall back to the DESC path. + // AssertNotCalled fails if some refactor re-introduces a parallel + // GetExecutionsByStatuses call on the planned list code path. + mockStore.AssertNotCalled(t, "GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything) + + require.Len(t, result.Purchases, 5, "all 5 rows must reach the response, none dropped by handler post-processing") + // Ordering preserved end-to-end: the store returns ASC, the handler must + // pass that through unchanged (the in-memory re-sort has been removed). + for i, expected := range []string{ + "11111111-1111-1111-1111-111111111111", + "22222222-2222-2222-2222-222222222222", + "33333333-3333-3333-3333-333333333333", + "44444444-4444-4444-4444-444444444444", + "55555555-5555-5555-5555-555555555555", + } { + assert.Equal(t, expected, result.Purchases[i].ID, "row %d (soonest-first) must be %s", i, expected) + } +} + func TestHandler_pausePlannedPurchase(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -1385,7 +1466,7 @@ func TestHandler_getPlannedPurchases_ErrorGettingPlans(t *testing.T) { executions := []config.PurchaseExecution{{ExecutionID: "11111111-1111-1111-1111-111111111111", PlanID: "11111111-1111-1111-1111-111111111111"}} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(executions, nil) + mockStore.On("GetPlannedExecutions", ctx, mock.Anything, mock.Anything).Return(executions, nil) mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(nil, errors.New("database error")) handler := &Handler{config: mockStore, auth: mockAuth} diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 8d7c8cb08..9c12174c7 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -1021,7 +1021,7 @@ func TestHandler_HandleRequest_GetPlannedPurchases(t *testing.T) { {ID: "11111111-1111-1111-1111-111111111111", Name: "Test Plan", Services: map[string]config.ServiceConfig{"aws/rds": {Provider: "aws", Service: "rds"}}}, } - mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(executions, nil) + mockStore.On("GetPlannedExecutions", ctx, mock.Anything, mock.Anything).Return(executions, nil) mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) handler := &Handler{config: mockStore, auth: mockAuth, corsAllowedOrigin: "*", apiKey: "test-key"}