Skip to content

fix(history): flag a failed execution class listing as truncated and expose the actionable cap - #1276

Merged
IAmJSD merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-686
Oct 25, 2026
Merged

IAmJSD merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-686

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

Backend half of #686. Both gaps were left by #632 PR1 (#684).

Root cause

  1. A failed class listing was silent. fetchExecutionsByClass (internal/api/handler_history.go) logged a GetExecutionsByStatuses error and returned nil. fetchExecutionsAsHistory only derived its cap signal from row counts, and its len(executions) == 0 early return hard-coded false. So if the actionable query failed, every pending approval disappeared from /api/history while truncated stayed false.
  2. Only the terminal cap was in the response. HistoryResponse exposed executions_limit (config.DefaultListLimit, 100), which since fix(history): fetch actionable executions separately so terminal rows cannot starve them #684 caps only the terminal class. The actionable class (pending, notified, scheduled, approved, running, paused) has its own cap, config.MaxListLimit (1000), and the API never returned it. That left the UI banner nothing to quote except 100.

Fix

  • fetchExecutionsByClass now returns (rows, ok). A new helper, fetchExecutionClasses, fetches both classes and returns incomplete = actionable listing failed || terminal listing failed || actionable class at MaxListLimit. fetchExecutionsAsHistory ORs that into its cap signal, including on the no-executions early return. The result is truncated=true whenever a class listing errored, as the issue suggests. Completed history and the other class still render as before. Splitting out the helper keeps fetchExecutionsAsHistory at gocyclo 10.
  • HistoryResponse gains actionable_executions_limit (config.MaxListLimit), next to limit and executions_limit. This is the "or the API exposes both caps" option from the issue. truncated is still a single boolean. No per-class flag or count is added, so a scoped user learns nothing more about other tenants' row volume than before.
  • openapi.yaml and the HistoryResponse doc comments now describe the new field and the listing-error case.

Refs #686

This uses Refs, not Closes: the banner text the user sees is rendered in frontend/src/history.ts, which this PR does not change. The issue should stay open until the frontend follow-up below lands.

Verification

Unit tests with mock stores only. This is not a DB-level bug, so I added no integration test.

New and changed tests in internal/api:

  • TestHandler_getHistory_ClassListingErrorKeepsOtherClass: each subtest now also asserts truncated == true.
  • TestHandler_getHistory_BothClassListingsFailSetTruncated (new): covers the early-return path when both listings fail.
  • TestHandler_getHistory_EmptyClassListingsNotTruncated (new): two successful but empty listings must not raise the banner.
  • TestHandler_getHistory_ActionableExecutionsLimitExposed (new): actionable_executions_limit == MaxListLimit, executions_limit == DefaultListLimit, and truncated is set at the actionable cap.

Fail-before, run on the unfixed handler:

go test ./internal/api/ -count=1 -run 'TestHandler_getHistory_(ClassListingErrorKeepsOtherClass|BothClassListingsFailSetTruncated|EmptyClassListingsNotTruncated|ActionableExecutionsLimitExposed)$'
--- FAIL: TestHandler_getHistory_ClassListingErrorKeepsOtherClass   (both subtests: truncated false)
--- FAIL: TestHandler_getHistory_BothClassListingsFailSetTruncated
--- FAIL: TestHandler_getHistory_ActionableExecutionsLimitExposed
FAIL

Pass-after: the same command passes all 4 tests (including both subtests).

Mutation probes, each reverted afterwards. Every one is caught:

  • dropping !actionableOK from incomplete fails ClassListingErrorKeepsOtherClass/actionable_error_keeps_terminal
  • dropping !terminalOK fails ClassListingErrorKeepsOtherClass/terminal_error_keeps_actionable
  • returning false on the no-executions early return fails BothClassListingsFailSetTruncated

Cheap checks, run on this branch with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true:

go build ./...                                  # ok
go vet ./...                                    # ok
go vet -tags integration ./...                  # ok
go test ./internal/api/ -count=1                # ok
go test ./internal/mocks/ -count=1              # ok
golangci-lint run --allow-parallel-runners ./internal/api/...   # 0 issues (v2.10.1)
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_history.go internal/api/types.go   # no output

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 936996ad = origin/main a14dc8f2 plus these heads, merged in this order:

