Skip to content

fix(db): bind every raw-sql Date through its column encoder - #6337

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/sql-date-binding
Aug 6, 2026
Merged

fix(db): bind every raw-sql Date through its column encoder#6337
waleedlatif1 merged 2 commits into
stagingfrom
fix/sql-date-binding

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Why

#6327 fixed one instance of a Date interpolated into a raw drizzle sql template, but its stated mechanism was wrong and its coverage claim was incomplete.

Real mechanism (verified).drizzle() (drizzle-orm/postgres-js/driver) overwrites client.options.serializers for the temporal OIDs 1082/1083/1114/1184/1182/1185/1115/1231 with an identity function, because drizzle normally maps timestamps itself via the column's PgTimestamp.mapToDriverValue. A raw sql template carries no column context, so an interpolated Date skips that mapping, hits the now-identity serializer unchanged, and the wire encoder throws ERR_INVALID_ARG_TYPE.

A 2x2 matrix over prepare x fetch_types shows the pool options are irrelevant — in all four combinations postgres-js's own serializer produces the ISO string before drizzle(), and a raw Date after:

prepare=true fetch_types=true before=string:2026-08-06T00:00:00.000Z after=Date object (UNSERIALIZED)
prepare=true fetch_types=false before=string:2026-08-06T00:00:00.000Z after=Date object (UNSERIALIZED)
prepare=false fetch_types=true before=string:2026-08-06T00:00:00.000Z after=Date object (UNSERIALIZED)
prepare=false fetch_types=false before=string:2026-08-06T00:00:00.000Z after=Date object (UNSERIALIZED)
wire encode of a Date -> ERR_INVALID_ARG_TYPE

Sites fixed

SiteNotes
app/api/schedules/execute/route.ts (staleScheduleExecutionJobsFilter)Highest impact. Gated behind getAsyncBackendType() === 'database', and has no try/catch — a throw propagates a 500 from the schedule tick.
lib/data-drains/sources/cursor.ts (timeCursorPredicate)Cursor pagination predicate; the bound column is the caller's timestampCol.
lib/execution/remote-sandbox/image-registry.ts (x2, candidate query + claim guard)Duplicate predicate — hoisted to one beyondRetention fragment reused by both.
lib/workspace-events/state.ts (claimCooldown)Upsert setWhere; a throw here would be silently swallowed by the caller.

Each now binds with sql.param(date, table.column), matching the sibling usage in cleanup-stale-executions.

Detection that actually works

The packages/testingsql mock guard cannot be relied on: it is invisible to untested code, and 26 test files override the drizzle-orm mock (two of the sites above had passing tests). Added bun run check:sql-date-binding, following the check:tool-request-boundary pattern (Babel AST + exported pure function + bun:test unit test), wired into package.json and the test-build workflow.

