fix(history): fetch actionable executions separately so terminal rows cannot starve them

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,24 +160,24 @@ 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
 	}
 	approver := h.resolvePendingApproverEmail(ctx)
 	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

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)
+}

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)

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

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"`
 }