Skip to content

fix(db): encode Date binds in raw sql, document the fetch_types contract - #6040

Open
TheodoreSpeaks wants to merge 2 commits into
stagingfrom
fix/raw-sql-date-binds
Open

fix(db): encode Date binds in raw sql, document the fetch_types contract#6040
TheodoreSpeaks wants to merge 2 commits into
stagingfrom
fix/raw-sql-date-binds

Conversation

@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #6035 and the fetch_types audit. Last night's staging cleanup run failed one layer deeper: statements 1–2 of pruneLargeValueMetadata now succeed, and statement 3 (tombstones) — which had never executed before because every prior run died earlier — failed with ERR_INVALID_ARG_TYPE.

  • Root cause: a Date bound into a raw drizzle sql template has no encoder, so postgres-js's unsafe path receives the Date object and dies in Buffer.byteLength. This is a Node-side failure, independent of fetch_types (reproduced under both settings), and sql.param(date) without an encoder fails identically. The repo already had the correct idiom with a comment describing this exact failure (lib/global-work/summary.ts): bind through the matching column, sql.param(date, table.timestampColumn)
  • Fixed the tombstone prune (deleted_at < ${sql.param(deletedBefore, executionLargeValues.deletedAt)})
  • Fixed the same class in v1 logs cursor pagination (app/api/v1/logs/filters.ts — both ${cursorDate} binds): any cursor-paginated GET /api/v1/logs request throws. Reproduced end-to-end through a query builder against staging; zero prod occurrences in 3 days, so the path is apparently unexercised — but it's a live public-API bug dating to ~v0.4.8
  • Rewrote the packages/db/db.ts fetch_types docstring: the old text ("builds array parsers only", "the one behavior this changes") is what misled fix(cleanup): bind array params as arrays, not expanded value lists #6010 into shipping broken — it now states both directions, all array types, the safe forms, and points at the pinning test
  • Added a comment to packages/db/scripts/migrate.ts documenting why it must not inherit fetch_types: false: script migrations bind JS arrays through postgres-js's own tag, and copying the app's pool options there fails every script migration and blocks startup on every deploy and fresh self-hosted install
  • Hardened the shared testing mock: encoderless sql.param(array) and sql.param(date) now throw with pointers to the safe forms, turning every mocked sql site's tests into detectors for both classes
  • Strengthened the render tests: prune-metadata-sql.test.ts now asserts every bind is a scalar (no arrays, no Dates); added the missing scalar-params assertion to live-paused-statuses-sql.test.ts; new config-sql.test.ts renders the polling key-removal expression for real (its existing test passed against both previously-broken forms)

Type of Change

  • Bug fix

Testing

  • Executed the real pruneLargeValueMetadata end-to-end against local Postgres with the exact production client options (prepare: false, fetch_types: false): all 3 statements ran with real DELETEs, and a seeded 60-day-old tombstone was actually pruned (tombstonesDeleted: 1)
  • EXPLAINed all 3 rendered statements against staging through the cleanup bouncer: 3 planned, 0 failed, all params scalar strings
  • Reproduced the v1 cursor failure through a real query builder against staging before fixing; mutation-tested the new scalar-params assertion (fails on the bare-Date form, passes on the fix)
  • 124 tests across payloads, polling, v1 logs, cleanup pass; tsc --noEmit clean; lint + full audit suite pass

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Jul 29, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 29, 2026 2:46am

Request Review

@cursor

cursor Bot commented Jul 29, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches cleanup pruning, public v1 logs pagination, and DB pool documentation; fixes are narrow encoding changes with strong test coverage, but failures would affect background cleanup and cursor-paginated log queries.

Overview
Fixes runtime failures when raw Drizzle sql templates bind Date values without a column encoder—postgres-js cannot serialize bare Date objects (ERR_INVALID_ARG_TYPE). Large-value tombstone pruning (deleted_at < …) and v1 logs cursor pagination (both asc/desc tuple compares) now use sql.param(date, matchingColumn).

Documents the fetch_types: false contract in packages/db/db.ts (array read/write behavior, safe vs unsafe bind patterns) and notes in migrate.ts that script migrations must not copy that option because they bind JS arrays via postgres-js’s own tag.

Test hardening: render tests assert scalar-only bind params (no arrays, no Date instances); new config-sql.test.ts for webhook provider-config key removal; the shared sql.param mock throws on encoderless array/Date binds so unit tests catch the same bug class.

Reviewed by Cursor Bugbot for commit fb92941. Bugbot is set up for automated code reviews on this repo. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Fixes Date binds in raw drizzle sql under fetch_types: false, and documents the contract.

  • Tombstone prune and v1 logs cursor pagination bind Dates via sql.param(date, timestampColumn).
  • Expands packages/db/db.ts docs for read/write array behavior; notes migrate script must keep default fetch_types.
  • Hardens testing mock to reject encoderless sql.param(array|date); strengthens render tests (prune, live-paused, config key-removal).

Confidence Score: 5/5

Safe to merge; the Date-encoding fixes match the established column-encoder pattern and are covered by real-dialect render tests.

Production paths bind timestamps through matching schema columns; docs and mock guards reduce recurrence of array/Date raw-sql failures under the app pool options; no remaining defect in the changed lines.

Important Files Changed

Filename Overview
apps/sim/lib/execution/payloads/large-value-metadata.ts Tombstone prune binds deletedBefore through executionLargeValues.deletedAt; empty workspace-id guards clarified as intentional no-ops.
apps/sim/app/api/v1/logs/filters.ts Cursor comparisons encode cursorDate via workflowExecutionLogs.startedAt, matching the established timestamp-bind idiom.
packages/db/db.ts Docstring corrected for bidirectional array type-map effects of fetch_types: false and safe raw-SQL patterns.
packages/db/scripts/migrate.ts Documents why migration client must not copy app pool fetch_types: false (postgres-js tag array binds).
packages/testing/src/mocks/database.mock.ts Mock sql.param throws on encoderless arrays/Dates so unit tests surface the same failure classes as production pools.
apps/sim/lib/execution/payloads/prune-metadata-sql.test.ts Render assertions require scalar binds only (no arrays, no Dates) via real PgDialect.
apps/sim/lib/webhooks/polling/config-sql.test.ts New real-dialect test locks key-removal to scalar ARRAY elements, not a single array param.

Reviews (1): Last reviewed commit: "fix(db): encode Date binds in raw sql, d..." | Re-trigger Greptile

Sign up for free to 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