Skip to content

Commit f237416

Browse files
committed
fix(tables): prioritize oldest TTL cleanup rows
Page cleanup by created_at and id, with a supporting index, so capped runs make progress on the oldest rows first.
1 parent b8758bd commit f237416

6 files changed

Lines changed: 20313 additions & 33 deletions

File tree

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

Lines changed: 58 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
11
/**
22
* @vitest-environment node
33
*/
4+
import type { SQL } from 'drizzle-orm'
5+
import { PgDialect } from 'drizzle-orm/pg-core'
46
import { beforeEach, describe, expect, it, vi } from 'vitest'
57

8+
vi.unmock('@sim/db/schema')
9+
vi.unmock('drizzle-orm')
10+
611
const {
712
mockDeleteExecute,
813
mockListExecute,
@@ -27,6 +32,8 @@ vi.mock('@/lib/table/service', () => ({ withLockedTable: mockWithLockedTable }))
2732

2833
import { cleanupTableRowTtlTask, runCleanupTableRowTtl } from '@/background/cleanup-table-row-ttl'
2934

35+
const dialect = new PgDialect()
36+
3037
const table = {
3138
id: 'table-1',
3239
workspaceId: 'workspace-1',
@@ -49,10 +56,14 @@ describe('table row TTL cleanup', () => {
4956
)
5057
})
5158

52-
it('deletes expired rows in locked, keyset batches and signals the table', async () => {
59+
it('deletes expired rows in locked, created-at keyset batches and signals the table', async () => {
5360
mockDeleteExecute
54-
.mockResolvedValueOnce([{ count: 500, lastId: 'row-500' }])
55-
.mockResolvedValueOnce([{ count: 12, lastId: 'row-512' }])
61+
.mockResolvedValueOnce([
62+
{ count: 500, createdAt: '2026-01-01T00:00:00.123456', lastId: 'row-500' },
63+
])
64+
.mockResolvedValueOnce([
65+
{ count: 12, createdAt: '2026-01-02T00:00:00.000000', lastId: 'row-512' },
66+
])
5667

5768
await expect(runCleanupTableRowTtl()).resolves.toEqual({
5869
batches: 2,
@@ -61,27 +72,58 @@ describe('table row TTL cleanup', () => {
6172
})
6273
expect(mockWithLockedTable).toHaveBeenCalledTimes(2)
6374
expect(mockDeleteExecute).toHaveBeenCalledTimes(2)
75+
const secondQuery = dialect.sqlToQuery(mockDeleteExecute.mock.calls[1][0] as SQL)
76+
expect(secondQuery.sql.replace(/\$\d+/g, '?').replace(/\s+/g, ' ')).toContain(
77+
'AND (table_row.created_at, table_row.id) > (?::timestamp, ?)'
78+
)
79+
expect(secondQuery.params).toEqual(
80+
expect.arrayContaining(['2026-01-01T00:00:00.123456', 'row-500'])
81+
)
6482
expect(mockSignalTableRowsChanged).toHaveBeenCalledWith(table.id)
6583
})
6684

6785
it('compares TTL values with whole Date.now epoch seconds', async () => {
6886
const nowEpochMilliseconds = 1_700_000_000_123
6987
const nowEpochSeconds = 1_700_000_000
7088
const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(nowEpochMilliseconds)
71-
mockDeleteExecute.mockResolvedValue([{ count: 0, lastId: null }])
89+
mockDeleteExecute.mockResolvedValue([{ count: 0, createdAt: null, lastId: null }])
7290

7391
try {
7492
await runCleanupTableRowTtl()
7593
} finally {
7694
nowSpy.mockRestore()
7795
}
7896

79-
expect(mockListExecute.mock.calls[0][0]).toMatchObject({
80-
values: expect.arrayContaining([nowEpochSeconds]),
81-
})
82-
expect(mockDeleteExecute.mock.calls[0][0]).toMatchObject({
83-
values: expect.arrayContaining([nowEpochSeconds]),
84-
})
97+
expect(dialect.sqlToQuery(mockListExecute.mock.calls[0][0] as SQL).params).toContain(
98+
nowEpochSeconds
99+
)
100+
expect(dialect.sqlToQuery(mockDeleteExecute.mock.calls[0][0] as SQL).params).toContain(
101+
nowEpochSeconds
102+
)
103+
})
104+
105+
it('checks the oldest expired rows first without using creation time as an expiry rule', async () => {
106+
mockDeleteExecute.mockResolvedValue([{ count: 0, createdAt: null, lastId: null }])
107+
108+
await runCleanupTableRowTtl()
109+
110+
const query = dialect
111+
.sqlToQuery(mockDeleteExecute.mock.calls[0][0] as SQL)
112+
.sql.replace(/\s+/g, ' ')
113+
.replace(/\$\d+/g, '?')
114+
.trim()
115+
expect(query).toContain('AND (table_row.data->>?)::numeric <= ?')
116+
expect(query).toContain('ORDER BY table_row.created_at, table_row.id')
117+
expect(query).toContain(`to_char(created_at, 'YYYY-MM-DD"T"HH24:MI:SS.US')`)
118+
expect(query).not.toContain('table_row.created_by')
119+
})
120+
121+
it('rejects a batch without a creation-time cursor', async () => {
122+
mockDeleteExecute.mockResolvedValue([{ count: 1, lastId: 'row-1' }])
123+
124+
await expect(runCleanupTableRowTtl()).rejects.toThrow(
125+
'Table row TTL cleanup did not return a creation-time cursor'
126+
)
85127
})
86128

87129
it('does no work when already aborted', async () => {
@@ -114,7 +156,9 @@ describe('table row TTL cleanup', () => {
114156
})
115157

116158
it('stops after one hundred full batches', async () => {
117-
mockDeleteExecute.mockResolvedValue([{ count: 500, lastId: 'row-cursor' }])
159+
mockDeleteExecute.mockResolvedValue([
160+
{ count: 500, createdAt: '2026-01-01T00:00:00.000000', lastId: 'row-cursor' },
161+
])
118162

119163
await expect(runCleanupTableRowTtl()).resolves.toEqual({
120164
batches: 100,
@@ -144,12 +188,12 @@ describe('table row TTL cleanup', () => {
144188
const attempt = (tableAttempts.get(tableId) ?? 0) + 1
145189
tableAttempts.set(tableId, attempt)
146190
if (tableId === table.id && attempt === 1) {
147-
return [{ count: 500, lastId: 'row-500' }]
191+
return [{ count: 500, createdAt: '2026-01-01T00:00:00.000000', lastId: 'row-500' }]
148192
}
149193
if (tableId === secondTable.id) {
150-
return [{ count: 1, lastId: 'row-1' }]
194+
return [{ count: 1, createdAt: '2026-01-01T00:00:00.000000', lastId: 'row-1' }]
151195
}
152-
return [{ count: 0, lastId: null }]
196+
return [{ count: 0, createdAt: null, lastId: null }]
153197
}),
154198
})
155199
})

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

Lines changed: 53 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,17 @@ interface ExpiredTtlTableRef {
2525
interface DeletedTtlBatch {
2626
attempted: boolean
2727
deleted: number
28-
lastId: string | null
28+
cursor: TtlCleanupCursor | null
29+
}
30+
31+
interface TtlCleanupCursor {
32+
createdAt: string
33+
id: string
2934
}
3035

3136
interface TtlTableCleanupState {
3237
ref: ExpiredTtlTableRef
33-
afterId?: string
38+
after?: TtlCleanupCursor
3439
deleted: number
3540
complete: boolean
3641
}
@@ -81,18 +86,33 @@ async function listExpiredTtlTables(nowEpochSeconds: number): Promise<ExpiredTtl
8186

8287
function parseDeletedBatch(rows: unknown): Omit<DeletedTtlBatch, 'attempted'> {
8388
const [row] = Array.isArray(rows)
84-
? (rows as Array<{ count?: number | string; lastId?: string | null }>)
89+
? (rows as Array<{
90+
count?: number | string
91+
createdAt?: string | null
92+
lastId?: string | null
93+
}>)
8594
: []
8695
if (!row) throw new Error('Table row TTL cleanup did not return a deleted count')
8796

8897
const deleted = Number(row.count)
8998
if (!Number.isSafeInteger(deleted) || deleted < 0 || deleted > TTL_CLEANUP_BATCH_SIZE) {
9099
throw new Error('Table row TTL cleanup returned an invalid deleted count')
91100
}
92-
if (deleted > 0 && typeof row.lastId !== 'string') {
93-
throw new Error('Table row TTL cleanup did not return a row cursor')
101+
if (deleted > 0) {
102+
if (typeof row.lastId !== 'string') {
103+
throw new Error('Table row TTL cleanup did not return a row cursor')
104+
}
105+
if (typeof row.createdAt !== 'string') {
106+
throw new Error('Table row TTL cleanup did not return a creation-time cursor')
107+
}
108+
}
109+
return {
110+
deleted,
111+
cursor:
112+
typeof row.createdAt === 'string' && typeof row.lastId === 'string'
113+
? { createdAt: row.createdAt, id: row.lastId }
114+
: null,
94115
}
95-
return { deleted, lastId: row.lastId ?? null }
96116
}
97117

98118
async function deleteExpiredTableRowBatch(
@@ -101,29 +121,43 @@ async function deleteExpiredTableRowBatch(
101121
workspaceId: string,
102122
columnKey: string,
103123
nowEpochSeconds: number,
104-
afterId?: string
124+
after?: TtlCleanupCursor
105125
): Promise<Omit<DeletedTtlBatch, 'attempted'>> {
106-
const rows = await trx.execute<{ count: number | string; lastId: string | null }>(sql`
126+
const rows = await trx.execute<{
127+
count: number | string
128+
createdAt: string | null
129+
lastId: string | null
130+
}>(sql`
107131
WITH candidates AS MATERIALIZED (
108132
SELECT table_row.id
109133
FROM ${userTableRows} AS table_row
110134
WHERE table_row.table_id = ${tableId}
111135
AND table_row.workspace_id = ${workspaceId}
112-
${afterId ? sql`AND table_row.id > ${afterId}` : sql``}
136+
${
137+
after
138+
? sql`AND (table_row.created_at, table_row.id) > (${after.createdAt}::timestamp, ${after.id})`
139+
: sql``
140+
}
113141
AND jsonb_typeof(table_row.data->${columnKey}) = 'number'
114142
AND (table_row.data->>${columnKey})::numeric <= ${nowEpochSeconds}
115-
ORDER BY table_row.id
143+
ORDER BY table_row.created_at, table_row.id
116144
LIMIT ${TTL_CLEANUP_BATCH_SIZE}
117145
FOR UPDATE OF table_row SKIP LOCKED
118146
), deleted AS (
119147
DELETE FROM ${userTableRows} AS table_row
120148
USING candidates
121149
WHERE table_row.id = candidates.id
122-
RETURNING table_row.id
150+
RETURNING table_row.id, table_row.created_at
123151
)
124152
SELECT
125153
count(*)::integer AS count,
126-
max(id) AS "lastId"
154+
(array_agg(id ORDER BY created_at DESC, id DESC))[1] AS "lastId",
155+
(
156+
array_agg(
157+
to_char(created_at, 'YYYY-MM-DD"T"HH24:MI:SS.US')
158+
ORDER BY created_at DESC, id DESC
159+
)
160+
)[1] AS "createdAt"
127161
FROM deleted
128162
`)
129163
return parseDeletedBatch(rows)
@@ -132,7 +166,7 @@ async function deleteExpiredTableRowBatch(
132166
async function deleteExpiredRowsForTable(
133167
ref: ExpiredTtlTableRef,
134168
nowEpochSeconds: number,
135-
afterId?: string
169+
after?: TtlCleanupCursor
136170
): Promise<DeletedTtlBatch> {
137171
try {
138172
return await withLockedTable(
@@ -142,29 +176,29 @@ async function deleteExpiredRowsForTable(
142176
assertRowDelete(table)
143177
} catch (error) {
144178
if (error instanceof TableLockedError) {
145-
return { attempted: false, deleted: 0, lastId: null }
179+
return { attempted: false, deleted: 0, cursor: null }
146180
}
147181
throw error
148182
}
149183

150184
const ttlColumn = table.schema.columns.find((column) => column.type === 'ttl')
151-
if (!ttlColumn) return { attempted: false, deleted: 0, lastId: null }
185+
if (!ttlColumn) return { attempted: false, deleted: 0, cursor: null }
152186

153187
const batch = await deleteExpiredTableRowBatch(
154188
trx,
155189
table.id,
156190
table.workspaceId,
157191
getColumnId(ttlColumn),
158192
nowEpochSeconds,
159-
afterId
193+
after
160194
)
161195
return { attempted: true, ...batch }
162196
},
163197
{ expectedWorkspaceId: ref.workspaceId }
164198
)
165199
} catch (error) {
166200
if (asOrchestrationError(error)?.code === 'not_found') {
167-
return { attempted: false, deleted: 0, lastId: null }
201+
return { attempted: false, deleted: 0, cursor: null }
168202
}
169203
throw error
170204
}
@@ -195,7 +229,7 @@ export async function runCleanupTableRowTtl(
195229
if (state.complete) continue
196230
if (batches === TTL_CLEANUP_MAX_BATCHES || signal?.aborted) break
197231

198-
const batch = await deleteExpiredRowsForTable(state.ref, nowEpochSeconds, state.afterId)
232+
const batch = await deleteExpiredRowsForTable(state.ref, nowEpochSeconds, state.after)
199233
if (!batch.attempted) {
200234
state.complete = true
201235
continue
@@ -204,7 +238,7 @@ export async function runCleanupTableRowTtl(
204238
batches++
205239
deleted += batch.deleted
206240
state.deleted += batch.deleted
207-
state.afterId = batch.lastId ?? undefined
241+
state.after = batch.cursor ?? undefined
208242
if (batch.deleted < TTL_CLEANUP_BATCH_SIZE) state.complete = true
209243
}
210244
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
-- Concurrent index operations cannot run inside the migration runner's transaction.
2+
COMMIT;--> statement-breakpoint
3+
SET lock_timeout = 0;--> statement-breakpoint
4+
-- migration-safe: replay removes an invalid build created by this migration; concurrent operations preserve row writes.
5+
DROP INDEX CONCURRENTLY IF EXISTS "user_table_rows_table_created_id_idx";--> statement-breakpoint
6+
CREATE INDEX CONCURRENTLY IF NOT EXISTS "user_table_rows_table_created_id_idx" ON "user_table_rows" USING btree ("table_id","created_at","id");--> statement-breakpoint
7+
SET lock_timeout = '5s';

0 commit comments

Comments
 (0)