fix(reads): page the nine remaining unbounded sweeps and the digest RPC (#548) - #556
Merged
Merged
Conversation
…PC (#548) PostgREST caps every response at the API "Max rows" limit and returns the capped result as an ordinary 200, so truncation is indistinguishable from a complete read. #533 shipped `fetchAllRows` but wired it to only two call sites; nine reads were left unbounded. Two of them failed as wrong user-visible behaviour rather than a wrong number: - The digest's `notification_preferences` read. `resolveChannels(undefined)` treats a missing row as the schema default `{ inApp: true, email: true }`, so every student the truncated read dropped got emailed — including the ones who had turned email off. A short read could not mean "send more email"; now the read is chunked and paged, and a failure skips the tenant instead of falling through with an empty preferences map. - The digest's idempotency read. One notification row is written per recipient, so past the cap it truncated and a cron retry re-sent the day's digests, inverting the module's documented "re-runs never double-send" guarantee exactly when a retry happens. `get_daily_digest_candidates` could not be fixed from the caller side: it is a SETOF function with no ORDER BY, so `.range()` was not a stable window and there was no exact count to verify against. It now takes a keyset cursor on (tenant_id, user_id) — UNIQUE via tenant_users_unique — with ORDER BY and a clamped LIMIT. Keyset rather than OFFSET because the candidate set is computed live and shifts under a multi-second sweep. The caller stops on an EMPTY page, never a short one: a short page is ambiguous between "end of set" and "the row cap clamped us", and reading it as "done" is the original bug in a new place. Also adds `fetchAllRowsIn` for `.in()` lookups. Those have a second ceiling on the REQUEST side — a few hundred uuids overflow the gateway's URL limit — and response paging cannot help with a request that is never made. Verified by forcing the cap, not by a green run on a small dataset: - Live, against real PostgREST with the row cap lowered to 5 and 37 transactions / 23 payouts / 37 candidates seeded past it (scripts/qa-548-cap-forcing.mts). Unpaged reads come back with 5 rows while count reports 37; every paged read returns all 37 and the sums are exact. The opted-out student at index 30 is invisible to the unpaged read and found by the paged one. - In unit tests, via a fake PostgREST that clamps every response to 7 rows over a 40-student fixture. Reverting the fix fails 6 of them with exactly the expected numbers (7 of 40 sent; the opted-out student emailed). `tests/unit/unbounded-read-guard.test.ts` statically scans the tree for reads against growth tables that never bound themselves, so the next unbounded sweep fails in review. Pre-existing sites are recorded in KNOWN_UNBOUNDED as a ratchet, split into `scoped` (bounded by a user or a course) and `gap` (real platform-wide sweeps still outstanding under #540); a stale entry also fails, so the list cannot rot. `rollover_all_leagues`' tenant loop is untouched — it is PL/pgSQL and never crosses PostgREST, so it is not subject to the cap. Gates: typecheck, lint, build, test:unit 446/446 (was 411). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185GrowMKwTrLQE1it2HNGN
guillermoscript
marked this pull request as ready for review
July 26, 2026 17:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #548. Part of EPIC #540, §3.5.
The problem
PostgREST caps every response at the API "Max rows" limit (
supabase/config.toml:18, same default on the hosted project) and returns the capped result as an ordinary200. Truncation is indistinguishable from a complete read. #533 shippedlib/supabase/fetch-all-rows.tsbut wired it to only two call sites; nine reads were left unbounded.Seven of them fail as a wrong number. Two fail as wrong user-visible behaviour, which is why this is worth the diff:
daily-digest.ts—notification_preferencesresolveChannels(undefined)reads a missing row as the schema default{ inApp: true, email: true }, so every student the short read dropped gets emailed — including the ones who turned email off.daily-digest.ts— the idempotency readWhat changed
Nine reads paged, each with
{ count: 'exact' }and a primary-key.order()— an unordered.range()window is not a stable page, so that ordering is load-bearing, not cosmetic. Where a natural sort already existed (created_at) the PK is added as a tiebreaker rather than replacing it, so display order is unchanged.dashboard/admin/payouts/page.tsx·dashboard/teacher/revenue/page.tsx·actions/admin/revenue.tsactions/platform/billing-health.ts(all five reads)api/cron/solana-reconcile·api/cron/solana-pull— these are work queues, sofetchAllRowsthrowing means a short queue can no longer masquerade as a finished onelib/notifications/daily-digest.ts(candidates, tenants, settings, preferences, idempotency)get_daily_digest_candidatesnow paginates (20260726140000). It could not be fixed from the caller side: a SETOF function with noORDER BYgives.range()nothing stable to window over, and offers no exact count to verify against. It takes a keyset cursor on(tenant_id, user_id)— UNIQUE viatenant_users_unique, so a total order with no ties — plusORDER BYand a clampedLIMIT. Keyset rather thanLIMIT/OFFSETbecause the candidate set is computed live fromreview_cards/study_goalsand shifts under a multi-second sweep, which is exactly when OFFSET skips and repeats rows. Every parameter defaults, so the old zero-arg call still resolves.The caller stops on an empty page, never a short one. A short page is ambiguous between "end of the set" and "the row cap clamped us below what we asked for" — reading it as "done" is the original bug in a new place.
lib/supabase/fetch-all-rows-in.ts(new) for.in()lookups. These have a second, independent ceiling on the request side: a few hundred uuids overflow the gateway's URL limit, and paging the response cannot help with a request that is never made. It chunks the ids and reads each chunk throughfetchAllRows, so both ceilings are covered.fetch-all-rows.tsitself is untouched, as the issue asks.A failed preferences read now skips the tenant instead of continuing with an empty map. "Nobody has opted out" is the worst possible reading of "the read failed".
Verification — cap-forced, not "it works locally"
A test that passes on a development dataset proves nothing here, because on a development dataset every unpaged read is already complete. Both layers force the cap.
Live, against real PostgREST (
scripts/qa-548-cap-forcing.mts) — row cap lowered to 5, seeded to 37 transactions / 23 payouts / 37 candidates:Step 0 re-proves the cap is biting on every run — otherwise a green result would only mean the override failed to apply. Fixture and cap were torn down afterwards;
config.tomlis unmodified in the diff.In unit tests — a fake PostgREST that clamps every response to 7 rows over a 40-student fixture. The decisive check: reverting the fix fails 6 of them with exactly the expected numbers.
Covers both acceptance criteria directly: a second run in the same tenant-local day sends zero additional emails, and a student with
email: falseis not emailed even when the candidate set exceeds the cap.Guard against the next one
tests/unit/unbounded-read-guard.test.tsstatically extracts every PostgREST chain against a table whose row count grows with usage and fails on any that never bounds itself. Pre-existing sites are recorded inKNOWN_UNBOUNDEDas a ratchet, split intoscoped(bounded by one user or one course) andgap(real tenant/platform-wide sweeps still outstanding under #540 — the admin analytics and dashboard revenue sums, the admin listings, and three more cron queues). A stale entry also fails the test, so the list cannot rot as things get fixed. The detector has its own tests, because a scan that silently stops matching looks exactly like a clean repository.Out of scope, deliberately
rollover_all_leagues' tenant loop is untouched — it runs entirely in PL/pgSQL and never crosses PostgREST, so the cap does not apply.Note for the reviewer
The migration is applied to local only. Cloud needs
supabase db pushbefore this ships, or the digest cron will call the RPC with three arguments it does not have.Gates
npm run typechecknpm run lintnpm run buildnpm run test:unit146a8cfa)🤖 Generated with Claude Code
https://claude.ai/code/session_0185GrowMKwTrLQE1it2HNGN