[IAmJSD]

Problem

#1240 made a paginated GET /api/recommendations walk the cache in keyset chunks, which bounds the memory held. But every page request still read every filtered row into Go. The summary and regions describe the whole filtered set, and the suppression, account-override, enabled-provider, account-scope and min_savings_pct filters only ran in Go (see the old doc comment in internal/api/handler_recommendations_scan.go). Walking an N-row tenant in pages of L rows therefore still moved N rows over the wire and through json.Unmarshal N/L times.

Fix

A paginated request now goes through Scheduler.PageRecommendations → PostgresStore.PageStoredRecommendations:

• Filters in SQL, on the decoded payload (the same values the Go filters read):
  • the existing column filter;
  • enabled providers;
  • account scope. The handler turns the AccountScope into the exact set of IDs Allows grants (scopeAllowedAccountIDs), so a scoped user is never shown, or counted against, an out-of-scope or account-less row.
  • account overrides. Overrides are resolved in Go with the existing ResolveAccountConfigsForRecs for the (account, provider, service) keys the scope's rows actually have, then passed as a JSON rule object keyed by AccountConfigKey, the same key the Go map uses.
  • the min_savings_pct floor. It is evaluated in float8 operation for operation, so it rounds like Go: 57/100*100 = 56.99999999999999 is dropped at a floor of 57, as Go drops it.
• Suppressions stay in Go, where they apply. Rows on an active suppression 6-tuple (every stored spelling of the service, for example ec2 and compute) are few. The store reads them inside the snapshot, and the existing applySuppressionIndex + filterRecsByResolvedConfigs decide them. SQL keeps those rows by primary key. This avoids re-implementing the variant, Details-decode and "shared" tenancy rules in SQL.
• Aggregate summary: count, regions, and sum(... ORDER BY id) for savings and upfront, so the float8 additions happen in the order the Go summary used. Totals match the scan to the bit. count(DISTINCT id) detects duplicate IDs.
• One page plus one row reaches Go. The page rows go through the Go filters once more, which also stamps the suppression badge fields. Any disagreement falls back instead of returning a wrong row.
• One snapshot: the touched-row read, the aggregate and the page read run in one REPEATABLE READ, READ ONLY transaction. The summary and rows can no longer come from different collections. The old chunked scan could mix them; see the race test below.
• SQL is structured to stay cheap and planable. Per-row work is staged in OFFSET 0-fenced subqueries, so expressions are not inlined into each other. The page read uses LATERAL stages, so the planner walks recommendations_scan_key_idx in order and stops at the LIMIT.

Where SQL can't be faithful, Go is used (fallback)

PageRecommendations returns ErrRecommendationsPageNeedsScan, and the handler serves the request from the unchanged Go scan (getRecommendationsPageByScan), in these cases:

• Float values outside safe bounds. A float8 overflow or underflow raises an error in Postgres but not in Go. Values outside {0} ∪ [1e-100, 1e100] also fall back, as do non-number JSON in numeric fields and non-integer counts or terms.
• Override resolution fails. The scan logs this and passes rows through, so the fallback keeps that behavior.
• A row's override key was not resolved, for example a new account that appeared between the key read and the snapshot.
• A present-but-empty cloud_account_id, or two keys that join to the same AccountConfigKey string.
• Duplicate IDs. The scan then fails the request exactly as before.
• A cursor that is not valid UTF-8 or contains NUL. Postgres text can't hold it, but the scan compares it byte-wise.
• SQL kept a page row the Go filters reject. This is a defensive check.

None of these occur with data the collector writes. They exist so the result is never different from the scan, only slower.

Behavior notes