It parses every .ts/.tsx under apps/** and packages/** (12.9k files, ~10s), resolves the set of Date-valued bindings per file to a fixed point (new Date(...), : Date / Date | null annotations on variables, parameters and property signatures, and const b = a chains regardless of declaration order), then rejects:

  • a resolved Date interpolated into a sql`…` or sql<T>`…` template
  • sql.param(date) with no encoder argument

Escape hatch: // sql-date-bound: <reason> on the preceding line. Not covered: a Date arriving through a cross-file call whose type is only known to the type checker (the pass is per-file), and non-Date values needing an encoder (arrays remain covered only by the test mock).

Tests

  • scripts/check-sql-date-binding.test.ts — every unbound form the audit must reject, plus bound params, non-Date interpolations, a non-sql tag, and the annotation.
  • lib/workspace-events/state.test.ts — new file; claimCooldown had no coverage at all.
  • lib/data-drains/sources/cursor.test.tstimeCursorPredicate was untested.
  • app/api/schedules/execute/route.test.ts and image-registry.test.ts now compose createMockSql() instead of hand-rolled sql stubs, so the shared guard applies to them.

Red-then-green verified: reverting the sql.param at each of the four files turns the corresponding tests red (18 failures across schedules/image-registry, 4 across cursor/state) and the audit reports all five sites.

Also corrected the misleading comment in packages/testing/src/mocks/database.mock.ts, which blamed postgres-js under fetch_types: false, and noted the guard is a backstop rather than the gate.

Verification

  • bun run check:sql-date-binding — clean
  • cd apps/sim && bunx tsc --noEmit -p tsconfig.json — clean
  • bun run lint / bun run check — clean (9 pre-existing warnings in zoho-desk.test.ts)

`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`.
@waleedlatif1
waleedlatif1 requested a review from a team as a code ownerAugust 6, 2026 18:52
@vercel

vercelBot commented Aug 6, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

ProjectDeploymentActionsUpdated (UTC)
docsReadyReadyPreviewAug 6, 2026 7:14pm

Request Review

@cursor

cursorBot commented Aug 6, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches schedule execution, async job cleanup, data-drain pagination, and sandbox retention predicates—incorrect binding could skew cutoffs or break ticks, but changes follow an established pattern and are guarded by a new static audit.

Overview
Fixes runtime failures from unserialized Date values in raw Drizzle sql templates: Drizzle replaces postgres-js temporal serializers with identity, so interpolated Dates skip column encoding and can throw ERR_INVALID_ARG_TYPE at the wire layer.

Production fixes switch four call sites to sql.param(date, table.column)—schedule stale-job cleanup (staleScheduleExecutionJobsFilter), data-drain timeCursorPredicate, sandbox image retention (beyondRetention, shared between candidate query and claim), and workspace-event claimCooldown upsert setWhere.

Prevention adds bun run check:sql-date-binding: a Babel AST scan of apps/** and packages/** that flags unbound Date interpolations in sql templates and one-arg sql.param(date), with an optional // sql-date-bound: <reason> escape. Wired into package.json and the test-build workflow.

Tests add audit unit tests, new claimCooldown and timeCursorPredicate coverage, and route/image-registry mocks now use createMockSql() so the shared guard applies; testing mock comments clarify the guard is a backstop, not the repo-wide gate.

Reviewed by Cursor Bugbot for commit 06d8f38. Configure here.

@greptile-apps

greptile-appsBot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR binds raw-SQL Date values through their Drizzle column encoders and adds a repository-wide static audit to prevent regressions.

  • Updates five temporal SQL parameters across schedule execution, cursor pagination, sandbox-image cleanup, and workspace-event cooldown claims.
  • Adds focused tests, strengthens shared SQL mocks, and runs the new audit in CI.
  • Corrects the previously reported annotation escape hatch so only a documented comment with a nonempty reason suppresses a violation.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

FilenameOverview
scripts/check-sql-date-binding.tsAdds the AST-based Date-binding audit and correctly restricts opt-out annotations to the documented comment form with a nonempty reason.
scripts/check-sql-date-binding.test.tsCovers unbound Date detection, valid column-bound parameters, and malformed or incidental annotation markers.
apps/sim/app/api/schedules/execute/route.tsEncodes the stale-job cutoff through the started-at timestamp column.
apps/sim/lib/data-drains/sources/cursor.tsEncodes cursor timestamps through the caller-provided timestamp column.
apps/sim/lib/execution/remote-sandbox/image-registry.tsReuses one correctly encoded retention predicate for candidate selection and deletion claims.
apps/sim/lib/workspace-events/state.tsEncodes the cooldown threshold through the last-fired-at timestamp column.
packages/testing/src/mocks/database.mock.tsClarifies the serialization failure and retains mock-level guards as a testing backstop.
.github/workflows/test-build.ymlAdds the SQL Date-binding audit to the test-build workflow.

Reviews (2): Last reviewed commit: "fix(scripts): require the documented sql..." | Re-trigger Greptile

Comment threadscripts/check-sql-date-binding.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@greptile

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@cursor review

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 06d8f38. Configure here.

@waleedlatif1
waleedlatif1 merged commit 2ba4556 into stagingAug 6, 2026
5 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/sql-date-binding branch August 6, 2026 19:16
@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

Post-merge correction — independent verification found three claims in this description are wrong. The fix itself is confirmed correct; these corrections make it look more valuable, not less.

1. image-registry.ts does NOT have "zero scheduled cron." It has a 30 4 * * * entry in docker/crontab:46 and an enabled Helm CronJob cleanup-sandbox-images (values.yaml:1441-1449), documented in background-jobs.mdx:57. It is a no-op only when the sandbox provider isn't prebuilt.

2. schedules/execute is not gated off — that is Cloud-only.database is the default async backend (env-capabilities.ts:1105-1122; trigger-dev requires TRIGGER_DEV_ENABLED + TRIGGER_PROJECT_ID + TRIGGER_SECRET_KEY), and no Helm or Docker manifest sets those. This was a live bug on every self-hosted deployment.

3. "Failed on every run" is refuted as stated. 13 runs were scheduled in the window: 11 logged failures, 0 successes, and 2 logged nothing at all. The failure was also partial — the job still completed its Deleted N old async jobs leg every time; only the async_jobs stale-select broke.

Also: the production log contains no Postgres error — only Drizzle's Failed query: wrapper plus bound params. The logs show which query failed, not why.

Confirmed by the same verification: a live 2×2 matrix against real PostgreSQL proved prepare/fetch_types are irrelevant and drizzle's serializer overwrite is the sole cause; reverting each sql.param individually produced exactly 14/4/1/3 failures as claimed (82/82 green restored); and reverting the already-merged #6327 fix showed this detector would have caught the original outage.

Two follow-ups worth a separate PR (detector gaps, not regressions):

  • isDateExpression has no MemberExpression case, so sql\… ${opts.since}`/${row.createdAt}` are invisible to it. A repo sweep found no current bug hiding there, but that is the most likely shape of the next instance.
  • The tag is matched by the bare identifier sql, which cannot distinguish postgres-js's own client tag. packages/db/scripts/reconcile-workspace-storage.ts:21 already declares const sql = postgres(url, …) — a tag that serializes Dates correctly — and passes today only because it happens never to interpolate a Date.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@waleedlatif1