Skip to content

feat(api): page the History view with a keyset cursor (has_more, next_cursor) - #1277

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

IAmJSD merged 9 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-674

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on fix/issue-686 (#1276). Until that merges, this diff also shows its commit (0ad90b7). Review only the last commit.

Root cause

GET /api/history read at most limit purchase_history rows (default 100, max 1000) and at most 100 terminal executions (failed, expired, canceled, audit-gap and partially completed). It returned truncated (#630) when a cap was hit, but it had no way to ask for the next rows, so older rows could not be reached at all. #632's scope note moved paging to this issue.

Fix

One paged stream. Completed purchases and terminal executions are merged into one list, newest first. The order is (timestamp DESC, source DESC, row id DESC):

  • timestamp is purchase_history.timestamp or purchase_executions.scheduled_date;
  • the row id is purchase_history.id or execution_id.

Both ids are UUIDs and unique per table, so every row has its own position. purchase_id can't be the tiebreaker because legacy rows repeat it (migration 000108). Each response lists limit rows of the stream.

Keyset cursor, not offset.

  • When more rows the caller may see follow the page, the response sets has_more: true and next_cursor. Passing next_cursor back as cursor resumes strictly after the last row shown.
  • Why not an offset:
    • The account scope and the provider/account filters on executions run in Go after the SQL read. A SQL offset would count rows the caller never sees, and a Go-side offset would need every earlier row re-read on every page.
    • Every row written above the offset (new purchases land at the top) would shift the page, so rows would repeat or be skipped.
  • With a keyset cursor, rows written between page loads never shift a later page, and a deep page costs the same as the first.
  • The cursor is opaque: base64url of v1.<source>.<unix µs>.<uuid>. Anything that doesn't decode to that form is a 400, so a bad cursor never reaches SQL.
  • offset is rejected with 400 instead of being ignored. feat(api): page the History view past the first limit rows (offset, has_more) #674's issue text asked for an offset parameter. A client built to that spec would otherwise get the first page back for every offset and keep asking for more.

Pending approvals are not paged. The actionable class (pending, notified, scheduled, approved, running, paused) is read and listed the same way as before:

  • the first page lists all of it, up to its own 1000 cap, whatever the page size;
  • pages fetched with a cursor leave it out, and they don't run the stale-approval sweep again.

So paging can never push a pending approval off the first page. Before this PR it stayed visible only because each source had its own cap. Now it holds by construction. #686's signals are unchanged: a failed listing or a full actionable class still sets truncated.

Scope.

  • has_more is judged on rows the caller may see. The page reads one visible row past limit, so rows outside the caller's scope never set has_more and never leave a trailing empty page.
  • next_cursor only names a row that was in the response.
  • The scope check runs again on every page, so a crafted cursor still returns only in-scope rows.
  • The scope rule is still filterPurchaseHistoryByAllowedAccounts, which now runs per batch. Its result is mapped back to rows by markOutOfScope, so the scope rule still has a single implementation.

Store. Two new methods in internal/config/store_postgres_history_page.go:

  • ListPurchaseHistoryPage: GetPurchaseHistoryFiltered's predicate plus the keyset condition. It also returns each row's id, and still scans through the shared scanPurchaseHistoryRow.
  • ListHistoryExecutionsPage: the terminal statuses with clean completed rows excluded, the scheduled_date date bounds pushed into SQL, plus the keyset condition.

Each source is read in batches of max(limit+1, 100), capped at 1000. A batch is read again only when filters or scope hide rows. There is no migration: idx_purchase_history_timestamp and idx_purchase_executions_scheduled_date cover both queries.

Response and docs.

  • HistoryResponse gains has_more and next_cursor.
  • truncated is also true whenever has_more is, so today's banner keeps showing until the UI has Load more.
  • executions_limit now equals limit, because terminal executions share the page.
  • In openapi.yaml, /api/history now documents the limit and cursor parameters, the paging contract, and the new fields.

Closes #674

Interaction with #1200 (scope-aware summary totals)

This branch doesn't touch the summary. On its own, summary still covers the rows in the response, as it did before. Combined with #1200, the summary is whole-scope on every page.

I checked this with a trial merge of this branch and #1200's head (abec6cd) in a scratch worktree:

Verification

All on the Linux VM with Go 1.26.9, with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true. Integration tests used real Postgres via testcontainers (the shared container). This is local mock and integration evidence only: there's no deployed-stack or real-account coverage. The change is DB and handler only.

Fail before, pass after (real Postgres). A throwaway probe seeded 12 completed purchases and paged with limit=5, following next_cursor:

New tests.

  • internal/api/handler_history_page_integration_test.go, real Postgres:
    • TestIntegration_HistoryPaging_EveryRowOnceInOrder: two tenants, purchases and terminal executions sharing timestamps within and across tables, limit=7. Every row comes back exactly once in stream order. Both pending approvals, older than every other row, are on page 1 and on no later page. A purchase and a failed execution written at the top after page 1 don't shift, repeat or skip anything.
    • TestIntegration_HistoryPaging_ScopedUserNeverPagesIntoAnotherTenant: tenant-B rows are newer, interleaved and older. The tenant-A user gets exactly the tenant-A rows plus their own ambient execution, once each and in order. No cursor names a hidden row, and has_more is false on the last page although tenant-B rows follow it. Tenant B's pending approval never appears.
  • internal/config/store_postgres_history_page_integration_test.go, real Postgres:
    • walks of both page methods with ties and pages that end inside a tie group, including a head insert between pages;
    • the keyset predicate where the two tables share a timestamp;
    • a drift guard: ListPurchaseHistoryPage selects the same rows as GetPurchaseHistoryFiltered for 8 filter shapes (provider, both account columns, the date edges ±1µs);
    • inclusive scheduled_date bounds.
  • internal/api/handler_history_page_test.go, unit/mock:
    • parameter validation: 16 cases covering offset, malformed, wrong version, unknown source, out-of-range timestamp, non-UUID, non-canonical UUID, injection, overlong;
    • a cursor round trip;
    • an invalid cursor reads nothing;
    • mixed-stream paging;
    • scoped paging that needs a second store batch, with a full last page followed only by hidden rows;
    • cursor pages skip the actionable fetch and the sweep;
    • a terminal-execution read failure sets truncated;
    • a purchase_history read failure is a 500;
    • a non-advancing keyset read is refused instead of looping.

Mutation probes. Each one was applied by hand, run, then reverted:

  • <= → < in the cross-source keyset condition: TestIntegration_HistoryKeyset_CrossSourceTies and TestIntegration_HistoryPaging_EveryRowOnceInOrder fail.
  • Dropping id DESC from the purchase_history ORDER BY: TestIntegration_ListPurchaseHistoryPage_WalksEveryRowOnce fails.
  • Skipping the scope check on terminal executions: 2 unit tests fail.
  • Judging has_more before the visibility check: TestHandler_getHistory_ScopedPagingStaysInScope fails. The first version of the test let this survive, so its fixture now ends on a full page.
  • Fetching actionable rows on every page: 2 unit tests fail.
  • Encoding the cursor at millisecond precision: the round-trip test fails.

Existing tests. The history handler tests now mock ListPurchaseHistoryPage / ListHistoryExecutionsPage instead of GetAllPurchaseHistory / GetPurchaseHistoryFiltered. For a []PurchaseHistoryRecord fixture, the shared mock assigns row ids by position, so most of these edits are one-line renames. These assertions changed on purpose:

  • "exactly limit rows ⇒ truncated": exactly a page of rows is no longer a false positive, so the tests now use limit+1.
  • executions_limit now follows limit.
  • TestHandler_getHistory_ScopedUserTruncationLeaksNoCounts: other tenants' rows no longer set truncated. It still asserts that no count leaks.
  • TestHandler_getHistory_FetchesDisjointClassesWithTheirLimits: asserts one actionable read plus one terminal page read.
go build ./... && go vet ./... && go vet -tags integration ./...
go test ./internal/api/ ./internal/config/ ./internal/mocks/ ./internal/server/ ./internal/analytics/ -count=1
go test -tags integration -count=1 -timeout 60m -run 'TestIntegration_HistoryPaging_' ./internal/api/
go test -tags integration -count=1 -timeout 60m -run 'TestIntegration_ListPurchaseHistoryPage_|TestIntegration_ListHistoryExecutionsPage_|TestIntegration_HistoryKeyset_' ./internal/config/
golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration ./internal/{api,config,mocks,server,analytics}/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" <changed non-test files>   # silent

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.

Notes and things left out

Frontend follow-up for the UI owner (#676)

No change under frontend/. What "Load more" needs, in frontend/src/api/history.ts, frontend/src/history.ts and frontend/src/types.ts:

  • Add has_more?: boolean, next_cursor?: string and actionable_executions_limit?: number to HistoryResponse, and let getHistory pass cursor (and limit if wanted).
  • Load more. When has_more is true, show a "Load more" button under the history table. On click, repeat the request with the same filters plus cursor=next_cursor, then append the returned rows to the history list.
    • Pages after the first never contain pending or in-flight rows, so the approval queue doesn't change.
    • Drop the stored cursor whenever a filter changes or History reloads.
    • Hide the button once a response has has_more: false.
  • De-duplicate. When appending, de-duplicate by purchase_id and keep the row from the newest response. A stale approval swept to expired after page 1 shows up again on a later page in its new state (see the notes above).
  • Banner. When has_more is true, the rows can be paged to, so replace "Narrow the date range or filters to see them" with the Load more button. Keep the banner only when truncated && !has_more, meaning rows that paging can't reach: the actionable cap (quote actionable_executions_limit) or a failed listing. The executions_limit text in the current banner now equals limit.
  • Summary cards. Keep the cards from the first response; don't re-render them from later pages. With feat(api): scope-aware SQL totals for the History summary cards #1200 the summary is the same on every page. Without it, a later page's summary covers only that page.

Maintainers: please mirror the issue's triage labels on this PR: priority/p2, severity/medium, urgency/this-quarter, impact/many, effort/m, type/feat, triaged.

🤖 Generated with Claude Code

IAmJSD and others added 2 commits October 23, 2026 05:42
…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>
…_cursor)

GET /api/history listed at most `limit` purchase_history rows and 100
terminal executions, and rows past those caps could not be reached: there
was no way to ask for the next page.

Completed purchases and terminal executions are now one stream, newest
first, ordered by (timestamp DESC, source DESC, row id DESC), which is
unique. Each response lists `limit` rows of it and, when more rows the
caller may see follow, sets has_more and next_cursor; passing next_cursor
back as `cursor` returns the next page. The cursor is a keyset position,
not an offset: the account scope and the execution filters run in Go
after the SQL read, so an offset would count rows the caller never sees,
and every row written ahead of it would shift the page. With the keyset,
a later page resumes strictly after the last row shown, so rows written
between page loads never repeat or skip a row, and a page costs the same
at any depth. `offset` is rejected with 400 instead of ignored, so a
client written against an offset API cannot loop on the first page.

Actionable executions (pending approvals, in-flight purchases) are not
paged: the first page lists the whole class as before, and cursor pages
leave it out, so paging can never push a pending approval off the first
page. The LeanerCloud#686 signals are kept: a failed execution listing or a full
actionable class still sets truncated.

has_more is judged on rows the caller may see, so other tenants' rows
never set it, and the cursor only ever names a row the caller was shown.
The scope is applied again on every page.

Store: ListPurchaseHistoryPage (GetPurchaseHistoryFiltered's predicate
plus the keyset, returning each row's id) and ListHistoryExecutionsPage
(the terminal statuses, clean completed rows excluded, scheduled_date
bounds pushed into SQL). No migration: the existing timestamp and
scheduled_date indexes serve both.

Closes LeanerCloud#674

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged urgency/this-quarter Within the quarter impact/many Affects most users effort/m Days type/feat New capability 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.

IAmJSD and others added 5 commits October 25, 2026 16:12
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>
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>
Main (LeanerCloud#1200) made purchaseHistoryFilterWhere the one purchase_history
predicate builder and historyExecutionsPredicate the one executions
predicate, shared by the History row fetch and the summary aggregate so
the totals count exactly the rows the list can show. This branch's page
queries carried their own copies (purchaseHistoryPageConds and an inline
status/clean-completed predicate). Use the shared ones instead, so the
paged list and the whole-scope summary cannot drift apart.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main (LeanerCloud#1200) added purchaseHistoryRowStatus so listed purchase_history
rows and the summary's aggregate groups get the same Status. The paged
stream set the literal "completed"; use the constant.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The History summary tests came in from main (LeanerCloud#1200) and mock the old
purchase_history reads (GetAllPurchaseHistory, GetPurchaseHistoryFiltered).
This branch reads purchases through ListPurchaseHistoryPage, so the
filter-equality test panicked on a nil row filter. Mock the paged read
instead; the assertions are unchanged.

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

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Brought this branch up to date with main. This branch is stacked on #1276, so I merged #1276's updated head (ee67abd, which already contains origin/main 2514c01) instead of merging main twice. New head: 2b3c8bb. A trial merge with the current main (c1f1781) is clean and passes go vet -tags integration ./....

Conflict resolution (merge commit 998ebbd):

  • internal/api/main_nonet_test.go and internal/api/main_integration_test.go: both sides added a teardown hook list for the shared Postgres server. This branch called it afterAllTests, and main (test: share one Postgres server per package across integration tests #1400) calls it afterTests. I took main's files unchanged.
  • internal/api/handler_history.go, getHistory: I combined it with feat(api): scope-aware SQL totals for the History summary cards #1200 along the lines this PR's description proposed:
    • The page is fetched first. loadHistoryScope then runs once, and scope.filter applies it to the actionable rows.
    • staleExecs is declared outside the Cursor == nil branch, so the sweep stays deferred until after the summary aggregate. Cursor pages don't sweep.
    • The response carries main's Summary and SummaryComplete plus this branch's has_more and next_cursor. The summary is now whole-scope on every page.
  • internal/api/handler_history.go, fetchPurchaseHistory: main's historyFilters.purchaseHistoryFilter() stays, because the summary uses it. The keyset batch read now starts from it, with Limit set to the batch size, and calls ListPurchaseHistoryPage.

Follow-up commits (semantic conflicts that merged without a textual conflict):

  • fix(config): ListPurchaseHistoryPage now uses main's purchaseHistoryFilterWhere instead of this branch's purchaseHistoryPageConds copy, and ListHistoryExecutionsPage uses main's historyExecutionsPredicate. The paged list and the summary aggregate now share the same predicates.
  • fix(api): paged purchase rows get main's purchaseHistoryRowStatus instead of the literal "completed".
  • fix(api): feat(api): scope-aware SQL totals for the History summary cards #1200's handler_history_summary_test.go mocked GetAllPurchaseHistory and GetPurchaseHistoryFiltered. It now mocks ListPurchaseHistoryPage, the change this description predicted. Before it, TestGetHistory_SummaryFilterMatchesRowFetchFilter panicked.

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 on analytics, api, config, mocks and server, and gocyclo is silent.
  • go test -race -short passes on those packages.
  • go test -race -tags integration passes on analytics, api, config and server. Analytics passed on a rerun after a Postgres container startup timeout on the loaded VM.
  • The secrets scan is clean and the planted key was flagged. The allowlist test passes.

When merging, note that ListHistoryExecutionsPage still projects COALESCE(canceled_by, cancelled_by). If #1253 merges first, this branch should switch to plain canceled_by.

@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 2b3c8bb0e79f5c0ba2b3e6d4052e5ecd9884f68c

Blocking:

  • none

Non-blocking:

  • internal/api/handler_history_page.go historyStream.next: if a terminal-execution batch read fails mid-page while purchase_history reads succeed, the page carries on with purchases only. next_cursor can then point past executions that were never shown. The page is flagged truncated, but it also has has_more: true, and the frontend (history-paging.ts) then offers Load more instead of the banner. Following the cursor skips those executions for the rest of that paging session. It's a narrow transient-failure case. A clean fix is to end the page at the failure (no cursor past the last consumed execution key), or to clear next_cursor when incomplete is set.
  • internal/api/handler_history_page.go fetchHistoryPage has no scan budget. Provider and account filters on terminal executions run in Go, and scope runs in Go for both sources, so a request whose filter matches nothing in purchase_executions (for example provider=gcp on an AWS-heavy table) reads the whole terminal class in batches of 100. Each batch with visible candidates also reloads the account scope (markOutOfScope -> filterPurchaseHistoryByAllowedAccounts -> loadHistoryScope). feat(api): scope-aware SQL totals for the History summary cards #1200 is merged now and getHistory already holds scope, so passing it in and using scope.allows would drop both the per-batch reload and the sameHistoryRow re-matching.
  • internal/api/types.go:974 and openapi.yaml truncated: the "can be a false positive when exactly a cap's worth of rows exist" sentence is now wrong for the paged stream, since has_more reads one row ahead. The body says it was left for fix(history): truncation banner quotes the wrong cap and a failed class listing does not set truncated #686/feat(api): scope-aware SQL totals for the History summary cards #1200 to rewrite. Both of those are now in or under this branch, so it can be fixed here.
  • Merge order: this conflicts with fix(api): document every router.go route in openapi.yaml and guard against drift #1100 in openapi.yaml (/api/history parameters and HistoryResponse). git merge-tree between the two heads reports a conflict.

Attacked:

  • Keyset correctness: worked through historyKeysetCondition for all three source relations against the (ts DESC, source DESC, id DESC) order. The tie-break is correct. Both columns are TIMESTAMPTZ NOT NULL (µs, matching the µs cursor), and both ids are UUID and unique (purchase_history.id PK, unique_execution_id), so the key is total. The cursor decode is strict (version, source enum, year range, canonical UUID, length ≤ 256) and every value is bound, with no string interpolation. Scope: page rows go through markOutOfScope, actionable rows through scope.filter. has_more and next_cursor only reflect visible rows. Actionable rows and the stale sweep are first-page only.
  • Ran go build ./..., go vet ./..., go vet -tags integration ./internal/...: ok. go test -race -short -count=1 on api, config, mocks, server and analytics: ok. go test -race -tags integration -count=1 ./internal/config/ ./internal/api/ (full packages, testcontainers Postgres): ok. golangci-lint on api and config, with and without --build-tags integration: 0 issues. gocyclo -over 10 on the changed non-test files: silent. Merge with current origin/main: clean.
  • Mutation / fail-before: (1) <= -> < in the cross-source keyset branch fails TestIntegration_HistoryKeyset_CrossSourceTies, TestIntegration_HistoryPaging_EveryRowOnceInOrder and ..._ScopedUserNeverPagesIntoAnotherTenant. (2) Dropping the scope check on terminal executions fails TestHandler_getHistory_ScopedPagingStaysInScope, ..._ScopedUserTruncationLeaksNoCounts and the scoped integration test. Both reverted.

Body-stale:

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.

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>
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Merged current main into the branch. #1276, the base this PR was stacked on, has merged, so this PR's diff against main is now only #674's changes. New head: 2b349dd.

Conflicts:

One more fix in the merge: handler_history_truncation_test.go merged without conflict markers but declared TestHandler_getHistory_ActionableExecutionsLimitExposed twice. #1276 and this branch had each added it at a different offset. This branch's version, with one declaration, is kept.

Checks:

  • go build and go vet (default, integration, e2e) pass. golangci-lint reports 0 issues with and without the integration tag, and gocyclo is silent.
  • go test -race -short passes on api, config, mocks, server and analytics.
  • go test -race -tags integration passes on api, config, server and analytics against real Postgres.
  • The git-secrets scan is clean, and the allowlist test gives 137 PASS.

Note: #1254 deletes the hand-rolled analytics mockConfigStore that this branch adds two stubs to. Whichever of the two merges second will conflict in internal/analytics/collector_test.go. The resolution is to drop the stubs.

@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 2b349dd69ccbd301f81d4bb8bd30356e225e55a2

Blocking:

  • none

Non-blocking:

  • internal/api/handler_history_page.go (markOutOfScope → filterPurchaseHistoryByAllowedAccounts → loadHistoryScope): every batch of every source reloads the account scope, including ListCloudAccounts for scoped users.
    • Each request can now walk the whole of purchase_history and purchase_executions in 1000-row batches. That happens when a scoped user's visible rows are sparse, or when the provider/account filters match only a few executions, since those run in Go.
    • On main a request was bounded at about limit + 100 + 1000 rows. Now it is O(table) SQL reads plus O(table/1000) scope loads, all inside one Lambda invocation.
    • The body notes this. feat(api): scope-aware SQL totals for the History summary cards #1200 is merged and getHistory already calls loadHistoryScope, so a cheap follow-up is to load the scope once and pass it into the stream (scope.allows per row). That also removes the sameHistoryRow re-matching.
    • A scan budget, to stop a sparse-scope request from timing out on a large table, should be tracked as a follow-up issue.
  • The first-page contents change while the frontend has no Load more yet (feat(frontend): Load more on the History view, Approval Queue from all loaded pages #676 is open, and the frontend never sends limit, so it gets 100).
  • internal/api/types.go:984 and openapi.yaml (truncated): "can be a false positive when exactly a cap's worth of rows exist" / "exactly limit or executions_limit rows" is now wrong for the paged stream, where has_more is exact. The body deferred this to fix(history): truncation banner quotes the wrong cap and a failed class listing does not set truncated #686 and feat(api): scope-aware SQL totals for the History summary cards #1200, and both have merged.
  • The issue asked for offset, and this ships a keyset cursor and returns 400 for offset. The reasoning in the body is sound: the scope runs in Go, so an offset would count rows the caller can't see and would shift as new rows arrive. Because it is an API contract change from the issue text, it is listed for the owner as well.

Attacked:

  • Keyset correctness: worked through historyKeysetCondition for all three source relations against the order (timestamp DESC, source DESC, id DESC). Confirmed purchase_history.id and purchase_executions.execution_id are UUID, timestamp and scheduled_date are TIMESTAMPTZ, the cursor round-trips at microseconds, and the Go SortsBefore lowercase-hex comparison matches PostgreSQL's uuid ordering.
  • Scope: confirmed the stream applies the same historyScope.allows rule as before, and that has_more and next_cursor only ever come from visible rows. A crafted cursor is decoded strictly (version, source, range-checked µs, canonical UUID) and the scope is re-applied on every page. Pending approvals appear on the first page only, and the stale sweep runs only there.
  • Ran: go build ./..., go vet ./... (also with -tags integration, and tests/e2e with -tags e2e), go test -race -short -count=1 on api/config/mocks/server/analytics (ok), go test -race -tags integration -run History on api and config (ok, real Postgres), golangci-lint with and without the integration tag (0 issues), and gocyclo -over 10 (silent).
  • Mutation proofs, each reverted afterwards:
    • Changing the cross-source <= to < fails TestIntegration_HistoryKeyset_CrossSourceTies, ..._HistoryPaging_EveryRowOnceInOrder and ..._ScopedUserNeverPagesIntoAnotherTenant.
    • Judging the page-full check before the visibility check fails TestHandler_getHistory_ScopedPagingStaysInScope.

Body-stale:

Reviewer: rB18. 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

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

Addendum to my review above, posted after skimming the earlier reviews. The verdict is unchanged.

Blocking:

  • none

Non-blocking:

  • Raised in an earlier review (IAmJSD's agent, on 2b3c8bb0), and I agree. It is still present on 2b349dd6.
    • In internal/api/handler_history_page.go, historyStream.next drops the execution source and keeps going when a terminal-execution batch read fails mid-page.
    • The page can then emit has_more: true with a next_cursor that lies past executions it never showed. Following that cursor skips them for the rest of the paging session, although truncated is set.
    • Fix: end the page at the failure, or omit next_cursor when page.incomplete is set.

Attacked:

  • See the review above.

Body-stale:

  • See the review above.

Reviewer: rB18. 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.

Conflict: internal/analytics/collector_test.go. LeanerCloud#1254 folded the hand-rolled analytics mock onto mocks.MockConfigStore, so took main's file and dropped this branch's two stubs there; the shared mock already carries them from this branch's internal/mocks/stores.go change.

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 (now 9395bccf). There was one conflict, internal/analytics/collector_test.go: #1254 (merged) moved the hand-rolled analytics mock onto mocks.MockConfigStore, so I took main's file and dropped this branch's two stubs there. The shared mock already carries them through this branch's internal/mocks/stores.go change. Build, vet (all tag sets), race unit tests (analytics, api, mocks, config), analytics integration, api History integration and golangci-lint all pass; secrets scan clean. Needs a fresh review at this head.

@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 9395bccf0ca2f3196494286b5b04a32894b2ce47

Blocking:

  • none

Non-blocking:

  • internal/api/handler_history_page.go markOutOfScope: it is correct today. historyScope.allows reads only AccountID, CreatedByUserID, CloudAccountID and Provider, and sameHistoryRow compares all four plus PurchaseID and Timestamp, so the greedy re-match of the kept subset is exact. The catch is the coupling: if allows ever reads a field that sameHistoryRow doesn't compare, two rows that match on the compared fields could swap visibility and leak an out-of-scope row. feat(api): scope-aware SQL totals for the History summary cards #1200 is merged, so this can be one loadHistoryScope per request plus scope.allows(&rows[i].row) per row. That removes both the coupling and the per-batch getAccountScope + ListCloudAccounts reload. The body itself suggests this.
  • Cost for sparse scoped users: one request can walk all of purchase_history and the terminal executions in 1000-row batches before it fills a page or reaches the end. The body acknowledges this, and the date window bounds it. Fine for now, worth tracking alongside fix(history): pending approvals can fall off the first page because filters are applied after the executions cap; add paging and push provider/account/date filters into the executions SQL #632 PR2 (scope in SQL).
  • First-page composition changes: the default limit=100 is now shared by completed purchases and terminal executions. Before, up to 100 purchases plus 100 terminal executions were shown. Main's frontend already has Load more (frontend/src/history-paging.ts) for exactly this contract (has_more, next_cursor, cursor), so the older rows are one click away.
  • internal/api/types.go Truncated doc still says it "can be a false positive when exactly a cap's worth of rows exist". That no longer holds for the paged stream. The body notes this and leaves it on purpose.
  • From an earlier review (on 2b3c8bb0, repeated by rB18), and still present at this head: historyStream.next drops the execution source when a terminal-execution batch read fails mid-page and carries on with purchases. The page then sets truncated together with has_more/next_cursor pointing past executions it never showed. Main's historyTruncationMessage returns '' whenever a cursor exists, so the user gets no banner, and following the cursor skips those executions for the rest of the session. This needs a transient DB error, so I agree it isn't blocking. The fix is to omit next_cursor when page.incomplete is set, or to end the page at the failure.

Attacked:

  • Keyset correctness. historyKeysetCondition matches SortsBefore and the ORDER BY: cross-source ties use <= or < by source rank, and same-source ties use the uuid tiebreak. purchase_history.id is a UUID PK and purchase_executions.execution_id is UUID with unique_execution_id (000008), so the key is unique. Lowercase hex string order equals PostgreSQL uuid byte order. The cursor is µs (timestamptz precision), its UUID must be canonical, year bounds are enforced, the length is capped, and all values are bound parameters (no injection path). offset gives a 400.
  • Mutation probes (each reverted):
    • markOutOfScope made a no-op: 7 unit tests plus TestIntegration_HistoryPaging_ScopedUserNeverPagesIntoAnotherTenant FAIL.
    • Same-source tiebreak < changed to <=: TestIntegration_ListPurchaseHistoryPage_WalksEveryRowOnce, ..._ListHistoryExecutionsPage_WalksEveryRowOnce and ..._HistoryKeyset_CrossSourceTies FAIL.
    • has_more judged before the visibility check: TestHandler_getHistory_ScopedPagingStaysInScope FAILS.
  • Ran: go build ./...; go vet (plain and -tags integration) on api/config/mocks/server; go test -race -short -count=1 on the same four (ok); go test -race -tags integration -run TestIntegration_History ./internal/api/ and -run 'TestIntegration_ListPurchaseHistoryPage_|TestIntegration_ListHistoryExecutionsPage_|TestIntegration_HistoryKeyset_|TestIntegration_HistorySummary' ./internal/config/ (all pass on real Postgres); golangci-lint v2.10.1 with and without the integration tag (0 issues); gocyclo -over 10 on the changed non-test files (silent). A local merge with current main (2 commits ahead) vets and passes the api/config unit tests.

Body-stale:

Reviewer: rB22. 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 9395bccf0ca2f3196494286b5b04a32894b2ce47 merged onto main (eb9e4802), together with #1277, #1284, #1288, #1290, #1294, #1306 in that order (combined tree 6bb85247, 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 23:59
@IAmJSD
IAmJSD merged commit a61b9be into LeanerCloud:main Oct 25, 2026
38 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 9395bccf, green CI, and the combined-tree verification above. Squash-merged at exactly 9395bccf0ca2f3196494286b5b04a32894b2ce47.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/many Affects most users priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/feat New capability urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(api): page the History view past the first limit rows (offset, has_more)

1 participant