Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions apps/sim/hooks/queries/tables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ type TableRowsParams = Omit<TableRowsQueryInput, 'filter' | 'sort'> &

export type TableRowsResponse = Pick<
ContractJsonResponse<typeof listTableRowsContract>['data'],
'rows' | 'totalCount'
'rows' | 'totalCount' | 'nextCursor'
>

interface RowMutationContext {
Expand Down Expand Up @@ -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<typeof useQueryClient>, tableId: string) {
Expand Down Expand Up @@ -1295,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
Expand Down
43 changes: 43 additions & 0 deletions apps/sim/hooks/queries/utils/table-rows-pagination.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,49 @@ 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 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)
})
})
})

describe('getNextTableRowsPageParam', () => {
Expand Down
26 changes: 19 additions & 7 deletions apps/sim/hooks/queries/utils/table-rows-pagination.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ export type TableRowsPageParam = number | TableRowsCursor
interface TableRowsPageLike {
rows: ReadonlyArray<{ id: string; orderKey?: string }>
totalCount: number | null
/**
* 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
}

/** Rows loaded across all fetched pages. */
Expand All @@ -17,18 +24,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
}
Expand Down
41 changes: 40 additions & 1 deletion apps/sim/lib/api/contracts/tables.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,46 @@
* @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`, 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', () => {
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', () => {
Expand Down
17 changes: 14 additions & 3 deletions apps/sim/lib/api/contracts/tables.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { isRecordLike } from '@sim/utils/object'
import { z } from 'zod'
import {
booleanQueryFlagSchema,
folderIdSchema,
privateSecretProvenanceBundleSchema,
requiredFieldSchema,
Expand Down Expand Up @@ -800,11 +801,21 @@ 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` — 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(
(value) =>
value === null || value === undefined || value === '' ? undefined : value === 'true',
z.boolean().optional()
(value) => (value === null || value === undefined || value === '' ? undefined : value),
booleanQueryFlagSchema.optional()
)
.default(true),
})
Expand Down
33 changes: 27 additions & 6 deletions apps/sim/lib/table/planner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,36 @@ export type DbTransaction = Parameters<Parameters<typeof db.transaction>[0]>[0]
const READ_STATEMENT_TIMEOUT_MS = 15_000
const READ_LOCK_TIMEOUT_MS = 3_000

async function setReadTimeouts(trx: DbTransaction): Promise<void> {
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 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, 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<void> {
/**
* 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
Expand All @@ -34,8 +56,7 @@ export async function withReadGuards<T>(
opts?: { seqscanOff?: boolean }
): Promise<T> {
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)
})
}
Expand Down
18 changes: 16 additions & 2 deletions apps/sim/lib/table/rows/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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

/**
Expand Down
Loading