From 0ad90b7aebd67d348862edcc0d12e96b22a04942 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 05:42:52 +0000 Subject: [PATCH] fix(history): flag a failed execution class listing as truncated and expose the actionable cap fetchExecutionsByClass logged a listing error and skipped the class, so a failed actionable query hid every pending approval while the response kept truncated=false. The class fetch now reports failure and fetchExecutionsAsHistory raises the incomplete-results signal (including on the no-executions early return). HistoryResponse gains actionable_executions_limit (config.MaxListLimit) next to executions_limit (terminal cap, DefaultListLimit), so the UI banner can quote the cap that was actually hit instead of always quoting the terminal cap. The banner text itself lives in frontend/ and is left to the UI owner. Refs #686 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_history.go | 53 ++++++++++++------- ...dler_history_actionable_starvation_test.go | 28 ++++++++++ .../api/handler_history_truncation_test.go | 10 ++++ internal/api/openapi.yaml | 14 +++-- internal/api/types.go | 9 +++- 5 files changed, 90 insertions(+), 24 deletions(-) diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 40e9fb678..25a71fab6 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -85,11 +85,12 @@ func (h *Handler) getHistory(ctx context.Context, req *events.LambdaFunctionURLR // filters) because the cap drops rows before they run. It is a single // boolean: a count would reveal other tenants' row volume to a scoped user. return HistoryResponse{ - Summary: summarizePurchaseHistory(all), - Purchases: all, - Truncated: len(completed) >= filters.Limit || executionsCapped, - Limit: filters.Limit, - ExecutionsLimit: config.DefaultListLimit, + Summary: summarizePurchaseHistory(all), + Purchases: all, + Truncated: len(completed) >= filters.Limit || executionsCapped, + Limit: filters.Limit, + ExecutionsLimit: config.DefaultListLimit, + ActionableExecutionsLimit: config.MaxListLimit, }, nil } @@ -161,17 +162,14 @@ const approvalExpiryWindow = 7 * 24 * time.Hour // provider via the recs' collapsed provider, account via CloudAccountID, // date via ScheduledDate. // -// 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. +// The third return value reports that executions may be missing: either class +// reached its cap (actionable at MaxListLimit, terminal at DefaultListLimit) so +// older executions may have been dropped before the filters ran, or a class +// listing failed (issue #686). func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyFilters) ([]config.PurchaseHistoryRecord, []config.PurchaseExecution, bool) { - 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...) + executions, incomplete := h.fetchExecutionClasses(ctx) if len(executions) == 0 { - return nil, nil, false + return nil, nil, incomplete } approver := h.resolvePendingApproverEmail(ctx) userEmailCache := h.resolveUserEmails(ctx, executions) @@ -208,19 +206,36 @@ func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyF } out = append(out, executionToHistoryRow(exec, approver, createdByEmail)) } - capped := len(actionable) >= config.MaxListLimit || terminalSurfaced >= config.DefaultListLimit + capped := incomplete || terminalSurfaced >= config.DefaultListLimit return out, staleExecs, capped } +// fetchExecutionClasses lists the actionable and terminal classes, each with +// its own cap, and returns them concatenated (actionable first). incomplete +// reports that rows may be missing before the filters run: a class listing +// failed, or the actionable class reached its MaxListLimit cap. The terminal +// cap is judged by the caller on the rows it actually surfaces. +func (h *Handler) fetchExecutionClasses(ctx context.Context) ([]config.PurchaseExecution, bool) { + actionable, actionableOK := h.fetchExecutionsByClass(ctx, actionableExecutionStatuses, config.MaxListLimit) + terminal, terminalOK := h.fetchExecutionsByClass(ctx, terminalExecutionStatuses, config.DefaultListLimit) + executions := make([]config.PurchaseExecution, 0, len(actionable)+len(terminal)) + executions = append(executions, actionable...) + executions = append(executions, terminal...) + incomplete := !actionableOK || !terminalOK || len(actionable) >= config.MaxListLimit + return executions, incomplete +} + // 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 { +// the class skipped (ok=false) so the other class and completed history still +// render; the caller flags the response as incomplete so the hidden rows are +// not silent (issue #686). +func (h *Handler) fetchExecutionsByClass(ctx context.Context, statuses []string, limit int) ([]config.PurchaseExecution, bool) { 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 nil, false } - return rows + return rows, true } func isTerminalExecutionStatus(status string) bool { diff --git a/internal/api/handler_history_actionable_starvation_test.go b/internal/api/handler_history_actionable_starvation_test.go index 29befc3bd..114b08c06 100644 --- a/internal/api/handler_history_actionable_starvation_test.go +++ b/internal/api/handler_history_actionable_starvation_test.go @@ -126,10 +126,38 @@ func TestHandler_getHistory_ClassListingErrorKeepsOtherClass(t *testing.T) { purchases := body["purchases"].([]any) require.Len(t, purchases, 1) assert.Equal(t, tt.wantID, purchases[0].(map[string]any)["purchase_id"]) + assert.Equal(t, true, body["truncated"], "a failed class listing hides rows, so the response must flag incomplete results") }) } } +// TestHandler_getHistory_BothClassListingsFailSetTruncated covers the +// no-executions early return: when both class listings fail the response +// carries only completed history and must still flag incomplete results. +func TestHandler_getHistory_BothClassListingsFailSetTruncated(t *testing.T) { + ctx := context.Background() + store := newStarvationStore(ctx) + store.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(nil, errors.New("listing failed")) + + body := runStarvationHistory(t, ctx, store) + + assert.Empty(t, body["purchases"]) + assert.Equal(t, true, body["truncated"]) +} + +// TestHandler_getHistory_EmptyClassListingsNotTruncated guards the other side +// of the listing-error signal: two successful but empty listings are a +// complete result and must not raise the banner. +func TestHandler_getHistory_EmptyClassListingsNotTruncated(t *testing.T) { + ctx := context.Background() + store := newStarvationStore(ctx) + store.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{}, nil) + + body := runStarvationHistory(t, ctx, store) + + assert.Equal(t, false, body["truncated"]) +} + func TestHandler_getHistory_TruncatedPerClass(t *testing.T) { now := time.Now() tests := []struct { diff --git a/internal/api/handler_history_truncation_test.go b/internal/api/handler_history_truncation_test.go index 5e022713b..1f786d657 100644 --- a/internal/api/handler_history_truncation_test.go +++ b/internal/api/handler_history_truncation_test.go @@ -118,6 +118,16 @@ func TestHandler_getHistory_ExecutionsLimitIsFixed(t *testing.T) { assert.Equal(t, true, body["truncated"]) } +// The actionable class (pending, approved, ...) has its own, larger cap. The +// response must expose it so the truncation banner can quote the cap that was +// actually hit instead of the terminal-class cap (issue #686). +func TestHandler_getHistory_ActionableExecutionsLimitExposed(t *testing.T) { + body := runAdminHistory(t, nil, pendingExecutions(config.MaxListLimit), map[string]string{}) + assert.Equal(t, true, body["truncated"]) + assert.EqualValues(t, config.MaxListLimit, body["actionable_executions_limit"]) + assert.EqualValues(t, config.DefaultListLimit, body["executions_limit"]) +} + func TestHandler_getHistory_TruncatedHonorsCustomLimit(t *testing.T) { body := runAdminHistory(t, historyRows(5), nil, map[string]string{"limit": "5"}) assert.Equal(t, true, body["truncated"]) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index ba01d7f22..d1a84f4c1 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -4027,7 +4027,9 @@ components: 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. + running, ...) reached its own cap (`actionable_executions_limit`), + and when listing either execution class failed, so its rows are + missing. limit: type: integer description: Row cap on completed purchase history rows for this request. @@ -4035,8 +4037,14 @@ components: type: integer description: >- Row cap on terminal executions (failed, expired, canceled, ...), - fixed and ignoring the limit parameter. Actionable executions use a - separate cap of 1000. + fixed and ignoring the limit parameter. Actionable executions use + the separate `actionable_executions_limit` cap. + actionable_executions_limit: + type: integer + description: >- + Row cap on actionable executions (pending, notified, scheduled, + approved, running, paused), fixed at 1000 and ignoring the limit + parameter. HistorySummary: type: object diff --git a/internal/api/types.go b/internal/api/types.go index 9ff2a91f1..01dc8e3d9 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -959,15 +959,20 @@ 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 also fires when the actionable execution class hits its MaxListLimit cap. + // It also fires when the actionable execution class hits its MaxListLimit cap, + // and when an execution class listing failed, so its rows are missing. // 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 terminal executions (failed, expired, // canceled, ...), fixed regardless of the request's limit parameter. - // Actionable executions (pending, approved, ...) use config.MaxListLimit. + // Actionable executions (pending, approved, ...) use ActionableExecutionsLimit. ExecutionsLimit int `json:"executions_limit"` + // ActionableExecutionsLimit is the row cap on actionable executions + // (pending, notified, scheduled, approved, running, paused), fixed at + // config.MaxListLimit regardless of the request's limit parameter. + ActionableExecutionsLimit int `json:"actionable_executions_limit"` } // HistorySummary provides aggregate statistics for purchase history.