From 9159d1e6567719f95b6190c21a80ab73bf915625 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 13 Aug 2026 20:47:12 -0700 Subject: [PATCH 1/2] fix(tables): stop every table paginating forever on a null totalCount --- apps/sim/hooks/queries/tables.ts | 11 ++++-- .../utils/table-rows-pagination.test.ts | 29 +++++++++++++++ .../queries/utils/table-rows-pagination.ts | 24 ++++++++---- apps/sim/lib/api/contracts/tables.test.ts | 37 ++++++++++++++++++- apps/sim/lib/api/contracts/tables.ts | 13 +++++-- apps/sim/lib/table/planner.ts | 29 ++++++++++++--- 6 files changed, 123 insertions(+), 20 deletions(-) diff --git a/apps/sim/hooks/queries/tables.ts b/apps/sim/hooks/queries/tables.ts index 21c3c70ec49..f6ab08ecc9d 100644 --- a/apps/sim/hooks/queries/tables.ts +++ b/apps/sim/hooks/queries/tables.ts @@ -140,7 +140,7 @@ type TableRowsParams = Omit & export type TableRowsResponse = Pick< ContractJsonResponse['data'], - 'rows' | 'totalCount' + 'rows' | 'totalCount' | 'nextCursor' > interface RowMutationContext { @@ -195,8 +195,13 @@ async function fetchTableRows({ }, signal, }) - const { rows, totalCount } = response.data - return { rows, totalCount } + const { rows, totalCount, nextCursor } = response.data + /** + * `nextCursor` is kept because it is the only authoritative end-of-table signal: the server + * sets it exactly when the drain proved an unreturned witness row, so it covers a page cut by + * the byte budget as well as one cut by `limit`. See {@link hasMoreTableRows}. + */ + return { rows, totalCount, nextCursor } } function invalidateRowCount(queryClient: ReturnType, tableId: string) { diff --git a/apps/sim/hooks/queries/utils/table-rows-pagination.test.ts b/apps/sim/hooks/queries/utils/table-rows-pagination.test.ts index d34eee750de..c5d348ddcfe 100644 --- a/apps/sim/hooks/queries/utils/table-rows-pagination.test.ts +++ b/apps/sim/hooks/queries/utils/table-rows-pagination.test.ts @@ -51,6 +51,35 @@ describe('hasMoreTableRows', () => { it('returns false when a stale-low count is already exceeded', () => { expect(hasMoreTableRows([makePage(10, 5)])).toBe(false) }) + + /** + * The server sets `nextCursor` exactly when the drain proved an unreturned witness row, so it + * answers correctly for a page cut by the byte budget — where both page fullness and the count + * mislead. It therefore wins over the count rules whenever it is present. + */ + describe('nextCursor', () => { + it('ends the drain on a null cursor even when the count claims more rows', () => { + expect(hasMoreTableRows([{ ...makePage(36, 100), nextCursor: null }])).toBe(false) + }) + + it('continues on a non-null cursor even when the count is already covered', () => { + // A byte-cut page: fewer rows than asked for, and the advisory count disagrees. + expect(hasMoreTableRows([{ ...makePage(3, 3), nextCursor: 'c1' }])).toBe(true) + }) + + it('reads the cursor from the last page, not page 0', () => { + const pages = [ + { ...makePage(1000, null), nextCursor: 'c1' }, + { ...makePage(12, null, 1000), nextCursor: null }, + ] + expect(hasMoreTableRows(pages)).toBe(false) + }) + + it('falls back to the count rules for pages cached before the cursor was threaded through', () => { + expect(hasMoreTableRows([makePage(36, 100)])).toBe(true) + expect(hasMoreTableRows([makePage(3, 3)])).toBe(false) + }) + }) }) describe('getNextTableRowsPageParam', () => { diff --git a/apps/sim/hooks/queries/utils/table-rows-pagination.ts b/apps/sim/hooks/queries/utils/table-rows-pagination.ts index afca1ea5c22..3dcb4d7d8f8 100644 --- a/apps/sim/hooks/queries/utils/table-rows-pagination.ts +++ b/apps/sim/hooks/queries/utils/table-rows-pagination.ts @@ -9,6 +9,11 @@ export type TableRowsPageParam = number | TableRowsCursor interface TableRowsPageLike { rows: ReadonlyArray<{ id: string; orderKey?: string }> totalCount: number | null + /** + * Optional because pages cached before this field was threaded through predate it; those fall + * back to the count rules below. + */ + nextCursor?: string | null } /** Rows loaded across all fetched pages. */ @@ -17,18 +22,23 @@ export function countLoadedTableRows(pages: readonly TableRowsPageLike[]): numbe } /** - * Whether more rows may exist past the fetched pages. A page is terminal only when it is - * empty or when page 0's `COUNT(*)` is already covered — never when it is merely shorter - * than the requested page size, so a short server page can never be misread as end-of-table. + * Whether more rows may exist past the fetched pages. * - * `totalCount` is advisory (computed in a separate transaction from the page read). A - * stale-high count self-corrects via the empty-page rule at the cost of one extra request; - * a stale-low count (rows deleted after page 0's COUNT) stops the drain early — accepted, - * since the view is already stale and the run-stream/interval invalidations refetch it. + * `nextCursor` is the authoritative answer and is preferred whenever the server sent one: it is + * non-null exactly when the drain proved an unreturned witness row, so it is correct for a page + * cut by the byte budget as well as one cut by `limit`. Page fullness cannot answer this — a + * byte-cut page is legitimately shorter than the requested size. + * + * The count rules remain as a fallback for pages cached before `nextCursor` was threaded through. + * They are weaker: `totalCount` is advisory (computed in a separate transaction from the page + * read), so a stale-high count self-corrects via the empty-page rule at the cost of one extra + * request, and a stale-low count stops the drain early. A null `totalCount` is read as "unknown, + * assume more" — which is why the `includeTotal` coercion bug made every table page forever. */ export function hasMoreTableRows(pages: readonly TableRowsPageLike[]): boolean { const lastPage = pages[pages.length - 1] if (!lastPage || lastPage.rows.length === 0) return false + if (lastPage.nextCursor !== undefined) return lastPage.nextCursor !== null const totalCount = pages[0].totalCount return totalCount == null || countLoadedTableRows(pages) < totalCount } diff --git a/apps/sim/lib/api/contracts/tables.test.ts b/apps/sim/lib/api/contracts/tables.test.ts index 0f368363d3b..e384d099c49 100644 --- a/apps/sim/lib/api/contracts/tables.test.ts +++ b/apps/sim/lib/api/contracts/tables.test.ts @@ -2,7 +2,42 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' -import { tableEventStreamQuerySchema } from '@/lib/api/contracts/tables' +import { tableEventStreamQuerySchema, tableRowsQuerySchema } from '@/lib/api/contracts/tables' + +/** + * `requestJson` parses the query through this schema on the CLIENT before building the URL, so + * these values arrive as the caller's real types, not as URL strings. A string-only coercion + * therefore read the grid's `includeTotal: param === 0` boolean as `false`, page 0 came back with + * `totalCount: null`, and `hasMoreTableRows` — which treats a null total as "more may exist" — + * reported `hasNextPage` forever. Every table then paid a wasted extra page fetch on mount and + * before every row insert. + */ +describe('tableRowsQuerySchema includeTotal', () => { + it('accepts a real boolean, which is what the client passes', () => { + expect( + tableRowsQuerySchema.parse({ workspaceId: 'ws-1', includeTotal: true }).includeTotal + ).toBe(true) + expect( + tableRowsQuerySchema.parse({ workspaceId: 'ws-1', includeTotal: false }).includeTotal + ).toBe(false) + }) + + it('still accepts the URL strings a direct API caller sends', () => { + expect( + tableRowsQuerySchema.parse({ workspaceId: 'ws-1', includeTotal: 'true' }).includeTotal + ).toBe(true) + expect( + tableRowsQuerySchema.parse({ workspaceId: 'ws-1', includeTotal: 'false' }).includeTotal + ).toBe(false) + }) + + it('defaults to true when absent or empty, so a bare request still gets its count', () => { + expect(tableRowsQuerySchema.parse({ workspaceId: 'ws-1' }).includeTotal).toBe(true) + expect(tableRowsQuerySchema.parse({ workspaceId: 'ws-1', includeTotal: '' }).includeTotal).toBe( + true + ) + }) +}) describe('tableEventStreamQuerySchema', () => { it('parses an explicit cursor', () => { diff --git a/apps/sim/lib/api/contracts/tables.ts b/apps/sim/lib/api/contracts/tables.ts index a75f2ab495e..576fff3383c 100644 --- a/apps/sim/lib/api/contracts/tables.ts +++ b/apps/sim/lib/api/contracts/tables.ts @@ -1,6 +1,7 @@ import { isRecordLike } from '@sim/utils/object' import { z } from 'zod' import { + booleanQueryFlagSchema, folderIdSchema, privateSecretProvenanceBundleSchema, requiredFieldSchema, @@ -800,11 +801,17 @@ export const tableRowsQueryBaseSchema = z.object({ .optional() ) .default(0), + /** + * Absent, null, and empty all fall through to the `true` default, so a bare request still + * gets its count. Everything else goes to {@link booleanQueryFlagSchema}, which accepts a real + * boolean as well as the URL strings — `requestJson` parses this schema on the CLIENT before + * building the URL, so the value arrives as the caller's own type, and a string-only coercion + * silently read the grid's `includeTotal: param === 0` as `false`. + */ includeTotal: z .preprocess( - (value) => - value === null || value === undefined || value === '' ? undefined : value === 'true', - z.boolean().optional() + (value) => (value === null || value === undefined || value === '' ? undefined : value), + booleanQueryFlagSchema.optional() ) .default(true), }) diff --git a/apps/sim/lib/table/planner.ts b/apps/sim/lib/table/planner.ts index 85f4969b6f6..6f8ea5df2e9 100644 --- a/apps/sim/lib/table/planner.ts +++ b/apps/sim/lib/table/planner.ts @@ -15,14 +15,32 @@ export type DbTransaction = Parameters[0]>[0] const READ_STATEMENT_TIMEOUT_MS = 15_000 const READ_LOCK_TIMEOUT_MS = 3_000 -async function setReadTimeouts(trx: DbTransaction): Promise { - await trx.execute(sql.raw(`SET LOCAL statement_timeout = '${READ_STATEMENT_TIMEOUT_MS}ms'`)) - await trx.execute(sql.raw(`SET LOCAL lock_timeout = '${READ_LOCK_TIMEOUT_MS}ms'`)) +/** + * Applies every guard in ONE round-trip. Each `trx.execute` is its own serial round-trip (the + * driver runs `prepare: false`), and every user-table read opens a transaction, so issuing these + * separately cost 2–3 round-trips on every page, count, and drain batch. + * + * `set_config(name, value, is_local => true)` is exactly `SET LOCAL` — transaction-scoped, dying + * with the commit — but it is a function call, so several fit in a single `SELECT`. Semicolon- + * joining `SET LOCAL` statements would not work here: the driver sends this over the extended + * protocol, which rejects multiple commands in one message. + */ +async function setReadGuards(trx: DbTransaction, seqscanOff: boolean): Promise { + /** + * Only ever set to `off`, never explicitly to `on` — the unflagged path must leave whatever + * the server default is, exactly as the separate `SET LOCAL enable_seqscan = off` did. + */ + const seqscan = seqscanOff ? sql`, set_config('enable_seqscan', 'off', true)` : sql`` + await trx.execute(sql` + select + set_config('statement_timeout', ${`${READ_STATEMENT_TIMEOUT_MS}ms`}, true), + set_config('lock_timeout', ${`${READ_LOCK_TIMEOUT_MS}ms`}, true)${seqscan} + `) } /** * Runs a user-table read inside a transaction that always caps `statement_timeout` - * / `lock_timeout` (see {@link setReadTimeouts}). Pass `seqscanOff` for queries + * / `lock_timeout` (see {@link setReadGuards}). Pass `seqscanOff` for queries * with no tenant-bounded index plan — custom column sorts and filtered counts — * where the planner otherwise seq-scans the whole shared `user_table_rows` * relation (every tenant's rows); see {@link withSeqscanOff} for the measured @@ -34,8 +52,7 @@ export async function withReadGuards( opts?: { seqscanOff?: boolean } ): Promise { return db.transaction(async (trx) => { - await setReadTimeouts(trx) - if (opts?.seqscanOff) await trx.execute(sql`SET LOCAL enable_seqscan = off`) + await setReadGuards(trx, opts?.seqscanOff ?? false) return fn(trx) }) } From 9861887d5bcfd7901db47672ecd2ffea0fb8a34c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 13 Aug 2026 20:55:38 -0700 Subject: [PATCH 2/2] fix(tables): keep an emptied view terminated, and count masked reads off the seq scan --- apps/sim/hooks/queries/tables.ts | 8 ++++++++ .../utils/table-rows-pagination.test.ts | 16 +++++++++++++++- .../queries/utils/table-rows-pagination.ts | 6 ++++-- apps/sim/lib/api/contracts/tables.test.ts | 12 ++++++++---- apps/sim/lib/api/contracts/tables.ts | 6 +++++- apps/sim/lib/table/planner.ts | 16 ++++++++++------ apps/sim/lib/table/rows/service.ts | 18 ++++++++++++++++-- 7 files changed, 66 insertions(+), 16 deletions(-) diff --git a/apps/sim/hooks/queries/tables.ts b/apps/sim/hooks/queries/tables.ts index f6ab08ecc9d..405fbf21e02 100644 --- a/apps/sim/hooks/queries/tables.ts +++ b/apps/sim/hooks/queries/tables.ts @@ -1300,6 +1300,14 @@ export function useDeleteTableRowsAsync({ workspaceId, tableId }: RowMutationCon ...page, rows: page.rows.filter((r) => keep.has(r.id)), ...(page.totalCount != null ? { totalCount: keep.size } : {}), + /** + * The view is being emptied on purpose, so it has no next page — stated + * explicitly because the server's cursor would otherwise say otherwise and + * scrolling would pull back the very rows the job is deleting. Only the + * row-count arithmetic used to carry this, which {@link hasMoreTableRows} + * no longer consults once a cursor is present. + */ + nextCursor: null, })), } : old diff --git a/apps/sim/hooks/queries/utils/table-rows-pagination.test.ts b/apps/sim/hooks/queries/utils/table-rows-pagination.test.ts index c5d348ddcfe..14ece2e1b0f 100644 --- a/apps/sim/hooks/queries/utils/table-rows-pagination.test.ts +++ b/apps/sim/hooks/queries/utils/table-rows-pagination.test.ts @@ -75,10 +75,24 @@ describe('hasMoreTableRows', () => { expect(hasMoreTableRows(pages)).toBe(false) }) - it('falls back to the count rules for pages cached before the cursor was threaded through', () => { + it('falls back to the count rules when a page carries no cursor', () => { expect(hasMoreTableRows([makePage(36, 100)])).toBe(true) expect(hasMoreTableRows([makePage(3, 3)])).toBe(false) }) + + /** + * The async "select all" delete strips rows from the active view and pins `nextCursor: null` + * so scrolling cannot pull back the rows the background job is still deleting. Deselecting a + * few leaves kept rows on the last page, so the row-count arithmetic that used to suppress + * `hasNextPage` no longer fires — only the pinned cursor does. + */ + it('stays terminated for a partially-emptied view whose pages pin a null cursor', () => { + const pages = [ + { ...makePage(2, 2), nextCursor: null }, + { ...makePage(1, null, 2), nextCursor: null }, + ] + expect(hasMoreTableRows(pages)).toBe(false) + }) }) }) diff --git a/apps/sim/hooks/queries/utils/table-rows-pagination.ts b/apps/sim/hooks/queries/utils/table-rows-pagination.ts index 3dcb4d7d8f8..81537755374 100644 --- a/apps/sim/hooks/queries/utils/table-rows-pagination.ts +++ b/apps/sim/hooks/queries/utils/table-rows-pagination.ts @@ -10,8 +10,10 @@ interface TableRowsPageLike { rows: ReadonlyArray<{ id: string; orderKey?: string }> totalCount: number | null /** - * Optional because pages cached before this field was threaded through predate it; those fall - * back to the count rules below. + * Optional only so this loose page shape stays usable by callers that do not have a server + * response to hand (tests, and the optimistic mappings). On the wire it is required — the + * contract declares it non-optional and `requestJson` validates the response — so a real page + * always carries it and the count fallback below is defensive, not a live path. */ nextCursor?: string | null } diff --git a/apps/sim/lib/api/contracts/tables.test.ts b/apps/sim/lib/api/contracts/tables.test.ts index e384d099c49..739d934acc8 100644 --- a/apps/sim/lib/api/contracts/tables.test.ts +++ b/apps/sim/lib/api/contracts/tables.test.ts @@ -7,10 +7,14 @@ import { tableEventStreamQuerySchema, tableRowsQuerySchema } from '@/lib/api/con /** * `requestJson` parses the query through this schema on the CLIENT before building the URL, so * these values arrive as the caller's real types, not as URL strings. A string-only coercion - * therefore read the grid's `includeTotal: param === 0` boolean as `false`, page 0 came back with - * `totalCount: null`, and `hasMoreTableRows` — which treats a null total as "more may exist" — - * reported `hasNextPage` forever. Every table then paid a wasted extra page fetch on mount and - * before every row insert. + * therefore read the grid's `includeTotal: param === 0` boolean as `false`, and page 0 came back + * with `totalCount: null` on every table. + * + * What that broke is the **filtered** total: `rowTotal` was permanently null, so select-all and + * everything downstream of it (bulk delete, run scope, the selected-count label) silently fell + * back to the table's UNFILTERED `rowCount`. It also left `hasMoreTableRows` reading a null total + * as "more may exist" — though that half is now answered by `nextCursor` instead, so this schema + * is not what removes the wasted page fetch. */ describe('tableRowsQuerySchema includeTotal', () => { it('accepts a real boolean, which is what the client passes', () => { diff --git a/apps/sim/lib/api/contracts/tables.ts b/apps/sim/lib/api/contracts/tables.ts index 576fff3383c..1f98a6cbfaa 100644 --- a/apps/sim/lib/api/contracts/tables.ts +++ b/apps/sim/lib/api/contracts/tables.ts @@ -806,7 +806,11 @@ export const tableRowsQueryBaseSchema = z.object({ * gets its count. Everything else goes to {@link booleanQueryFlagSchema}, which accepts a real * boolean as well as the URL strings — `requestJson` parses this schema on the CLIENT before * building the URL, so the value arrives as the caller's own type, and a string-only coercion - * silently read the grid's `includeTotal: param === 0` as `false`. + * silently read the grid's `includeTotal: param === 0` as `false` — leaving `totalCount` null on + * every table, and select-all falling back to the unfiltered row count. + * + * Unparseable values now reject rather than resolving to `false`, matching `limit` and `offset` + * in this same schema, which have always thrown on garbage. */ includeTotal: z .preprocess( diff --git a/apps/sim/lib/table/planner.ts b/apps/sim/lib/table/planner.ts index 6f8ea5df2e9..2220d5faa31 100644 --- a/apps/sim/lib/table/planner.ts +++ b/apps/sim/lib/table/planner.ts @@ -16,14 +16,18 @@ const READ_STATEMENT_TIMEOUT_MS = 15_000 const READ_LOCK_TIMEOUT_MS = 3_000 /** - * Applies every guard in ONE round-trip. Each `trx.execute` is its own serial round-trip (the - * driver runs `prepare: false`), and every user-table read opens a transaction, so issuing these - * separately cost 2–3 round-trips on every page, count, and drain batch. + * Applies every guard in ONE round-trip. Each awaited `trx.execute` is its own round-trip, and + * every user-table read opens a transaction, so issuing these separately cost 2–3 round-trips on + * every page, count, and drain batch. * * `set_config(name, value, is_local => true)` is exactly `SET LOCAL` — transaction-scoped, dying - * with the commit — but it is a function call, so several fit in a single `SELECT`. Semicolon- - * joining `SET LOCAL` statements would not work here: the driver sends this over the extended - * protocol, which rejects multiple commands in one message. + * with the commit, and reverting the same way on a savepoint rollback — but it is a function call, + * so several fit in one `SELECT`. It also takes the values as bound parameters, which `SET LOCAL` + * cannot. That is the reason they must be one statement rather than semicolon-joined: a bound + * parameter forces the extended protocol, which rejects multiple commands per message. + * + * The guards are the first statement in the transaction, so an invalid value aborts it before + * `fn(trx)` can run — there is no path where a read proceeds unguarded. */ async function setReadGuards(trx: DbTransaction, seqscanOff: boolean): Promise { /** diff --git a/apps/sim/lib/table/rows/service.ts b/apps/sim/lib/table/rows/service.ts index d0491f40919..015e9970917 100644 --- a/apps/sim/lib/table/rows/service.ts +++ b/apps/sim/lib/table/rows/service.ts @@ -1152,7 +1152,13 @@ export async function queryRows( // unfiltered count already plans an index-only scan on the table_id prefix. // The count uses the full-view WHERE (no cursor seek): totals cover the whole // view, not the remaining pages. - const hasFilter = Boolean(userClause) + /** + * The delete mask counts as a filter: it injects JSONB predicates into `baseConditions`, which + * is exactly the plan shape `countRowsTenantBounded` exists to keep off a seq scan of the shared + * relation. Reading only `userClause` sent a masked-but-unfiltered count down the plain branch, + * bounded only by the statement timeout. + */ + const hasFilter = Boolean(userClause || deleteMask) const countPromise = includeTotal ? hasFilter ? countRowsTenantBounded(whereClause) @@ -1264,7 +1270,15 @@ interface BoundedFetchResult { anchorOffset: number } -/** Belt-and-braces bound on drain iterations; unreachable in practice. */ +/** + * Belt-and-braces bound on drain iterations. + * + * Unreachable only because every iteration either consumes at least one row or cuts, and a bounded + * page's `limit` is capped at {@link TABLE_LIMITS.MAX_QUERY_LIMIT} — so the limit cut always fires + * first. That makes the two constants exactly tight: raising `MAX_QUERY_LIMIT` above this bound + * would let the loop exit with rows still unread and `hasMore: false`, which clients now trust as + * end-of-table (they terminate on `nextCursor`, which this decides). Raise both together. + */ const MAX_QUERY_BATCHES = 1000 /**