diff --git a/app/actions/platform/payouts.ts b/app/actions/platform/payouts.ts index d05e5722..30d94828 100644 --- a/app/actions/platform/payouts.ts +++ b/app/actions/platform/payouts.ts @@ -7,6 +7,7 @@ import { revalidatePath } from 'next/cache' import { getCurrentUserId } from '@/lib/supabase/tenant' import { PROVIDER_CAPABILITIES, type PaymentProvider } from '@/lib/payments/types' import { computeOwedBalances, DEFAULT_SCHOOL_PERCENTAGE, type TenantOwed } from '@/lib/payments/payouts-owed' +import { fetchAllRows } from '@/lib/supabase/fetch-all-rows' async function verifySuperAdmin() { const supabase = await createClient() @@ -24,32 +25,57 @@ export async function getPayoutsOwed(): Promise { await verifySuperAdmin() const admin = createAdminClient() - const [{ data: tenants }, { data: splits }, { data: txns }, { data: paid }] = await Promise.all([ - admin.from('tenants').select('id, name'), - admin.from('revenue_splits').select('tenant_id, school_percentage'), - admin - .from('transactions') - .select('tenant_id, payment_provider, amount, currency, school_percentage_snapshot, status, transaction_date') - .in('status', ['successful', 'refunded']) - .in('payment_provider', PLATFORM_SETTLED_PROVIDERS), - admin - .from('payouts') - .select('tenant_id, amount, currency, period_end, paid_at, created_at') - .eq('payout_method', 'manual') - .eq('status', 'paid'), + // Every read here is a whole-relation sweep whose rows are then summed, so a + // PostgREST row cap would misstate the balance instead of failing (issue + // #533): a short `transactions` read underpays the school, a short `payouts` + // read overpays it. `fetchAllRows` pages and then verifies the row count it + // collected against the server's own — hence `{ count: 'exact' }` and the + // primary-key `.order()` on each of the four. + const [tenants, splits, txns, paid] = await Promise.all([ + fetchAllRows('tenants', (from, to) => + admin.from('tenants').select('id, name', { count: 'exact' }).order('id').range(from, to) + ), + fetchAllRows('revenue_splits', (from, to) => + admin + .from('revenue_splits') + .select('tenant_id, school_percentage', { count: 'exact' }) + .order('split_id') + .range(from, to) + ), + fetchAllRows('transactions', (from, to) => + admin + .from('transactions') + .select( + 'tenant_id, payment_provider, amount, currency, school_percentage_snapshot, status, transaction_date', + { count: 'exact' } + ) + .in('status', ['successful', 'refunded']) + .in('payment_provider', PLATFORM_SETTLED_PROVIDERS) + .order('transaction_id') + .range(from, to) + ), + fetchAllRows('payouts', (from, to) => + admin + .from('payouts') + .select('tenant_id, amount, currency, period_end, paid_at, created_at', { count: 'exact' }) + .eq('payout_method', 'manual') + .eq('status', 'paid') + .order('payout_id') + .range(from, to) + ), ]) const schoolPercentageByTenant = new Map( - (splits || []).map((s) => [s.tenant_id, s.school_percentage as number]) + splits.map((s) => [s.tenant_id, s.school_percentage as number]) ) return computeOwedBalances( - (tenants || []).map((t) => ({ + tenants.map((t) => ({ tenantId: t.id, tenantName: t.name, schoolPercentage: schoolPercentageByTenant.get(t.id) ?? DEFAULT_SCHOOL_PERCENTAGE, })), - (txns || []) + txns .filter((t) => t.tenant_id && t.payment_provider && t.amount != null) .map((t) => ({ tenantId: t.tenant_id as string, @@ -63,7 +89,7 @@ export async function getPayoutsOwed(): Promise { // `period_end` is the exact "covered through" when a payout recorded a period; // manually recorded ones leave it null today, so fall back to when the money // actually moved (issue #511). - (paid || []).map((p) => ({ + paid.map((p) => ({ tenantId: p.tenant_id, amount: p.amount, currency: p.currency || 'usd', diff --git a/app/api/cron/enforce-plan-limits/route.ts b/app/api/cron/enforce-plan-limits/route.ts index 98b26249..41aca27e 100644 --- a/app/api/cron/enforce-plan-limits/route.ts +++ b/app/api/cron/enforce-plan-limits/route.ts @@ -1,6 +1,7 @@ import { NextRequest, NextResponse } from 'next/server' import { createClient, type SupabaseClient } from '@supabase/supabase-js' import { reconcileAccessCutoff } from '@/lib/billing/access-cutoff' +import { fetchAllRows } from '@/lib/supabase/fetch-all-rows' export const runtime = 'nodejs' @@ -46,7 +47,24 @@ export async function GET(req: NextRequest) { const supabase = getSupabaseAdmin() - const { data: tenants } = await supabase.from('tenants').select('id') + // Paged and verified rather than read in one shot (issue #533). A PostgREST + // row cap would truncate this list into a plain short array, and since the + // sweep's only job is to visit EVERY tenant, the tail would simply stop being + // reconciled with nothing in the response to say so — silently disabling the + // organic-growth trigger this cron exists to be. An incomplete read now fails + // the run loudly so the scheduler surfaces it. + let tenants: { id: string }[] + try { + tenants = await fetchAllRows<{ id: string }>('tenants', (from, to) => + supabase.from('tenants').select('id', { count: 'exact' }).order('id').range(from, to) + ) + } catch (err) { + console.error('enforce-plan-limits: could not read the tenant list', err) + return NextResponse.json( + { error: err instanceof Error ? err.message : 'Failed to read tenants' }, + { status: 500 } + ) + } const result = { scheduled: 0, @@ -59,7 +77,7 @@ export async function GET(req: NextRequest) { notifyFailures: 0, } - for (const tenant of tenants || []) { + for (const tenant of tenants) { try { const decision = await reconcileAccessCutoff(supabase, tenant.id, { notifyDueStages: true }) if (decision.action === 'schedule') result.scheduled++ diff --git a/docs/PLATFORM_ADMIN.md b/docs/PLATFORM_ADMIN.md index 89bf0b6b..4b698ba1 100644 --- a/docs/PLATFORM_ADMIN.md +++ b/docs/PLATFORM_ADMIN.md @@ -70,6 +70,31 @@ const adminClient = createAdminClient() **Important gotcha:** PostgREST FK embedding (`table(related_col)`) silently fails with the admin client for some FK relationships. Always fetch related records in a separate query using `.in('id', ids)`. +### Platform-wide sweeps must be paged (issue #533) + +PostgREST caps every response at the project's configured API **"Max rows"**. `supabase/config.toml` sets `max_rows = 1000` for the local stack; the cloud project has no `pgrst.db_max_rows` role override, so it runs on Supabase's hosted default, also **1000**. A capped response is an ordinary `200` with fewer rows in it — there is no error, no flag, and nothing downstream can tell it apart from a complete result. + +That makes it lethal for platform pages, which read whole cross-tenant relations and then do arithmetic over them. `getPayoutsOwed()` used to read `transactions` and `payouts` unbounded: past 1000 rows it would have underpaid or overpaid schools by however much fell off the end, confidently and silently. + +So: **any read whose rows are summed, counted, or iterated for side effects must use `fetchAllRows()`** (`lib/supabase/fetch-all-rows.ts`) rather than a bare `.select()`. + +```ts +import { fetchAllRows } from '@/lib/supabase/fetch-all-rows' + +const rows = await fetchAllRows('transactions', (from, to) => + admin + .from('transactions') + .select('tenant_id, amount', { count: 'exact' }) // required — the completeness check compares against it + .eq('status', 'successful') + .order('transaction_id') // required — a unique key, or pages overlap/skip + .range(from, to) +) +``` + +It pages until the relation is exhausted, shrinks its page size to whatever the server actually returns (so it is correct for any cap without knowing the number), and throws — naming the relation and both counts — if it ends up with fewer rows than the server said exist. A wrong total is worse than a failed page, so it fails. + +Reads that are already bounded (`.limit()`, `.single()`, a tenant-scoped page query) don't need it, and neither do aggregations done in SQL: `get_platform_stats()` and `get_platform_revenue()` sum inside Postgres and return one row, which is immune by construction and remains the better option for a large relation. + --- ## Feature Areas diff --git a/lib/supabase/fetch-all-rows.ts b/lib/supabase/fetch-all-rows.ts new file mode 100644 index 00000000..89c0ac3a --- /dev/null +++ b/lib/supabase/fetch-all-rows.ts @@ -0,0 +1,151 @@ +/** + * Complete, verified reads for platform-wide sweeps (issue #533). + * + * PostgREST caps every response at the project's configured API "Max rows" + * (`supabase/config.toml:18` sets `max_rows = 1000` locally; the cloud project + * carries no `pgrst.db_max_rows` role override, so it runs on Supabase's + * hosted default of 1000). A capped response is a plain 200 with fewer rows — + * indistinguishable from a complete one. Any caller that reads a whole + * relation and then does arithmetic over it therefore produces a confidently + * wrong number rather than an error: `getPayoutsOwed()` underpaid a school + * when `transactions` truncated and overpaid it when `payouts` did, and the + * `enforce-plan-limits` cron silently stopped visiting tenants past the cap. + * + * `fetchAllRows` pages until the relation is exhausted and then ASSERTS the + * result is complete against PostgREST's own exact count, so a cap (or a + * future change to it) surfaces as a thrown error instead of a quiet + * shortfall. Nothing here depends on the cap's value: the page size is ours, + * and the assertion is what actually guarantees completeness. + */ + +/** + * Rows requested per page. Deliberately below the 1000 cap so the common case + * never brushes against it — but the loop does not rely on that: a server + * configured lower simply hands back shorter pages, and `fetchAllRows` adopts + * the size it actually got (see below) rather than mistaking a capped page for + * the end of the relation. Nothing here breaks if the cap is changed. + */ +export const FETCH_ALL_PAGE_SIZE = 500 + +/** + * Runaway guard — a backstop against a server that keeps returning full pages + * forever, and against a pathologically small cap turning a big relation into + * an unbounded request storm. At the default page size this covers 1,000,000 + * rows, far past anything these sweeps read. + */ +const MAX_PAGES = 2000 + +/** The shape of a `count: 'exact'` PostgREST response, narrowed to what this helper reads. */ +interface CountedPage { + data: T[] | null + error: { message: string } | null + count: number | null +} + +/** + * Read every row of a query, verifying nothing was silently dropped. + * + * The caller builds one page per call. Two requirements, both load-bearing: + * + * - **`{ count: 'exact' }` on the select.** It is what the completeness + * assertion compares against; without it there is nothing to verify and the + * helper degrades to a plain paging loop. + * - **A stable `.order()` on a unique column** (a primary key). `.range()` + * windows over an unordered relation may overlap or skip rows between + * requests, which would trade a truncation bug for a duplication bug. + * + * ```ts + * const txns = await fetchAllRows('transactions', (from, to) => + * admin + * .from('transactions') + * .select('tenant_id, amount', { count: 'exact' }) + * .eq('status', 'successful') + * .order('transaction_id') + * .range(from, to) + * ) + * ``` + * + * @param relation Name used in error messages — the operator needs to know + * which read came up short. + * @throws if a page errors, if the relation is larger than `MAX_PAGES` pages, + * or if fewer rows were collected than the server said exist. + */ +export async function fetchAllRows( + relation: string, + page: (from: number, to: number) => PromiseLike>, + pageSize: number = FETCH_ALL_PAGE_SIZE, +): Promise { + const rows: T[] = [] + /** + * The count reported by the FIRST page — a snapshot of the relation as the + * sweep started. Later pages report a moving total under concurrent writes; + * comparing against the snapshot keeps the assertion about truncation rather + * than about how busy the table is. + */ + let expected: number | null = null + /** + * Shrinks to whatever the server is actually willing to return. PostgREST + * applies "Max rows" to each ranged request too, so asking for a window + * wider than the cap yields a short page that is NOT the end of the relation. + * Adopting the observed size makes the loop correct for any cap without + * knowing its value. + */ + let effectivePageSize = pageSize + + for (let request = 0; request < MAX_PAGES; request++) { + // Offset by rows already collected rather than by page index: it stays + // correct after `effectivePageSize` shrinks mid-sweep. + const from = rows.length + const { data, error, count } = await page(from, from + effectivePageSize - 1) + if (error) { + throw new Error(`fetchAllRows(${relation}): page at offset ${from} failed — ${error.message}`) + } + if (request === 0) expected = count + + const batch = data ?? [] + rows.push(...batch) + + if (batch.length < effectivePageSize) { + // Short page. Either the relation is exhausted, or the server capped this + // request below what we asked for. The count settles it: if rows are + // still owed and the server just proved it can return `batch.length` at a + // time, keep going at that size. An empty page means it can return + // nothing more, and the assertion below turns that into a loud error. + if (expected != null && rows.length < expected && batch.length > 0) { + effectivePageSize = batch.length + continue + } + assertComplete(relation, rows.length, expected) + return rows + } + // An exactly-full page is ambiguous — the next one may be empty — so we + // always ask again. The trailing empty request is expected, not wasteful. + } + + throw new Error( + `fetchAllRows(${relation}): still returning full pages after ${MAX_PAGES} requests ` + + `(${rows.length} rows); refusing to loop further.`, + ) +} + +/** + * The last line of defence. Collecting FEWER rows than the server itself + * reported is the failure this module exists to catch: the loop above handles + * the ordinary row cap, so a shortfall here means something else ate rows — the + * server refusing to return any more, or rows deleted mid-sweep shifting the + * offsets. Either way the caller's totals would be wrong, so it gets an error + * instead of a number. + * + * Collecting MORE is benign: rows inserted while the sweep ran. Treating that + * as corruption would fail the platform payouts page whenever a sale lands + * mid-render. + */ +function assertComplete(relation: string, fetched: number, expected: number | null) { + if (expected == null) return + if (fetched < expected) { + throw new Error( + `fetchAllRows(${relation}): incomplete read — fetched ${fetched} of ${expected} rows. ` + + `Any total computed from this would be silently wrong, so nothing is returned.`, + ) + } +} diff --git a/tests/unit/fetch-all-rows.test.ts b/tests/unit/fetch-all-rows.test.ts new file mode 100644 index 00000000..bd058cab --- /dev/null +++ b/tests/unit/fetch-all-rows.test.ts @@ -0,0 +1,141 @@ +import { describe, expect, it } from 'vitest' +import { fetchAllRows } from '@/lib/supabase/fetch-all-rows' + +/** + * Builds a fake PostgREST pager over `rows`. + * + * `serverCap` emulates the API "Max rows" setting: PostgREST clamps EVERY + * response — including a ranged one — to that many rows while `count` keeps + * reporting the true total. That is precisely why a short page cannot be read + * as "the relation is exhausted". + */ +function pagerOver( + rows: number[], + options: { serverCap?: number; count?: number | null; growTo?: number[] } = {} +) { + const calls: Array<{ from: number; to: number }> = [] + const page = async (from: number, to: number) => { + calls.push({ from, to }) + // A row inserted mid-sweep: later pages see the longer relation. + const source = calls.length > 1 && options.growTo ? options.growTo : rows + let slice = source.slice(from, to + 1) + if (options.serverCap != null) slice = slice.slice(0, options.serverCap) + return { + data: slice, + error: null, + count: options.count === undefined ? rows.length : options.count, + } + } + return { page, calls } +} + +const range = (n: number) => Array.from({ length: n }, (_, i) => i) + +describe('fetchAllRows', () => { + it('returns everything from a single short page without asking again', async () => { + const { page, calls } = pagerOver(range(3)) + await expect(fetchAllRows('t', page, 10)).resolves.toEqual([0, 1, 2]) + expect(calls).toEqual([{ from: 0, to: 9 }]) + }) + + it('handles an empty relation', async () => { + const { page } = pagerOver([]) + await expect(fetchAllRows('t', page, 10)).resolves.toEqual([]) + }) + + it('accumulates multiple pages in order', async () => { + const { page, calls } = pagerOver(range(25)) + await expect(fetchAllRows('t', page, 10)).resolves.toEqual(range(25)) + expect(calls).toEqual([ + { from: 0, to: 9 }, + { from: 10, to: 19 }, + { from: 20, to: 29 }, + ]) + }) + + it('asks once more when the relation is an exact multiple of the page size', async () => { + // The boundary case: page 2 comes back exactly full, so "full page" alone + // cannot mean "there is more" — it must be confirmed by an empty page 3. + const { page, calls } = pagerOver(range(20)) + await expect(fetchAllRows('t', page, 10)).resolves.toEqual(range(20)) + expect(calls).toHaveLength(3) + }) + + it('reads the whole relation when the server caps pages below the requested size', async () => { + // The #533 scenario as it actually presents: we ask for 1000 rows, the + // server's "Max rows" is 400, so every page comes back short. Reading that + // as "exhausted" is exactly the silent truncation the issue describes — + // here it must still return all 1500 rows. + const { page, calls } = pagerOver(range(1500), { serverCap: 400 }) + await expect(fetchAllRows('transactions', page, 1000)).resolves.toEqual(range(1500)) + // First request asks for 1000 and gets 400; the rest adopt that width. + expect(calls[0]).toEqual({ from: 0, to: 999 }) + expect(calls[1]).toEqual({ from: 400, to: 799 }) + expect(calls.at(-1)).toEqual({ from: 1200, to: 1599 }) + }) + + it('throws, naming the relation and both counts, when rows go missing anyway', async () => { + // The server reports 1500 rows but refuses to hand back more than the first + // 600 — a shortfall the page loop cannot recover from. It must be an error, + // never a low number. + let call = 0 + const page = async (from: number, to: number) => { + call++ + const slice = call <= 2 ? range(1500).slice(from, to + 1).slice(0, 300) : [] + return { data: slice, error: null, count: 1500 } + } + await expect(fetchAllRows('transactions', page, 1000)).rejects.toThrow( + /fetchAllRows\(transactions\): incomplete read — fetched 600 of 1500 rows/ + ) + }) + + it('does not throw when rows were inserted while the sweep ran', async () => { + // First page reports 15; by page 2 the relation has grown to 16. Extra rows + // are benign — only a shortfall means data was dropped. + const grown = range(16) + const { page } = pagerOver(range(15), { growTo: grown }) + await expect(fetchAllRows('t', page, 10)).resolves.toHaveLength(16) + }) + + it('skips the assertion entirely when the caller did not request a count', async () => { + const { page } = pagerOver(range(5), { count: null }) + await expect(fetchAllRows('t', page, 10)).resolves.toEqual(range(5)) + }) + + it('ignores a moving count from later pages and trusts the first page snapshot', async () => { + let call = 0 + const page = async (from: number, to: number) => { + call++ + // Page 1 says 15 (the truth at sweep start); page 2 claims 900 because + // another process is bulk-inserting. Trusting the latest count would + // manufacture a false "incomplete read". + return { data: range(15).slice(from, to + 1), error: null, count: call === 1 ? 15 : 900 } + } + await expect(fetchAllRows('t', page, 10)).resolves.toEqual(range(15)) + }) + + it('propagates a PostgREST error from a later page with its offset', async () => { + let call = 0 + const page = async (from: number, to: number) => { + call++ + if (call === 2) return { data: null, error: { message: 'statement timeout' }, count: null } + return { data: range(25).slice(from, to + 1), error: null, count: 25 } + } + await expect(fetchAllRows('payouts', page, 10)).rejects.toThrow( + /fetchAllRows\(payouts\): page at offset 10 failed — statement timeout/ + ) + }) + + it('refuses to loop forever when every page comes back full', async () => { + // A pathological server that always returns a full page would otherwise + // spin until the process died. + const page = async () => ({ data: range(10), error: null, count: null }) + await expect(fetchAllRows('t', page, 10)).rejects.toThrow(/refusing to loop further/) + }) + + it('defaults to a page size of 500', async () => { + const { page, calls } = pagerOver(range(1)) + await fetchAllRows('t', page) + expect(calls[0]).toEqual({ from: 0, to: 499 }) + }) +})