Skip to content

Commit bd53d8e

Browse files
committed
fix(tables): bound delete trigger snapshots
1 parent 0f31976 commit bd53d8e

9 files changed

Lines changed: 184 additions & 52 deletions

File tree

apps/sim/background/cleanup-table-row-ttl.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ vi.mock('@sim/db', () => ({
3232

3333
vi.mock('@trigger.dev/sdk', () => ({ task: mockTask }))
3434
vi.mock('@/lib/table/events', () => ({ signalTableRowsChanged: mockSignalTableRowsChanged }))
35+
vi.mock('@/lib/table/constants', () => ({ getDeleteSnapshotBatchSize: () => 500 }))
3536
vi.mock('@/lib/table/service', () => ({ withLockedTable: mockWithLockedTable }))
3637
vi.mock('@/lib/table/ttl-availability', () => ({
3738
isTableRowTtlEnabled: mockIsTableRowTtlEnabled,

apps/sim/background/cleanup-table-row-ttl.ts

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { task } from '@trigger.dev/sdk'
55
import { sql } from 'drizzle-orm'
66
import { asOrchestrationError } from '@/lib/core/orchestration/types'
77
import { getColumnId } from '@/lib/table/column-keys'
8+
import { getDeleteSnapshotBatchSize } from '@/lib/table/constants'
89
import { signalTableRowsChanged } from '@/lib/table/events'
910
import { assertRowDelete, TableLockedError } from '@/lib/table/mutation-locks'
1011
import type { DbTransaction } from '@/lib/table/planner'
@@ -17,7 +18,6 @@ import type { RowData, TableSchema } from '@/lib/table/types'
1718
const logger = createLogger('CleanupTableRowTtl')
1819
const cleanupDb = dbFor('cleanup')
1920

20-
const TTL_CLEANUP_BATCH_SIZE = 500
2121
const TTL_CLEANUP_MAX_BATCHES = 100
2222

2323
interface ExpiredTtlTableRef {
@@ -96,12 +96,12 @@ async function listExpiredTtlTables(nowEpochSeconds: number): Promise<ExpiredTtl
9696
return Array.isArray(rows) ? rows : []
9797
}
9898

99-
function parseDeletedBatch(rows: unknown): DeletedTtlRows {
99+
function parseDeletedBatch(rows: unknown, batchSize: number): DeletedTtlRows {
100100
if (!Array.isArray(rows)) {
101101
throw new Error('Table row TTL cleanup did not return deleted rows')
102102
}
103103
const deletedRows = rows as Array<{ id?: unknown; data?: unknown; createdAt?: unknown }>
104-
if (deletedRows.length > TTL_CLEANUP_BATCH_SIZE) {
104+
if (deletedRows.length > batchSize) {
105105
throw new Error('Table row TTL cleanup returned an invalid deleted count')
106106
}
107107
const parsed = deletedRows.map((row) => {
@@ -129,6 +129,7 @@ async function deleteExpiredTableRowBatch(
129129
workspaceId: string,
130130
columnKey: string,
131131
nowEpochSeconds: number,
132+
batchSize: number,
132133
after?: TtlCleanupCursor
133134
): Promise<DeletedTtlRows> {
134135
const rows = await trx.execute<{ id: string; data: RowData; createdAt: string }>(sql`
@@ -145,7 +146,7 @@ async function deleteExpiredTableRowBatch(
145146
AND jsonb_typeof(table_row.data->${columnKey}) = 'number'
146147
AND (table_row.data->>${columnKey})::numeric <= ${nowEpochSeconds}
147148
ORDER BY table_row.created_at, table_row.id
148-
LIMIT ${TTL_CLEANUP_BATCH_SIZE}
149+
LIMIT ${batchSize}
149150
FOR UPDATE OF table_row SKIP LOCKED
150151
), deleted AS (
151152
DELETE FROM ${userTableRows} AS table_row
@@ -160,12 +161,13 @@ async function deleteExpiredTableRowBatch(
160161
FROM deleted
161162
ORDER BY "createdAt", id
162163
`)
163-
return parseDeletedBatch(rows)
164+
return parseDeletedBatch(rows, batchSize)
164165
}
165166

166167
async function deleteExpiredRowsForTable(
167168
ref: ExpiredTtlTableRef,
168169
nowEpochSeconds: number,
170+
batchSize: number,
169171
after?: TtlCleanupCursor
170172
): Promise<DeletedTtlBatch> {
171173
try {
@@ -190,6 +192,7 @@ async function deleteExpiredRowsForTable(
190192
table.workspaceId,
191193
getColumnId(ttlColumn),
192194
nowEpochSeconds,
195+
batchSize,
193196
after
194197
)
195198
return {
@@ -202,7 +205,7 @@ async function deleteExpiredRowsForTable(
202205
{ expectedWorkspaceId: ref.workspaceId }
203206
)
204207
if (batch.attempted && batch.rows.length > 0) {
205-
void fireTableTrigger(
208+
await fireTableTrigger(
206209
ref.id,
207210
batch.tableName,
208211
'delete',
@@ -232,6 +235,7 @@ export async function runCleanupTableRowTtl(
232235
}
233236

234237
const nowEpochSeconds = Math.floor(Date.now() / 1000)
238+
const batchSize = getDeleteSnapshotBatchSize()
235239
const tableRefs = await listExpiredTtlTables(nowEpochSeconds)
236240
const tableStates: TtlTableCleanupState[] = tableRefs.map((ref) => ({
237241
ref,
@@ -250,7 +254,12 @@ export async function runCleanupTableRowTtl(
250254
if (state.complete) continue
251255
if (batches === TTL_CLEANUP_MAX_BATCHES || signal?.aborted) break
252256

253-
const batch = await deleteExpiredRowsForTable(state.ref, nowEpochSeconds, state.after)
257+
const batch = await deleteExpiredRowsForTable(
258+
state.ref,
259+
nowEpochSeconds,
260+
batchSize,
261+
state.after
262+
)
254263
if (!batch.attempted) {
255264
state.complete = true
256265
continue
@@ -260,7 +269,7 @@ export async function runCleanupTableRowTtl(
260269
deleted += batch.deleted
261270
state.deleted += batch.deleted
262271
state.after = batch.cursor ?? undefined
263-
if (batch.deleted < TTL_CLEANUP_BATCH_SIZE) state.complete = true
272+
if (batch.deleted < batchSize) state.complete = true
264273
}
265274
}
266275

apps/sim/lib/table/__tests__/service-filter-threading.test.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,31 @@ describe('delete trigger dispatch', () => {
234234
'req-delete-many'
235235
)
236236
})
237+
238+
it('dispatches byte-bounded ID-delete snapshots before loading the next batch', async () => {
239+
setEnv({
240+
TABLE_MAX_ROW_SIZE_BYTES: TABLE_LIMITS.DELETE_SNAPSHOT_BATCH_MAX_BYTES * 2,
241+
})
242+
dbChainMockFns.returning
243+
.mockResolvedValueOnce([{ id: 'row-1', data: { name: 'Ada' } }])
244+
.mockResolvedValueOnce([{ id: 'row-2', data: { name: 'Grace' } }])
245+
246+
try {
247+
await deleteRowsByIds(
248+
TABLE,
249+
{ tableId: TABLE.id, workspaceId: TABLE.workspaceId, rowIds: ['row-1', 'row-2'] },
250+
'req-delete-bounded'
251+
)
252+
} finally {
253+
setEnv({ TABLE_MAX_ROW_SIZE_BYTES: undefined })
254+
}
255+
256+
expect(mockFireTableTrigger).toHaveBeenCalledTimes(2)
257+
expect(mockFireTableTrigger.mock.calls[0][3]).toEqual([{ id: 'row-1', data: { name: 'Ada' } }])
258+
expect(mockFireTableTrigger.mock.calls[1][3]).toEqual([
259+
{ id: 'row-2', data: { name: 'Grace' } },
260+
])
261+
})
237262
})
238263

239264
describe('bulk update/delete limited-subset ordering', () => {

apps/sim/lib/table/constants.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ declare module '@/lib/table/constants?constants-test' {
3939

4040
import {
4141
getBillingDisabledTableLimits,
42+
getDeleteSnapshotBatchSize,
4243
getMaxPageBytes,
4344
TABLE_LIMITS,
4445
} from '@/lib/table/constants?constants-test'
@@ -86,3 +87,23 @@ describe('getMaxPageBytes', () => {
8687
expect(getMaxPageBytes()).toBe(2 * 1024 * 1024)
8788
})
8889
})
90+
91+
describe('getDeleteSnapshotBatchSize', () => {
92+
beforeEach(() => {
93+
for (const key of Object.keys(mockEnv)) delete mockEnv[key]
94+
})
95+
96+
it('derives a worst-case row cap from the delete snapshot byte budget', () => {
97+
expect(getDeleteSnapshotBatchSize()).toBe(
98+
Math.floor(TABLE_LIMITS.DELETE_SNAPSHOT_BATCH_MAX_BYTES / TABLE_LIMITS.MAX_ROW_SIZE_BYTES)
99+
)
100+
})
101+
102+
it('always processes one row and never exceeds the delete row-count cap', () => {
103+
mockEnv.TABLE_MAX_ROW_SIZE_BYTES = String(TABLE_LIMITS.DELETE_SNAPSHOT_BATCH_MAX_BYTES * 2)
104+
expect(getDeleteSnapshotBatchSize()).toBe(1)
105+
106+
mockEnv.TABLE_MAX_ROW_SIZE_BYTES = '1'
107+
expect(getDeleteSnapshotBatchSize()).toBe(TABLE_LIMITS.DELETE_BATCH_SIZE)
108+
})
109+
})

apps/sim/lib/table/constants.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@ export const TABLE_LIMITS = {
3939
UPDATE_BATCH_SIZE: 100,
4040
/** Batch size for bulk delete operations */
4141
DELETE_BATCH_SIZE: 1000,
42+
/**
43+
* Maximum serialized row-data bytes returned from one committed delete for
44+
* post-commit trigger dispatch. The row-count cap is derived from this budget
45+
* and the configured maximum row size before a DELETE materializes snapshots.
46+
*/
47+
DELETE_SNAPSHOT_BATCH_MAX_BYTES: 32 * 1024 * 1024, // 32MB
4248
/** Maximum rows per batch insert */
4349
MAX_BATCH_INSERT_SIZE: 1000,
4450
/** Maximum rows per bulk update/delete operation */
@@ -149,6 +155,21 @@ export function getMaxRowSizeBytes(): number {
149155
})
150156
}
151157

158+
/**
159+
* Maximum rows one delete may materialize with their JSON data for trigger
160+
* dispatch. Uses the worst-case configured row size so every batch has an
161+
* explicit byte bound before PostgreSQL returns it to the app process.
162+
*/
163+
export function getDeleteSnapshotBatchSize(): number {
164+
return Math.max(
165+
1,
166+
Math.min(
167+
TABLE_LIMITS.DELETE_BATCH_SIZE,
168+
Math.floor(TABLE_LIMITS.DELETE_SNAPSHOT_BATCH_MAX_BYTES / getMaxRowSizeBytes())
169+
)
170+
)
171+
}
172+
152173
export type PlanName = keyof typeof DEFAULT_TABLE_PLAN_LIMITS
153174

154175
export interface TablePlanLimits {

apps/sim/lib/table/delete-runner.test.ts

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,17 +87,20 @@ describe('runTableDelete', () => {
8787
mockMarkJobReady.mockResolvedValue(true)
8888
mockMarkJobFailed.mockResolvedValue(undefined)
8989
mockDeletePageByIds.mockImplementation(
90-
(
90+
async (
9191
_t,
9292
_w,
9393
ids: string[],
9494
_proof,
9595
_revalidate,
96-
onDeleted?: (rows: Array<{ id: string; data: Record<string, unknown> }>) => void
96+
onDeleted?: (
97+
rows: Array<{ id: string; data: Record<string, unknown> }>,
98+
table?: typeof table
99+
) => void | Promise<void>
97100
) => {
98101
const rows = ids.map((id) => ({ id, data: { title: id } }))
99-
onDeleted?.(rows)
100-
return Promise.resolve(rows.length)
102+
await onDeleted?.(rows)
103+
return rows.length
101104
}
102105
)
103106
mockBuildFilterClause.mockReturnValue({})
@@ -208,6 +211,34 @@ describe('runTableDelete', () => {
208211
expect(mockSignalTableRowsChanged).toHaveBeenCalledWith('tbl_1')
209212
})
210213

214+
it('uses the table definition revalidated with each committed delete batch', async () => {
215+
const renamedTable = {
216+
...table,
217+
name: 'Renamed issues',
218+
schema: { columns: [{ id: 'col-title', name: 'Renamed title', type: 'string' }] },
219+
}
220+
mockSelectRowIdPage.mockResolvedValueOnce(['a']).mockResolvedValueOnce([])
221+
mockDeletePageByIds.mockImplementationOnce(
222+
async (_t, _w, ids: string[], _proof, _revalidate, onDeleted) => {
223+
const rows = ids.map((id) => ({ id, data: { 'col-title': id } }))
224+
await onDeleted?.(rows, renamedTable)
225+
return rows.length
226+
}
227+
)
228+
229+
await runTableDelete(basePayload())
230+
231+
expect(mockFireTableTrigger).toHaveBeenCalledWith(
232+
renamedTable.id,
233+
renamedTable.name,
234+
'delete',
235+
[{ id: 'a', data: { 'col-title': 'a' } }],
236+
null,
237+
renamedTable.schema,
238+
expect.any(String)
239+
)
240+
})
241+
211242
it('stops once maxRows is reached and caps the final page fetch to the remaining budget', async () => {
212243
// budget 3 with page size 2: first page fills 2, the second is capped to the remaining 1.
213244
mockSelectRowIdPage.mockResolvedValueOnce(['a', 'b']).mockResolvedValueOnce(['c'])

apps/sim/lib/table/delete-runner.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,20 @@ export async function runTableDelete(payload: TableDeletePayload): Promise<void>
123123
// an absent filter is still legitimate (delete-all is an explicit caller mode).
124124
if (filter && !filterClause) throw new Error('Filter is required for bulk delete')
125125
const excluded = new Set(excludeRowIds ?? [])
126-
const dispatchDeleteTriggers = (rows: DeletedTableRow[]) => {
127-
void fireTableTrigger(table.id, table.name, 'delete', rows, null, table.schema, requestId)
126+
const dispatchDeleteTriggers = async (
127+
rows: DeletedTableRow[],
128+
committedTable?: TableDefinition
129+
) => {
130+
const triggerTable = committedTable ?? table
131+
await fireTableTrigger(
132+
triggerTable.id,
133+
triggerTable.name,
134+
'delete',
135+
rows,
136+
null,
137+
triggerTable.schema,
138+
requestId
139+
)
128140
}
129141

130142
// Resume the persisted count: a retried attempt's earlier batches are already committed,

0 commit comments

Comments
 (0)