diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index e1d8f999d..864938cdf 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -4,6 +4,7 @@ package api import ( "context" "fmt" + "slices" "sort" "strconv" "time" @@ -91,9 +92,11 @@ func (h *Handler) getHistory(ctx context.Context, req *events.LambdaFunctionURLR }, nil } -// historyExecutionStatuses enumerates the PurchaseExecution statuses the -// History view loads alongside completed purchases. Issue #372 assumed -// approval executes synchronously (approved -> completed/failed inside one +// actionableExecutionStatuses are the PurchaseExecution statuses a user can still +// act on or that are in flight. They are fetched separately from the terminal +// class so newer terminal rows cannot push them out of the capped listing. +// +// Issue #372 assumed approval executes synchronously (approved -> completed/failed inside one // HTTP request) and so excluded "approved"; issue #621 showed that // assumption breaks under interruption: a Lambda timeout or a crash mid- // execution leaves the row stuck in "approved"/"running"/"paused", in @@ -103,6 +106,15 @@ func (h *Handler) getHistory(ctx context.Context, req *events.LambdaFunctionURLR // states too, rendered with a clear "in progress" badge rather than as a // (misleading) completed row. // +// "scheduled" is included so Gmail-style pre-fire delayed executions (issue #291 +// wave-2) appear in the History view with a Revoke button before the cloud SDK +// call fires. Without this entry the row is invisible to the History UI, making +// the Revoke button unreachable (issue #290, second-wave CR Finding E). +var actionableExecutionStatuses = []string{"pending", "notified", "scheduled", "approved", "running", "paused"} + +// terminalExecutionStatuses are the finished PurchaseExecution statuses the +// History view loads alongside completed purchases. +// // "completed" is also loaded, but fetchExecutionsAsHistory synthesizes a // row for it ONLY when the execution carries a non-empty Error — the // audit-gap case where the purchase succeeded but its purchase_history @@ -118,16 +130,12 @@ func (h *Handler) getHistory(ctx context.Context, req *events.LambdaFunctionURLR // partial-failure marker and is flagged IsAuditGap so its execution-level // dollars are excluded from the dashboard totals — the committed dollars are // already counted via the per-rec purchase_history rows that succeeded. -// "scheduled" is included so Gmail-style pre-fire delayed executions (issue #291 -// wave-2) appear in the History view with a Revoke button before the cloud SDK -// call fires. Without this entry the row is invisible to the History UI, making -// the Revoke button unreachable (issue #290, second-wave CR Finding E). // Both the US-spelling status (config.StatusCanceled) and the legacy British // spelling (config.LegacyStatusCanceled) are listed: during the expand-contract // rename (migration 000089) old code may still write the legacy value before // the rolling deploy completes. The contract migration (#1278) normalizes the // data once the deploy is verified stable; drop the legacy entry here then. -var historyExecutionStatuses = []string{"pending", "notified", "scheduled", "approved", "running", "paused", "completed", "partially_completed", "failed", "expired", config.StatusCanceled, config.LegacyStatusCanceled} +var terminalExecutionStatuses = []string{"completed", "partially_completed", "failed", "expired", config.StatusCanceled, config.LegacyStatusCanceled} // approvalExpiryWindow is how long a pending approval stays actionable // before the History view flips it to "expired". Aligns with the @@ -152,14 +160,15 @@ const approvalExpiryWindow = 7 * 24 * time.Hour // provider via the recs' collapsed provider, account via CloudAccountID, // date via ScheduledDate. // -// The third return value reports that the fetch reached its cap, so older -// executions may have been dropped before the filters ran. +// The third return value reports that either class reached its cap (actionable +// at MaxListLimit, terminal at DefaultListLimit), so older executions may have +// been dropped before the filters ran. func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyFilters) ([]config.PurchaseHistoryRecord, []config.PurchaseExecution, bool) { - executions, err := h.config.GetExecutionsByStatuses(ctx, historyExecutionStatuses, config.DefaultListLimit) - if err != nil { - logging.Warnf("history: failed to load non-completed executions: %v", err) - return nil, nil, false - } + actionable := h.fetchExecutionsByClass(ctx, actionableExecutionStatuses, config.MaxListLimit) + terminal := h.fetchExecutionsByClass(ctx, terminalExecutionStatuses, config.DefaultListLimit) + executions := make([]config.PurchaseExecution, 0, len(actionable)+len(terminal)) + executions = append(executions, actionable...) + executions = append(executions, terminal...) if len(executions) == 0 { return nil, nil, false } @@ -167,9 +176,8 @@ func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyF userEmailCache := h.resolveUserEmails(ctx, executions) out := make([]config.PurchaseHistoryRecord, 0, len(executions)) var staleExecs []config.PurchaseExecution - // Rows the History view can surface; clean completed executions do not - // count toward the cap signal (the store already excludes them). - surfaced := 0 + // Terminal-class rows surfaced; drives the terminal cap signal. + terminalSurfaced := 0 for _rvc := range executions { exec := executions[_rvc] // Dedup: a normal completed execution is already represented by its @@ -180,7 +188,9 @@ func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyF if exec.Status == "completed" && exec.Error == "" { continue } - surfaced++ + if isTerminalExecutionStatus(exec.Status) { + terminalSurfaced++ + } // Collect stale pending/notified executions for the post-assembly // expire sweep. We do NOT mutate status here to keep the GET // read-only: the response reflects current DB state; the sweep @@ -197,7 +207,23 @@ func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyF } out = append(out, executionToHistoryRow(exec, approver, createdByEmail)) } - return out, staleExecs, surfaced >= config.DefaultListLimit + capped := len(actionable) >= config.MaxListLimit || terminalSurfaced >= config.DefaultListLimit + return out, staleExecs, capped +} + +// fetchExecutionsByClass lists one status class. A listing error is logged and +// the class skipped so the other class and completed history still render. +func (h *Handler) fetchExecutionsByClass(ctx context.Context, statuses []string, limit int) []config.PurchaseExecution { + rows, err := h.config.GetExecutionsByStatuses(ctx, statuses, limit) + if err != nil { + logging.Warnf("history: failed to load executions with statuses %v: %v", statuses, err) + return nil + } + return rows +} + +func isTerminalExecutionStatus(status string) bool { + return slices.Contains(terminalExecutionStatuses, status) } // isStaleExecution reports whether the execution is a pending/notified @@ -235,6 +261,10 @@ func (h *Handler) expireStaleExecutions(staleExecs []config.PurchaseExecution) { if len(staleExecs) == 0 { return } + if len(staleExecs) > config.DefaultListLimit { + logging.Warnf("history: sweeping %d of %d stale executions", config.DefaultListLimit, len(staleExecs)) + staleExecs = staleExecs[:config.DefaultListLimit] + } if runtime.IsLambda() { h.expireStaleExecutionsSweep(staleExecs) return diff --git a/internal/api/handler_history_actionable_starvation_test.go b/internal/api/handler_history_actionable_starvation_test.go new file mode 100644 index 000000000..5a9e7a028 --- /dev/null +++ b/internal/api/handler_history_actionable_starvation_test.go @@ -0,0 +1,171 @@ +package api + +import ( + "context" + "errors" + "fmt" + "slices" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func executionsWithStatus(n int, status string, scheduled time.Time, idPrefix string) []config.PurchaseExecution { + execs := make([]config.PurchaseExecution, n) + for i := range execs { + execs[i] = config.PurchaseExecution{ + ExecutionID: fmt.Sprintf("%s-%d", idPrefix, i), + Status: status, + ScheduledDate: scheduled, + } + } + return execs +} + +func isActionableCall(statuses []string) bool { + return slices.Equal(statuses, actionableExecutionStatuses) +} + +func isTerminalCall(statuses []string) bool { + return slices.Equal(statuses, terminalExecutionStatuses) +} + +func newStarvationStore(ctx context.Context) *MockConfigStore { + store := new(MockConfigStore) + store.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + store.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil).Maybe() + return store +} + +func runStarvationHistory(t *testing.T, ctx context.Context, store *MockConfigStore) map[string]any { + t.Helper() + mockAuth, req := adminHistoryReq(ctx) + handler := &Handler{auth: mockAuth, config: store} + result, err := handler.getHistory(ctx, req, map[string]string{}) + require.NoError(t, err) + return historyJSON(t, result) +} + +func TestHandler_getHistory_PendingNotStarvedByNewerTerminalRows(t *testing.T) { + ctx := context.Background() + now := time.Now() + execs := executionsWithStatus(150, "failed", now, "failed") + pending := executionsWithStatus(1, "pending", now.Add(-time.Hour), "pending") + store := newStarvationStore(ctx) + store.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(append(execs, pending...), nil) + + body := runStarvationHistory(t, ctx, store) + + ids := map[string]bool{} + for _, p := range body["purchases"].([]any) { + ids[p.(map[string]any)["purchase_id"].(string)] = true + } + assert.True(t, ids["pending-0"], "older pending row must survive 150 newer failed rows") + summary := body["summary"].(map[string]any) + assert.EqualValues(t, 1, summary["total_pending"]) +} + +func TestHandler_getHistory_ReturnsMoreThanDefaultLimitPendingRows(t *testing.T) { + ctx := context.Background() + now := time.Now() + pending := executionsWithStatus(config.DefaultListLimit+50, "pending", now, "pending") + store := newStarvationStore(ctx) + store.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(pending, nil) + + body := runStarvationHistory(t, ctx, store) + + assert.Len(t, body["purchases"], config.DefaultListLimit+50) + assert.EqualValues(t, config.DefaultListLimit+50, body["summary"].(map[string]any)["total_pending"]) + assert.Equal(t, false, body["truncated"]) +} + +func TestHandler_getHistory_FetchesDisjointClassesWithTheirLimits(t *testing.T) { + ctx := context.Background() + store := newStarvationStore(ctx) + store.On("GetExecutionsByStatuses", ctx, mock.MatchedBy(isActionableCall), config.MaxListLimit).Return([]config.PurchaseExecution{}, nil).Once() + store.On("GetExecutionsByStatuses", ctx, mock.MatchedBy(isTerminalCall), config.DefaultListLimit).Return([]config.PurchaseExecution{}, nil).Once() + + runStarvationHistory(t, ctx, store) + + store.AssertNumberOfCalls(t, "GetExecutionsByStatuses", 2) + for _, s := range actionableExecutionStatuses { + assert.NotContains(t, terminalExecutionStatuses, s, "classes must be disjoint") + } +} + +func TestHandler_getHistory_ClassListingErrorKeepsOtherClass(t *testing.T) { + now := time.Now() + tests := []struct { + name string + failActive bool + wantID string + }{ + {"terminal error keeps actionable", false, "pending-0"}, + {"actionable error keeps terminal", true, "failed-0"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + store := newStarvationStore(ctx) + actionableRet := mock.Arguments{executionsWithStatus(1, "pending", now, "pending"), nil} + terminalRet := mock.Arguments{executionsWithStatus(1, "failed", now, "failed"), nil} + if tt.failActive { + actionableRet = mock.Arguments{nil, errors.New("actionable listing failed")} + } else { + terminalRet = mock.Arguments{nil, errors.New("terminal listing failed")} + } + store.On("GetExecutionsByStatuses", ctx, mock.MatchedBy(isActionableCall), mock.Anything).Return(actionableRet...) + store.On("GetExecutionsByStatuses", ctx, mock.MatchedBy(isTerminalCall), mock.Anything).Return(terminalRet...) + + body := runStarvationHistory(t, ctx, store) + + purchases := body["purchases"].([]any) + require.Len(t, purchases, 1) + assert.Equal(t, tt.wantID, purchases[0].(map[string]any)["purchase_id"]) + }) + } +} + +func TestHandler_getHistory_TruncatedPerClass(t *testing.T) { + now := time.Now() + tests := []struct { + name string + actionable int + terminal int + wantTruncated bool + }{ + {"terminal class at cap", 0, config.DefaultListLimit, true}, + {"both under cap", config.DefaultListLimit, config.DefaultListLimit - 1, false}, + {"actionable class at cap", config.MaxListLimit, 0, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + rows := append(executionsWithStatus(tt.actionable, "pending", now, "pending"), + executionsWithStatus(tt.terminal, "failed", now, "failed")...) + store := newStarvationStore(ctx) + store.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(rows, nil) + + body := runStarvationHistory(t, ctx, store) + + assert.Equal(t, tt.wantTruncated, body["truncated"]) + }) + } +} + +func TestHandler_getHistory_StaleSweepBoundedToDefaultListLimit(t *testing.T) { + t.Setenv("AWS_LAMBDA_RUNTIME_API", "127.0.0.1:9001") + ctx := context.Background() + stale := executionsWithStatus(150, "pending", time.Now().Add(-8*24*time.Hour), "stale") + store := newStarvationStore(ctx) + store.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(stale, nil) + store.On("TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, "expired", mock.Anything).Return(nil, nil) + + runStarvationHistory(t, ctx, store) + + store.AssertNumberOfCalls(t, "TransitionExecutionStatus", config.DefaultListLimit) +} diff --git a/internal/api/handler_history_truncation_test.go b/internal/api/handler_history_truncation_test.go index 4fe6dfc6c..5e022713b 100644 --- a/internal/api/handler_history_truncation_test.go +++ b/internal/api/handler_history_truncation_test.go @@ -69,10 +69,18 @@ func TestHandler_getHistory_TruncatedWhenCompletedAtCap(t *testing.T) { func TestHandler_getHistory_TruncatedWhenExecutionsAtCap(t *testing.T) { // The execution fetch is capped before the in-memory filters run, so a // full page means pending approvals may have been dropped. - body := runAdminHistory(t, nil, pendingExecutions(config.DefaultListLimit), map[string]string{}) + body := runAdminHistory(t, nil, failedExecutionsAtCap(), map[string]string{}) assert.Equal(t, true, body["truncated"]) } +func failedExecutionsAtCap() []config.PurchaseExecution { + execs := pendingExecutions(config.DefaultListLimit) + for i := range execs { + execs[i].Status = "failed" + } + return execs +} + func cleanCompletedExecutions(n int) []config.PurchaseExecution { execs := pendingExecutions(n) for i := range execs { @@ -90,11 +98,7 @@ func TestHandler_getHistory_NotTruncatedByCleanCompletedExecutions(t *testing.T) } func TestHandler_getHistory_TruncatedByFailedExecutions(t *testing.T) { - execs := pendingExecutions(config.DefaultListLimit) - for i := range execs { - execs[i].Status = "failed" - } - body := runAdminHistory(t, nil, execs, map[string]string{}) + body := runAdminHistory(t, nil, failedExecutionsAtCap(), map[string]string{}) assert.Equal(t, true, body["truncated"]) } @@ -108,7 +112,7 @@ func TestHandler_getHistory_AuditGapCompletedExecutionsCountTowardCap(t *testing } func TestHandler_getHistory_ExecutionsLimitIsFixed(t *testing.T) { - body := runAdminHistory(t, historyRows(2), pendingExecutions(config.DefaultListLimit), map[string]string{"limit": "500"}) + body := runAdminHistory(t, historyRows(2), failedExecutionsAtCap(), map[string]string{"limit": "500"}) assert.EqualValues(t, 500, body["limit"]) assert.EqualValues(t, config.DefaultListLimit, body["executions_limit"]) assert.Equal(t, true, body["truncated"]) @@ -133,7 +137,7 @@ func TestHandler_getHistory_ScopedUserTruncationLeaksNoCounts(t *testing.T) { ctx := context.Background() const scopedUserID = "scoped-user-id" own := scopedUserID - execs := pendingExecutions(config.DefaultListLimit) + execs := failedExecutionsAtCap() execs[0].CreatedByUserID = &own mockStore := new(MockConfigStore) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index d8f21b21e..2432c167c 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3494,12 +3494,17 @@ components: True when a fetch reached its row cap, so older rows (and possibly pending approvals) may be missing from purchases and summary. May be a false positive when exactly `limit` or `executions_limit` rows exist. + Also true when the actionable executions class (pending, approved, + running, ...) reached its own cap of 1000 rows. limit: type: integer description: Row cap on completed purchase history rows for this request. executions_limit: type: integer - description: Row cap on non-completed executions (fixed, ignores the limit parameter). + description: >- + Row cap on terminal executions (failed, expired, canceled, ...), + fixed and ignoring the limit parameter. Actionable executions use a + separate cap of 1000. HistorySummary: type: object diff --git a/internal/api/types.go b/internal/api/types.go index 5ec84b3d4..d851bbb40 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -930,12 +930,14 @@ type HistoryResponse struct { Summary HistorySummary `json:"summary"` // Truncated is true when a fetch reached its row cap, so older rows (and // possibly pending approvals) may be missing from Purchases and Summary. - // It can be a false positive when exactly Limit (or ExecutionsLimit) rows exist. + // It also fires when the actionable execution class hits its MaxListLimit cap. + // It can be a false positive when exactly a cap's worth of rows exist. Truncated bool `json:"truncated"` // Limit is the row cap on completed purchase_history rows for this request. Limit int `json:"limit"` - // ExecutionsLimit is the row cap on non-completed executions, which is - // fixed regardless of the request's limit parameter. + // ExecutionsLimit is the row cap on terminal executions (failed, expired, + // canceled, ...), fixed regardless of the request's limit parameter. + // Actionable executions (pending, approved, ...) use config.MaxListLimit. ExecutionsLimit int `json:"executions_limit"` } diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index a7caa7a01..ba42adbb4 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -3,6 +3,8 @@ package mocks import ( "context" "fmt" + "slices" + "sort" "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -1578,7 +1580,32 @@ func (m *MockConfigStore) GetExecutionsByStatuses(ctx context.Context, statuses if !ok { panic(fmt.Sprintf("mock: expected []config.PurchaseExecution, got %T", args.Get(0))) } - return v, args.Error(1) + return filterExecutionsLikeStore(v, statuses, limit), args.Error(1) +} + +// filterExecutionsLikeStore applies the store contract to canned rows: filter by +// status, drop clean completed rows, order by ScheduledDate DESC, then cap. +func filterExecutionsLikeStore(rows []config.PurchaseExecution, statuses []string, limit int) []config.PurchaseExecution { + if len(statuses) == 0 { + return nil + } + if limit <= 0 { + limit = config.DefaultListLimit + } + limit = min(limit, config.MaxListLimit) + var out []config.PurchaseExecution + for i := range rows { + r := &rows[i] + if !slices.Contains(statuses, r.Status) || (r.Status == "completed" && r.Error == "") { + continue + } + out = append(out, *r) + } + sort.SliceStable(out, func(i, j int) bool { return out[i].ScheduledDate.After(out[j].ScheduledDate) }) + if len(out) > limit { + out = out[:limit] + } + return out } // CountExecutionsByPlanAndStatus mocks the CountExecutionsByPlanAndStatus operation.