Branch Tested head
fix/issue-337 ff3420c
fix/issue-169 7d75eb5
fix/issue-342 66b221f
fix/issue-310 5e11c21
fix/issue-326 5f0ea59
fix/issue-243 365e959
fix/issue-343 6c94036
fix/issue-312 d2848a8
fix/issue-316 acd60dd
fix/issue-339 75ea9b9
fix/issue-339-commitment-type 73bdb19
fix/issue-353 5d25fcd
fix/issue-313 3156bc4
fix/issue-357 53d9e58
fix/issue-368 0ba727d
fix/issue-419 61648c9
fix/issue-111 b04c372
fix/issue-686 0ad90b7
fix/issue-674 793095d
fix/issue-1055 4ec96f1
fix/issue-559 dfa9525
fix/issue-558 034eed4
fix/issue-874 1720539
fix/issue-666 1e120da
fix/issue-668 fb26345
fix/issue-568 ce04173
fix/issue-311 e54f250
fix/issue-177 0cbb88a
fix/issue-151 c21377c
fix/issue-166 41fb77b
fix/issue-27 2d67622
fix/issue-323 69288ae
fix/issue-59 36463e8
fix/issue-43 35e4069
fix/issue-158 6f76f8a
fix/issue-104 bb00491
fix/issue-55 971ef1b
fix/issue-361 89ac220
fix/issue-120 b458719
fix/issue-36 42bac6e
fix/issue-525 57c97dd
fix/issue-712 e5bcda0
fix/issue-72 6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:

  • Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
  • Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
  • Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
    • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
    • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
    • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
  • What the combined run caught, each fixed on its own branch before this PR opened:
    • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
    • fix/issue-316 rejecting legacy-keyed plans;
    • fix/issue-120 versus main's global-filter test.
  • Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
  • Nothing was applied to any cloud.

Out of scope

  • No frontend changes (the UI owner owns frontend/). See below.
  • truncated is reused for the listing-error case, as the issue proposes, rather than adding a separate incomplete flag. The banner's "capped at" wording is therefore slightly off when the cause is an error rather than a cap. The follow-up below covers this.
  • Overlap check: open PR sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first #1242 and branch fix/issue-532 change handler_purchases.go and other files, not handler_history.go or types.go. fix/issue-532 also edits openapi.yaml, but in different hunks (around lines 640 and 760; this PR edits about line 4030).

Frontend follow-up for the UI owner

frontend/src/history.ts renderTruncationNotices (around line 398) currently renders:

Results are capped at the most recent ${limit} completed purchases and ${executions_limit} pending, failed or other non-completed executions. ...

Suggested change, which could be folded into the frontend PR the issue mentions:

  1. Add actionable_executions_limit?: number; to the history response type in frontend/src/types.ts (next to executions_limit, around line 206).
  2. Quote both execution caps, for example: "Results are capped at the most recent ${limit} completed purchases, ${actionable_executions_limit} pending or in-progress executions and ${executions_limit} failed or other finished executions. Older rows are not shown, so pending approvals and totals may be incomplete. Narrow the date range or filters to see them." Optionally, soften "capped at" to "may be incomplete", since truncated is now also set when an execution listing fails.
  3. Update frontend/src/__tests__/history-truncation-notice.test.ts mocks to include actionable_executions_limit: 1000, and assert that the banner quotes 1000 for the actionable class.

Labels

Maintainer: please mirror the issue's triage labels onto this PR: triaged, priority/p3, severity/low, urgency/eventually, impact/few, effort/s, type/bug.

🤖 Generated with Claude Code

…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 LeanerCloud#686

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience effort/s Hours type/bug Defect labels Oct 23, 2026
@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Bring the branch up to date with main.

