Skip to content

Commit 2ba4556

Browse files
authored
fix(db): bind every raw-sql Date through its column encoder (#6337)
* fix(db): bind every raw-sql Date through its column encoder `drizzle()` overwrites postgres-js's temporal serializers (OIDs 1082/1083/ 1114/1184/1182/1185/1115/1231) with an identity function because drizzle maps timestamps itself through the column's `mapToDriverValue`. A raw `sql` template carries no column context, so an interpolated `Date` skips that mapping, reaches the identity serializer unchanged, and the wire encoder throws `ERR_INVALID_ARG_TYPE`. The pools' `prepare` / `fetch_types` options are irrelevant: the serializer swap happens for all four combinations. Five live sites still interpolated a bare `Date`, the stale schedule-job filter among them — it has no try/catch, so a database async backend would surface a 500 from the schedule tick. Bind each cutoff with `sql.param(date, column)`. The testing `sql` mock's guard cannot see untested code or the tests that override the drizzle-orm mock, so add `check:sql-date-binding`: a Babel-AST audit over apps/** and packages/** that resolves Date-valued bindings per file and rejects any that reach a raw template unbound. Correct the mock's comment, which attributed the failure to postgres-js under `fetch_types: false`. * fix(scripts): require the documented sql-date-bound annotation form and a reason
1 parent 8e3e608 commit 2ba4556

13 files changed

Lines changed: 442 additions & 23 deletions

File tree

.github/workflows/test-build.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,9 @@ jobs:
159159
- name: Tool request transport boundary audit
160160
run: bun run check:tool-request-boundary
161161

162+
- name: SQL Date binding audit
163+
run: bun run check:sql-date-binding
164+
162165
- name: Verify generated tool metadata is in sync
163166
run: bun run tool-metadata:check
164167

apps/sim/app/api/schedules/execute/route.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
* @vitest-environment node
55
*/
66
import {
7+
createMockSql,
78
dbChainMock,
89
dbChainMockFns,
910
requestUtilsMockFns,
@@ -102,7 +103,7 @@ vi.mock('drizzle-orm', () => ({
102103
isNull: vi.fn((field: unknown) => ({ type: 'isNull', field })),
103104
or: vi.fn((...conditions: unknown[]) => ({ type: 'or', conditions })),
104105
asc: vi.fn((field: unknown) => ({ type: 'asc', field })),
105-
sql: vi.fn((strings: unknown, ...values: unknown[]) => ({ type: 'sql', strings, values })),
106+
sql: createMockSql(),
106107
}))
107108

108109
vi.mock('@sim/db', () => ({

apps/sim/app/api/schedules/execute/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -401,7 +401,7 @@ function staleScheduleExecutionJobsFilter(now: Date) {
401401
THEN (${asyncJobs.payload} ->> 'executionTimeoutMs')::double precision / 1000 + ${cleanupGraceSeconds}
402402
ELSE ${legacyMaxDurationSeconds}
403403
END
404-
) * interval '1 second' <= ${now}`
404+
) * interval '1 second' <= ${sql.param(now, asyncJobs.startedAt)}`
405405
)
406406
)
407407
}

apps/sim/lib/data-drains/sources/cursor.test.ts

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
11
/**
22
* @vitest-environment node
33
*/
4+
import type { PgColumn } from 'drizzle-orm/pg-core'
45
import { describe, expect, it } from 'vitest'
5-
import { decodeTimeCursor, encodeTimeCursor } from '@/lib/data-drains/sources/cursor'
6+
import {
7+
decodeTimeCursor,
8+
encodeTimeCursor,
9+
timeCursorPredicate,
10+
} from '@/lib/data-drains/sources/cursor'
611

712
describe('time cursor encoding', () => {
813
it('round-trips a valid cursor', () => {
@@ -24,3 +29,24 @@ describe('time cursor encoding', () => {
2429
expect(decodeTimeCursor(JSON.stringify({}))).toBeNull()
2530
})
2631
})
32+
33+
describe('timeCursorPredicate', () => {
34+
const timestampCol = { name: 'created_at' } as unknown as PgColumn
35+
const idCol = { name: 'id' } as unknown as PgColumn
36+
37+
it('returns undefined without a cursor', () => {
38+
expect(timeCursorPredicate(timestampCol, idCol, null)).toBeUndefined()
39+
})
40+
41+
it('binds the cursor timestamp through the column encoder', () => {
42+
const predicate = timeCursorPredicate(timestampCol, idCol, {
43+
ts: '2026-01-01T00:00:00.000Z',
44+
id: 'row-1',
45+
}) as unknown as { values: unknown[] }
46+
47+
expect(predicate.values).not.toContainEqual(new Date('2026-01-01T00:00:00.000Z'))
48+
expect(predicate.values).toContainEqual(
49+
expect.objectContaining({ value: new Date('2026-01-01T00:00:00.000Z') })
50+
)
51+
})
52+
})

apps/sim/lib/data-drains/sources/cursor.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ export function timeCursorPredicate(
4444
cursor: TimeCursor | null
4545
): SQL | undefined {
4646
if (!cursor) return undefined
47-
return sql`(date_trunc('milliseconds', ${timestampCol}), ${idCol}) > (${new Date(cursor.ts)}, ${cursor.id})`
47+
return sql`(date_trunc('milliseconds', ${timestampCol}), ${idCol}) > (${sql.param(new Date(cursor.ts), timestampCol)}, ${cursor.id})`
4848
}
4949

5050
/**

apps/sim/lib/execution/remote-sandbox/image-registry.test.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
* guard down, plus the failure modes that must leave the retention sweep a job to
77
* finish rather than losing the image silently.
88
*/
9+
import { createMockSql } from '@sim/testing'
910
import { beforeEach, describe, expect, it, vi } from 'vitest'
1011

1112
const {
@@ -88,7 +89,7 @@ vi.mock('drizzle-orm', () => ({
8889
lt: (...args: unknown[]) => args,
8990
notInArray: (...args: unknown[]) => args,
9091
or: (...args: unknown[]) => args,
91-
sql: (...args: unknown[]) => args,
92+
sql: createMockSql(),
9293
}))
9394

9495
vi.mock('@/lib/execution/remote-sandbox/provider', () => ({
@@ -977,10 +978,16 @@ describe('cleanupSandboxImages', () => {
977978
})
978979
})
979980

980-
/** True when any leaf of the mocked predicate tree is a `Date`, i.e. a time bound. */
981+
/**
982+
* True when any leaf of the mocked predicate tree is a `Date`, i.e. a time bound.
983+
* Cutoffs are bound through `sql.param(date, column)`, so the walk descends into
984+
* the mock's fragment and param objects as well as condition arrays.
985+
*/
981986
function hasTimeBound(predicate: unknown): boolean {
982987
if (predicate instanceof Date) return true
983-
return Array.isArray(predicate) && predicate.some(hasTimeBound)
988+
if (Array.isArray(predicate)) return predicate.some(hasTimeBound)
989+
if (predicate && typeof predicate === 'object') return Object.values(predicate).some(hasTimeBound)
990+
return false
984991
}
985992

986993
/**

apps/sim/lib/execution/remote-sandbox/image-registry.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1003,6 +1003,7 @@ export async function cleanupSandboxImages(retentionDays: number): Promise<{
10031003
const images = provider.images
10041004

10051005
const cutoff = new Date(Date.now() - retentionDays * 24 * 60 * 60 * 1000)
1006+
const beyondRetention = sql`coalesce(${sandboxImage.lastUsedAt}, ${sandboxImage.createdAt}) < ${sql.param(cutoff, sandboxImage.lastUsedAt)}`
10061007
const stale = await db
10071008
.select({
10081009
id: sandboxImage.id,
@@ -1015,7 +1016,7 @@ export async function cleanupSandboxImages(retentionDays: number): Promise<{
10151016
.where(
10161017
and(
10171018
eq(sandboxImage.provider, provider.id),
1018-
sql`coalesce(${sandboxImage.lastUsedAt}, ${sandboxImage.createdAt}) < ${cutoff}`,
1019+
beyondRetention,
10191020
sql`not exists (select 1 from workspace_sandbox ws where ws.spec_hash = ${sandboxImage.specHash})`
10201021
)
10211022
)
@@ -1033,11 +1034,7 @@ export async function cleanupSandboxImages(retentionDays: number): Promise<{
10331034
for (let offset = 0; offset < stale.length; offset += CLEANUP_CONCURRENCY) {
10341035
const chunk = stale.slice(offset, offset + CLEANUP_CONCURRENCY)
10351036
const outcomes = await Promise.all(
1036-
chunk.map((row) =>
1037-
claimAndDeleteImage(provider.id, images, row.specHash, [
1038-
sql`coalesce(${sandboxImage.lastUsedAt}, ${sandboxImage.createdAt}) < ${cutoff}`,
1039-
])
1040-
)
1037+
chunk.map((row) => claimAndDeleteImage(provider.id, images, row.specHash, [beyondRetention]))
10411038
)
10421039

10431040
deleted += outcomes.filter((outcome) => outcome === 'released').length
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockOnConflictDoUpdate, mockReturning } = vi.hoisted(() => ({
7+
mockOnConflictDoUpdate: vi.fn(),
8+
mockReturning: vi.fn(),
9+
}))
10+
11+
vi.mock('@sim/db', () => ({
12+
db: {
13+
insert: () => ({
14+
values: () => ({ onConflictDoUpdate: mockOnConflictDoUpdate }),
15+
}),
16+
},
17+
}))
18+
19+
import { claimCooldown } from '@/lib/workspace-events/state'
20+
21+
describe('claimCooldown', () => {
22+
beforeEach(() => {
23+
vi.clearAllMocks()
24+
mockOnConflictDoUpdate.mockReturnValue({ returning: mockReturning })
25+
mockReturning.mockResolvedValue([{ workflowId: 'workflow-1' }])
26+
})
27+
28+
it('claims the slot when the upsert returns a row', async () => {
29+
await expect(claimCooldown('workflow-1', 'block-1', 'scope-1', 60_000)).resolves.toBe(true)
30+
})
31+
32+
it('declines the slot when the cooldown predicate matched nothing', async () => {
33+
mockReturning.mockResolvedValue([])
34+
await expect(claimCooldown('workflow-1', 'block-1', 'scope-1', 60_000)).resolves.toBe(false)
35+
})
36+
37+
it('binds the cooldown threshold through the column encoder', async () => {
38+
await claimCooldown('workflow-1', 'block-1', 'scope-1', 60_000)
39+
40+
const { setWhere } = mockOnConflictDoUpdate.mock.calls[0][0]
41+
expect(setWhere.values.some((value: unknown) => value instanceof Date)).toBe(false)
42+
expect(
43+
setWhere.values.some(
44+
(value: unknown) => (value as { value?: unknown } | null)?.value instanceof Date
45+
)
46+
).toBe(true)
47+
})
48+
})

apps/sim/lib/workspace-events/state.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ export async function claimCooldown(
5959
.onConflictDoUpdate({
6060
target: [simTriggerState.workflowId, simTriggerState.blockId, simTriggerState.scopeKey],
6161
set: { lastFiredAt: now, updatedAt: now },
62-
setWhere: sql`${simTriggerState.lastFiredAt} IS NULL OR ${simTriggerState.lastFiredAt} < ${threshold}`,
62+
setWhere: sql`${simTriggerState.lastFiredAt} IS NULL OR ${simTriggerState.lastFiredAt} < ${sql.param(threshold, simTriggerState.lastFiredAt)}`,
6363
})
6464
.returning({ workflowId: simTriggerState.workflowId })
6565

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
"check:realtime-prune": "bun run scripts/check-realtime-prune-graph.ts",
3434
"check:tool-request-boundary": "bun test scripts/check-tool-request-boundary.test.ts && bun run scripts/check-tool-request-boundary.ts",
3535
"check:tool-registry-boundary": "bun run scripts/check-tool-registry-boundary.ts",
36+
"check:sql-date-binding": "bun test scripts/check-sql-date-binding.test.ts && bun run scripts/check-sql-date-binding.ts",
3637
"check:zustand-v5": "bun run scripts/check-zustand-v5-selectors.ts",
3738
"check:react-query": "bun run scripts/check-react-query-patterns.ts --check",
3839
"check:client-boundary": "bun run scripts/check-client-boundary-imports.ts --check",

0 commit comments

Comments
 (0)