• There are no API shape changes. Unpaginated requests are untouched.
• A suppression read failure still fails the request (issue chore(server/scheduler): medium/low findings from the 2026-07-28 full review #111). It does not fall back.
• On fallback, the scan repeats the freshness read. The cold-start kick is then a no-op, because of the collection marker or the Lambda guard.

Closes #981

Verification

All evidence is local: unit/mock tests, plus integration tests against a real Postgres 16 container (testcontainers). Nothing ran against a real cloud account.

Commands run on this branch (head 70567463):

go build ./... && go vet ./... && go vet -tags integration ./...
go test -count=1 ./internal/config/ ./internal/scheduler/ ./internal/api/ ./internal/mocks/ ./internal/server/   # each run separately: ok
golangci-lint (v2.10.1) run --allow-parallel-runners --build-tags integration ./internal/config/ ./internal/scheduler/ ./internal/api/ ./internal/mocks/ ./internal/testutil/ ./internal/server/   # 0 issues
gocyclo -over 10 -ignore "_test\.go" <changed non-test files>   # clean
go test -tags integration -count=1 -timeout 20m -run 'TestRecommendationsPageSQL_|TestGetRecommendations_PageDoesNotReadEveryRow|TestGetRecommendations_PageAndSummaryShareOneSnapshot|TestGetRecommendations_PagedScanMatchesLegacyOnPostgres' ./internal/api/   # ok (75.8s)

New tests:

• TestRecommendationsPageSQL_MatchesScanOracle (Postgres). This is the equivalence proof.
  • Dataset: a seeded cache of about 900 rows. It has registered and account-less rows; aws, azure and gcp; legacy and canonical slugs; Savings Plans; empty regions; engines; every pct-input shape; fractional dollar amounts; IDs whose byte order differs from linguistic order; and floor-boundary rows. It also has 60 suppressions, with and without variants, shared tenancy, unreadable Details, legacy ec2 vs compute, and expired rows, plus service configs and account overrides on every dimension.
  • Combinations: 2 enabled-provider sets × 6 account scopes (unrestricted, one account, account+unregistered, external-ID/name-only, nothing, scope without registered names) × 11 filters × page sizes 7/97/1000.
  • Check: each page is compared to the old Go scan from the same cursor with require.Equal: rows, badges, bit-exact totals, regions and next_cursor. Out-of-scope rows are asserted absent. The SQL path must not fall back on this fixture.
• TestRecommendationsPageSQL_FallsBackToScan / _DuplicateIDsStillFail (Postgres). Each fallback input makes the SQL read report NeedsScan, and the endpoint answers exactly as the scan does. Duplicate IDs still fail.
• TestGetRecommendations_PageDoesNotReadEveryRow (Postgres, the regression). It wraps the store, counts rows pulled through ListStoredRecommendationsChunk during a full paginated walk, and requires 0.
• TestGetRecommendations_PageAndSummaryShareOneSnapshot (Postgres, concurrency). Four readers page while a writer rewrites all 1200 rows in place with alternating values. Every response's summary must be one collection's total, and every row must come from that same collection.
• Unit tests: scheduler_page_test.go covers the suppression split and legacy spellings, each fallback reason, and suppression-read fail-closed. handler_recommendations_page_sql_test.go covers the SQL path's use of scope and limit, each handler-side fallback, the error path, and scopeAllowedAccountIDs against Allows.

Fail-before (fixture + bound test files copied onto origin/main at ae6854b3):

--- FAIL: TestGetRecommendations_PageDoesNotReadEveryRow    Should be zero, but was 9460  (a paginated request must not scan the cache into Go)
--- FAIL: TestGetRecommendations_PageAndSummaryShareOneSnapshot    []float64{1200, 2400} does not contain 1400 / 2200 / 1700  (summary mixes two collections)

Both pass on this branch (660 page reads raced 22 collections).

Mutation probes: each mutation was applied, the test listed was run, and the file was restored.

Mutation	Result
drop the account-scope condition	oracle FAIL (221 subtests)
route suppressed rows through the SQL override filter	oracle FAIL (133)
drop the empty-engine carve-out	oracle FAIL (117)
pct < → <=	oracle FAIL (15)
pct in exact numeric instead of float8	oracle FAIL (5)
sum ordered by row UUID instead of ID	oracle FAIL (143)
READ ONLY without REPEATABLE READ	snapshot race test FAIL
sum(sv) with no ORDER BY	survives

The no-ORDER-BY mutant survives because, in the current PG16 plan, the count(DISTINCT id) input sort already feeds the sums in ID order. The explicit ORDER BY is what guarantees the order, rather than the plan.

Rough timing (local docker Postgres, no network). This comes from a throwaway benchmark that is not committed: 20,000 rows, one 500-row page.

Filter	SQL path	Go scan
none	~190–210 ms	~380–400 ms
min_savings_pct=20	~285–295 ms	~400–420 ms

Over a real network the scan additionally ships every payload to the process, while the SQL path ships one page. The aggregate is still O(filtered rows) inside Postgres, which the summary inherently needs.

Combined-tree verification. Tested on a local, never-pushed tree: queue/platform @ 594ac3ec = origin/main ae6854b3 plus:

Branch	Tested head
fix/issue-357-delete-cleanup-lambda	56989d8
fix/issue-36-archera-gate	2d22986
fix/issue-35-assert-api-a	f793689
fix/issue-35-assert-api-b	fac72f3
fix/issue-120-stale-rec-sweep	07c2e9c
fix/issue-981-summary-sql	7056746

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: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests (-tags integration -count=1, one package at a time): internal/api, internal/auth, internal/config, internal/scheduler, internal/server and internal/testutil pass.
• Secrets: git-secrets on the changed files found nothing, and a planted fake key was flagged; scripts/test-git-secrets-allowlist.sh passes.

Left out / for reviewers

• The aggregate is still a full pass in Postgres, plus one DISTINCT read of override keys per request. Only the rows moved into Go are O(page).
• Suppressed-tuple rows are decoded in Go. This is bounded by recent purchases, not tenant size.
• No migration or new index. The existing recommendations_scan_key_idx serves the page read.
• Payload-type differences off the page. A row whose payload Go cannot decode (for example a string in savings) used to fail every page. It now falls back, so it still fails. A few such shapes that SQL does not type-check, such as a non-string provider, would now only fail when that row lands on a page. The collector never writes these.

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

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: ae6854b
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: b9703845-c4a1-482a-9129-6fb389b480fe

Commits

Reviewing files that changed from the base of the PR and between ae6854b and 7056746.

Files selected for processing: 17

• internal/api/handler_recommendations_page.go
• internal/api/handler_recommendations_page_bound_integration_test.go
• internal/api/handler_recommendations_page_sql_fixture_integration_test.go
• internal/api/handler_recommendations_page_sql_integration_test.go
• internal/api/handler_recommendations_page_sql_test.go
• internal/api/handler_recommendations_scan.go
• internal/api/mocks_test.go
• internal/api/types.go
• internal/config/interfaces.go
• internal/config/recommendations_page.go
• internal/config/store_postgres_recommendations_page.go
• internal/config/store_postgres_recommendations_page_sql.go
• internal/mocks/stores.go
• internal/scheduler/scheduler_page.go
• internal/scheduler/scheduler_page_test.go
• internal/server/interfaces.go
• internal/testutil/mocks.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• internal/api/handler_recommendations_page_sql_test.go: this new file registers sched.On("PageRecommendations", ...) but never calls AssertExpectations, which is the pattern TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite #35 (test(api): assert mock expectations in the first half of the api tests #1593/test(api,auth,scheduler,server,email): assert mock expectations in the remaining tests #1594) is removing. In the fallback cases (TestGetRecommendations_SQLPageFallsBackToScan), nothing proves the SQL path was tried before the scan. PagedReadUsesSQLPage covers the happy path through sched.Calls. A one-line t.Cleanup(func() { sched.AssertExpectations(t) }) would close the gap. On the combined tree it is the only internal/api file this PR adds to the TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite #35 grep list.
• internal/api/mocks_test.go MockScheduler.PageRecommendations walks m.ExpectedCalls without the mock's mutex. This is harmless today (the -race run was clean), but it would race if a parallel test registered expectations concurrently.
• getRecommendationsPage logs a Warnf on every fallback. A tenant with a persistent fallback shape (an empty cloud_account_id, out-of-bounds floats) will log once per page request. Consider Debug, or a single-line reason.
• A paginated request now makes up to four passes in Postgres: the triples DISTINCT, the suppressed-row read, the aggregate, then the page. The fewer rows sent to Go outweigh this in the PR's 20k-row timing, but the aggregate stays O(filtered rows), as the body says. The off-page payload-type caveat in "Left out" is accurate.

Attacked:

• Tenant scope (sensitive path): scopeAllowedAccountIDs tests the candidates scope.Accounts ∪ keys(nameByID) with Allows. Allows(id, name) is true only if id is on the allow-list or nameByID[id] is non-empty, so this set is exactly the IDs Go keeps. Account-less rows drop out via NULL = ANY(...). Overrides are resolved only for in-scope triples. The handler re-runs filterRecommendationsInScope and the provider filter on every page row and falls back on any disagreement. Mutation: with the Restricted condition removed from pageScopeCond, TestRecommendationsPageSQL_MatchesScanOracle FAILS (230 subtests). Restored afterwards.
• SQL vs Go semantics, read line by line against recEffectiveSavingsPct/recOnDemandBaseline (same float8 operation order, term/count integer checks, magnitude bounds), passesOverrideDimensions/inListRule/engineMatches (exclude wins, empty engine carve-out, SP min-count exempt), CanonicalSuppressionService vs SuppressionServiceSpellings (every spelling covered, so no suppressed row escapes the Go decision), the COLLATE "C" byte-order cursor, and the ordered sums. All parameters are bound (pageArgs); the field names interpolated into SQL are compile-time constants. The reads run in one REPEATABLE READ, READ ONLY transaction, and the triples read outside it is safe because a NULL rule makes the request fall back.
• Fail-before: I forced getRecommendationsPage onto the scan, and TestGetRecommendations_PageDoesNotReadEveryRow FAILS ("Should be zero, but was 9460"). It passes on the head.
• Ran on the head: go build ./..., go vet ./..., go vet -tags integration ./... (clean); go test -race -short -count=1 ./internal/{api,config,scheduler,mocks,server,testutil}/ (ok); go test -race -tags integration -count=1 -run 'TestRecommendationsPageSQL_|TestGetRecommendations_' ./internal/api/ (ok, 200s); go test -race -tags integration -count=1 ./internal/scheduler/ ./internal/config/ (ok); golangci-lint v2.10.1 on the changed packages, with and without --build-tags integration: 0 issues; gocyclo -over 10 on the changed non-test files: silent. On the combined tree (test(api): assert mock expectations in the first half of the api tests #1593 + test(api,auth,scheduler,server,email): assert mock expectations in the remaining tests #1594 + perf(api): page recommendations with every list filter in SQL and an aggregate summary #1596 merged locally onto ae6854b), the full internal/api integration suite and the unit suite also pass.

Body-stale:

• none

_{Reviewer: rQ4. 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]

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

Blocking:

• none

Non-blocking:

• internal/config/store_postgres_recommendations_page_sql.go (pageAccountExpr and the other payload expressions; pageScopeCond): SQL reads payload keys case-sensitively, but Go's json.Unmarshal matches them case-insensitively and lets the later key win. On a hand-edited payload such as {"cloud_account_id":"in-scope","Cloud_Account_ID":"other"}, SQL scopes the row to in-scope and Go to other. If that row lands on the page, the handler's rows.keep re-check falls back to the scan. If it does not, the scoped caller's summary counts it. The collector writes payloads with json.Marshal, so only a direct DB edit can produce this. Note it, don't block on it.
• handler_recommendations_scan.go:39: one persistent fallback trigger (for example a single row with savings outside [1e-100, 1e100], or a fractional count) makes every paginated request log a Warn and pay the triples read, the aggregate and then the full scan. That is slower than before this PR for that tenant. A counter or a rate-limited log would make this visible.
• pageScopeCond splices buildRecommendationFilter's unqualified column names (provider, region, cloud_account_id, monthly_savings) into a FROM list that also holds the lateral a/b stages. It is correct today because no stage column shares those names. A future stage column named like a table column would fail as an ambiguous reference, loudly rather than silently, so this is only a maintainability note.
• (Also raised by an earlier review, and I agree:) handler_recommendations_page_sql_test.go registers PageRecommendations expectations but never calls AssertExpectations, so the fallback unit test doesn't prove the SQL path was tried before the scan.

Attacked:

• Tenant scope (the sensitive part): scopeAllowedAccountIDs is exact. With nameByID keyed by internal IDs, identifies() is always false, so Allows(id, nameByID[id]) reduces to contains(id), and the candidate set (scope.Accounts plus nameByID keys) covers every ID that can pass. The touched read, the triples read, the aggregate and the page read all apply pageScopeCond. Mutation: with the Restricted condition removed, the oracle's scope=alpha subtests fail, because the handler's rows.keep re-check refuses the page.
• Filter parity, compared by hand with the Go code: the pct floor matches recEffectiveSavingsPct/recOnDemandBaseline branch for branch, including the AWS sec(iac): low-severity least-privilege gaps in hosted-infra grants (2026-09-02 audit) #323 guard and Term*12 as an int before the float conversion. The override SQL matches passesOverrideDimensions (exclude wins, the empty-engine carve-out, MinCount exempt for SP). Suppression spellings are complete by construction ({canonical} ∪ alias keys filtered by CanonicalSuppressionService). A missing rule makes the row unsure and the request fall back. Duplicate IDs and an invalid cursor fall back too. Float sums use ORDER BY sk, rid with "C" collation, the same order as the Go scan.
• Fail-before: forcing getRecommendationsPage onto the scan path makes TestGetRecommendations_PageDoesNotReadEveryRow fail with "Should be zero, but was 9460", and ..._PageAndSummaryShareOneSnapshot fail with "[1200 2400] does not contain 1400/1900/2200". Both were restored afterwards.
• Ran: go build ./..., go vet ./..., go vet -tags integration ./...; go test -race -short -count=1 on api, config, scheduler, mocks, server and testutil (all ok); go test -race -tags integration -run 'TestRecommendationsPageSQL_|TestGetRecommendations_Page|TestGetRecommendations_PagedScan' ./internal/api/ (ok, 209s); full go test -race -tags integration -count=1 on ./internal/scheduler/ (ok), ./internal/config/ (ok), ./internal/api/ (ok, 344s) and ./internal/server/ (ok). golangci-lint was not completed locally: the shared VM was saturated by other agents' lint runs, so I rely on CI and the body's 0-issues report.

Body-stale: none

_{Reviewer: rQ2. 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]

Local verification at 70567463528a308249bdbf006db91caea26bc18a merged onto main (d71af683), together with #1591, #1595, #1596 in that order (combined tree 71d6c3d4, 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 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

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