Conflicted files:
- internal/api/handler_history.go: main (LeanerCloud#1200) replaced
  summarizePurchaseHistory(all) in the getHistory response with the
  scope-aware SQL summary plus SummaryComplete; the branch added
  ActionableExecutionsLimit to the same literal. Kept both: main's
  Summary/SummaryComplete and the branch's ActionableExecutionsLimit.
- internal/api/types.go: both sides rewrote the Truncated doc comment.
  Combined them: main's note that Summary still counts rows beyond the
  caps while SummaryComplete is true, and the branch's note that a failed
  execution class listing also sets Truncated.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Merged origin/main (2514c01) into this branch. New head: ee67abd. A trial merge with the current main (c1f1781) is clean and passes go vet -tags integration ./....

Conflict resolution:

  • internal/api/handler_history.go: main (feat(api): scope-aware SQL totals for the History summary cards #1200) replaced summarizePurchaseHistory(all) in the getHistory response with the scope-aware SQL summary and added SummaryComplete. This branch added ActionableExecutionsLimit to the same struct literal. I kept both: main's Summary and SummaryComplete, plus this branch's ActionableExecutionsLimit.
  • internal/api/types.go: both sides rewrote the Truncated doc comment, and I combined them. Main's note says Summary still counts rows beyond the caps while SummaryComplete is true. This branch's note says a failed execution class listing also sets Truncated.

openapi.yaml merged without conflicts. Its truncated description now covers both points.

Checks on the merged head:

  • build, vet (also with -tags integration) and e2e vet pass.
  • golangci-lint found 0 issues with and without the integration tag, and gocyclo is silent.
  • go test -race -short ./internal/api passes.
  • go test -race -tags integration ./internal/api passes. The first run on the loaded shared VM failed on a Postgres container startup timeout, and the rerun passed.
  • The secrets scan is clean and the planted key was flagged. The allowlist test passes.

#1277, which is stacked on this branch, now merges this exact head, so the two resolutions are the same.

IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 25, 2026
This branch is stacked on fix/issue-686 (LeanerCloud#1276). That branch was brought
up to date with origin/main in ee67abd, so merging it here brings in
main and the LeanerCloud#686 conflict resolutions in one step.

Conflicted files:
- internal/api/main_nonet_test.go, internal/api/main_integration_test.go:
  both sides added a teardown hook list for the shared Postgres server
  (this branch: afterAllTests; main LeanerCloud#1400: afterTests). Took main's files
  unchanged; this branch only needed the hook, which main now provides.
- internal/api/handler_history.go, getHistory: main (LeanerCloud#1200) loads the
  account scope once (loadHistoryScope), filters rows with scope.filter,
  defers the stale-approval sweep until after the summary aggregate, and
  returns the scope-aware SQL summary with SummaryComplete. This branch
  pages the stream (fetchHistoryPage) and lists the actionable class only
  on the first page. Combined: the page is fetched first, the scope is
  loaded once and applied to the actionable rows, staleExecs is declared
  outside the Cursor == nil branch so the sweep stays deferred (and does
  not run on cursor pages), and the response carries main's
  Summary/SummaryComplete plus this branch's paging fields. The summary is
  now whole-scope on every page.
- internal/api/handler_history.go, fetchPurchaseHistory: main replaced the
  GetAllPurchaseHistory/GetPurchaseHistoryFiltered split with the shared
  historyFilters.purchaseHistoryFilter; this branch reads one keyset batch
  through ListPurchaseHistoryPage. Kept both: purchaseHistoryFilter is
  kept (the summary uses it) and the batch read now starts from it with
  Limit set to the batch size.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE ee67abd598ddc7a3d10d20392487f4350bd76ced

Blocking:

  • none

Non-blocking:

  • internal/api/handler_history.go fetchExecutionClasses: truncated now covers two causes, a cap being hit and a listing failing. The frontend banner still says "capped at", so until the feat(frontend): Load more on the History view, Approval Queue from all loaded pages #676 follow-up lands, a failed actionable listing shows a cap message rather than an error. Refs #686 (not Closes) is the right call for that reason.
  • actionable_executions_limit is a constant (1000) on every response. That's fine, it only gives the banner a number to quote.

Attacked:

  • Read the whole non-test diff. incomplete = !actionableOK || !terminalOK || len(actionable) >= MaxListLimit is ORed into capped, including on the early return when there are no executions, so every path that drops execution rows now raises truncated. Two successful empty listings stay false (tested). truncated is still a single boolean, so a scoped user learns no per-class counts. Nothing on the summary or scope paths changed.
  • Ran go build ./... and go vet ./internal/api/ (with and without -tags integration): ok. go test -race -short -count=1 ./internal/api/: ok. git merge-tree with current origin/main: clean.
  • Fail-before: I restored main's handler_history.go (kept types.go so it compiles). ClassListingErrorKeepsOtherClass (both subtests), BothClassListingsFailSetTruncated and ActionableExecutionsLimitExposed failed. EmptyClassListingsNotTruncated passed as expected, since it is the negative control. Restored afterwards.

Body-stale:

  • none

Reviewer: rB11. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at ee67abd598ddc7a3d10d20392487f4350bd76ced merged onto main (51957bcf), together with #1100, #1108, #1147, #1149, #1276, #1292 in that order (combined tree bc575de3, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 25, 2026 20:54
@IAmJSD
IAmJSD merged commit a0be4aa into LeanerCloud:main Oct 25, 2026
36 checks passed
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on ee67abd5, green CI, and the combined-tree verification above. Squash-merged at exactly ee67abd598ddc7a3d10d20392487f4350bd76ced.

IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 25, 2026
Brings the branch up to date with main now that LeanerCloud#1276 (the base this PR
was stacked on), LeanerCloud#1100 (route documentation) and their neighbours have
merged.

Conflicted files:
- internal/api/handler_history.go,
  internal/api/handler_history_actionable_starvation_test.go: main's
  copies are byte-identical to the LeanerCloud#1276 tip (ee67abd) this branch
  already contained, so the branch's paged versions are kept unchanged.
- internal/api/types.go: HistoryResponse keeps the branch's
  ExecutionsLimit comment and the HasMore/NextCursor fields; main's other
  type changes (RecommendationsResponse.NextCursor,
  UnrestrictedAccountAccess, nullable CurrentCoverage) merged cleanly.
- internal/api/openapi.yaml: GET /api/history takes main's corrected
  parameter list from LeanerCloud#1100 (provider, account_ids, account_id,
  start/end dates, no stray interval parameter) and the 400 response,
  plus the branch's description, the paged limit (maximum 1000) and the
  cursor parameter. HistoryResponse keeps the branch's limit and
  executions_limit text and its has_more/next_cursor properties.

Also, internal/api/handler_history_truncation_test.go merged without a
conflict marker but declared TestHandler_getHistory_ActionableExecutionsLimitExposed
twice: LeanerCloud#1276 and the branch each added it at a different offset. Main's
copy equals the LeanerCloud#1276 tip, so the branch's version (one declaration)
is kept.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/few Limited audience priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant