sql: serialize Date parameters as ISO 8601 in Postgres text format

[robobun]

Fixes #29010.

Repro

import { SQL } from 'bun';
const sql = new SQL('postgres://postgres@localhost:5432/testdb', { prepare: false });
const t = new Date('2024-01-15T12:30:45.000Z');
await sql`SELECT * FROM my_table WHERE created_at = ${t}`;
// PostgresError: invalid input syntax for type timestamp with time zone:
//   "Mon Jan 15 2024 12:30:45 GMT+0000 (Coordinated Universal Time)"

Cause

In PostgresRequest.zig#writeBind, the switch (tag) over the
parameter type only has binary-format cases for .timestamp /
.timestamptz. Any other tag — including 0 ("server decides",
which is what Signature.generate writes for dates so Postgres
picks the type, and what any prepare: false query sends) — falls
through to:

else => {
    const str = try String.fromJS(value, globalObject);
    ...
}

String.fromJS on a JS Date triggers the ordinary JS
toString() conversion, which for dates produces a locale /
timezone-dependent string that PostgreSQL cannot parse.

The prepared-statement binary path was already correct because
types.date.fromJS uses value.getUnixTimestamp().

Fix

1. In the else branch of writeBind, detect value.isDate() and
   emit the ISO 8601 / RFC 3339 text representation instead of
   falling into String.fromJS. This matches both postgres.js
   (types.date.serialize hardcodes .toISOString()) and pg
   (builds an ISO 8601 string).

2. Fix a pre-existing bug in JSC__JSValue__toISOString in
   bindings.cpp: it was writing to a local char buffer[64] and
   discarding the caller's output buffer entirely. Now writes into
   buf, which makes the Zig JSValue.toISOString helper actually
   return a populated slice. Widened the Zig-side buffer from
   *[28]u8 to *[64]u8 to match Bun::toISOString's contract.

Verification

test/regression/issue/29010.test.ts covers:

• Date in a SELECT parameter cast to ::timestamptz
• Date in an INSERT via sql([{ ... }]) row helper
• Date in an INSERT as a plain parameter
• Date constructed from a non-UTC ISO string (round-trips as UTC)
• Sanity: default (prepare: true) binary path still works

Without the fix, the four prepare: false tests fail with
ERR_POSTGRES_SERVER_ERROR 22007 invalid input syntax for type timestamp with time zone. With the fix, all five pass.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Standardizes ISO string buffering and changes Date serialization to emit ISO 8601 for PostgreSQL binds; updates native bindings to accept caller-provided buffers and adds a regression test for Date parameter round-trips.

Changes

Cohort / File(s)	Summary
JSValue bindings src/bun.js/bindings/JSValue.zig	Adds JSValue.ISOStringBuffer = [64]u8; changes toISOString and getDateNowISOString signatures to accept *ISOStringBuffer; documents failure behavior (negative count → empty slice).
C++ bindings src/bun.js/bindings/bindings.cpp	Forwards caller-provided ISO buffer into Bun::toISOString (removes local char[64]); replaces inline Date formatting in DateNowISOString with a call into Bun::toISOString after computing/validating now.
PostgreSQL bind serialization src/sql/postgres/PostgresRequest.zig	In writeBind non-binary path, special-cases JS Date: calls value.toISOString(...) and writes resulting bytes; failing when ISO serialization yields empty bytes (error.InvalidQueryBinding); removes fallback to text conversion for Date.
Regression test test/regression/issue/29010.test.ts	Adds tests exercising Bun.SQL Date parameter handling (prepare: false and true), including INSERT and SELECT round-trips and timezone-offset date case; resolves or skips against available Postgres instance.
Comment update src/s3/credentials.zig	Updates a commented example variable type to reference jsc.JSValue.ISOStringBuffer (comment-only change).

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: fixing Date parameter serialization to use ISO 8601 format in Postgres text format queries.
Description check	✅ Passed	The PR description comprehensively covers the issue, root cause, fix strategy, and verification approach with concrete examples and test coverage.
Linked Issues check	✅ Passed	The PR fully addresses issue #29010 by detecting JS Date values in text-format parameters, emitting ISO 8601 via toISOString instead of toString, fixing the bindings.cpp bug, and adding comprehensive regression tests.
Out of Scope Changes check	✅ Passed	All changes are scoped to the issue: JSValue.zig buffer type and function signatures, bindings.cpp C ABI and implementation, PostgresRequest.zig text-path serialization, and a focused regression test file.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[robobun]

^{Updated 9:37 AM PT - May 1st, 2026}

❌ @robobun, your commit 389dd7c has 2 failures in Build #49712 (All Failures):

• test/cli/run/no-orphans.test.ts - timeout on 🍎 14 x64
Details
test/cli/run/no-orphans.test.ts - timeout on 🍎 26 aarch64
Details
test/cli/run/no-orphans.test.ts - timeout on 🍎 14 aarch64
• test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts - core dumped on 🐧 3.23 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 29013

That installs a local version of the PR into your bun-29013 executable, so you can run:

bun-29013 --bun

[claude]

This PR correctly fixes Date serialization in text-format Bind messages, but the inline comments highlight a pre-existing binary-format/text-data mismatch for .time, .float4, .float4_array, and .numeric types in the same else branch — worth a human look before merging.

Extended reasoning...

Overview\nModifies 4 files: fixes a pre-existing bug in bindings.cpp (ISO string was written to a local buffer instead of the caller's), widens the Zig buffer declaration to match, adds a isDate() guard in writeBind's else branch to emit ISO 8601 instead of the locale-dependent toString() output, and adds a regression test.\n\n### Security risks\nNone — this is a serialization bug fix with no security implications.\n\n### Level of scrutiny\nThe fix itself is straightforward and correct. However, the flagged pre-existing issue (binary format code 1 declared in the first loop for .time/.float4/.float4_array/.numeric, but text bytes sent in the else branch) is a real protocol mismatch in the same code path being modified. The new comment "This path runs in text format" is misleading for those types. This warrants human review to decide whether to fix or defer the pre-existing mismatch.\n\n### Other factors\nThe test's afterAll unconditionally calling dockerCompose.down() when the local-postgres fallback was used is a minor nit. The build is in progress (#44483). No prior human reviewer comments to address.

[claude]

LGTM — the buffer fix and the guard in are both correct and well-targeted.

Extended reasoning...

Overview

Fixes a regression where JS Date parameters sent via the text-format path (prepare: false, or first execution of a fresh prepared statement) were serialized using Date.prototype.toString() instead of Date.prototype.toISOString(). The fix has three parts: (1) remove the local char buffer[64] in JSC__JSValue__toISOString that shadowed the caller's output buffer (plain C bug), (2) widen the Zig-side buffer from [28]u8 to [64]u8 to match the C contract, and (3) add an isDate() check in the else branch of writeBind that emits ISO 8601 instead of falling through to String.fromJS.

Security risks

None. The change only affects serialization of Date values in PostgreSQL protocol Bind messages. No auth, crypto, or permission paths are touched.

Level of scrutiny

Low-to-medium. The C++ and Zig changes are each 1-3 lines and address clear bugs. The Zig request change is additive (a Date-specific fast path before the existing text fallback). The test file properly tracks whether Docker was used and only tears down what it started.

Other factors

All bugs flagged by the hunt system are pre-existing and not introduced by this PR. The inline comment about unconditional dockerCompose.down() was already addressed in the committed test code (target.usedDocker flag). CI failures are all in unrelated valkey/azure-service-bus tests.

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/bun.js/bindings/JSValue.zig`:
- Around line 408-423: Extract the 64-byte ISO buffer into a single exported
type/constant on JSValue (e.g., pub const ISOStringBuffer = [64]u8) and replace
the literal [64]u8 occurrences in the extern declarations and function
signatures (JSC__JSValue__toISOString, toISOString,
JSC__JSValue__DateNowISOString, getDateNowISOString) with that named type (use
pointer form where needed, e.g., *jsc.JSValue.ISOStringBuffer). Update call
sites to declare buffers as var iso_buf: jsc.JSValue.ISOStringBuffer = undefined
and keep existing slicing/return logic unchanged but operating on the typed
buffer. Ensure the extern prototypes, function params, and callers all reference
the new ISOStringBuffer symbol to avoid future drift.

In `@src/sql/postgres/PostgresRequest.zig`:
- Around line 162-181: When value.isDate() is true, do not fall back to the old
String.fromJS path on ISO serialization/write failure; instead treat any ISO
serialization or write failure as a bind error and return it immediately.
Specifically, in the PostgresRequest.zig block that calls value.toISOString(...)
and then writer.write(iso) (and the subsequent l.writeExcludingSelf()), if
iso.len == 0 or any of those operations would fail, propagate/return a
bind-specific error (e.g., error.BindError) rather than clearing handled and
continuing to the String.fromJS branch; this prevents re-entering the old text
path for Date values and ensures failures for Date binding are reported as bind
errors.

In `@test/regression/issue/29010.test.ts`:
- Around line 69-74: The test teardown calls dockerCompose.down() in afterAll
which brings down the whole shared bun-test-services project; instead, change
the cleanup to only release the Postgres service acquired by resolvePostgres()
(postgres_plain) or use a refcounted release helper so we don't stop unrelated
services: replace the dockerCompose.down() call with a service-scoped operation
(e.g. stop/remove or release only 'postgres_plain') or wire in a refcounted
release function that decrements and only tears the project down when no suites
remain, ensuring afterAll only targets postgres_plain rather than the entire
project.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: aee2e565-66f3-489d-9739-622574e760b2

📥 Commits

Reviewing files that changed from the base of the PR and between 1afabdd and 0ffeaa6.

📒 Files selected for processing (4)

• src/bun.js/bindings/JSValue.zig
• src/bun.js/bindings/bindings.cpp
• src/sql/postgres/PostgresRequest.zig
• test/regression/issue/29010.test.ts

[claude]

LGTM — all raised issues addressed (afterAll guard added, toISOString buffer fixed, DateNowISOString ABI fixed in 0ffeaa6); pre-existing binary-format mismatches acknowledged as out-of-scope.

Extended reasoning...

Overview

Fixes Date parameter serialization in PostgreSQL text-format path (prepare:false and first execution of prepared statements). Four files: bindings.cpp (one-line toISOString buffer fix), JSValue.zig (extern sig + buffer size corrections), PostgresRequest.zig (Date detection in else branch), and a new regression test.

Security risks

None — this is pure data serialization logic with no auth, crypto, or permission changes.

Level of scrutiny

Low. The fix is targeted and mechanical: detect isDate(), call toISOString(), fall back to the existing String.fromJS path otherwise. The C++ fix (remove shadowing local buffer) is a single-line change with obvious correctness. Both have corresponding test coverage.

Other factors

All three issues flagged in my prior review run have been addressed: the afterAll/dockerCompose guard is now conditional on usedDocker, the toISOString local-buffer bug is fixed, and the DateNowISOString ABI mismatch was fixed in the follow-up commit already merged to main. The remaining pre-existing binary-format mismatches (time/float4/int4_array) are correctly deferred.

[claude]

LGTM — all prior review concerns addressed in the follow-up commits.

Extended reasoning...

Overview

The PR fixes a real user-visible bug: Date objects passed as SQL parameters under prepare: false were serialized via Date.prototype.toString() (locale-dependent) rather than Date.prototype.toISOString() (RFC 3339), causing PostgreSQL to reject them with 22007 invalid input syntax. The fix is in the else branch of writeBind in PostgresRequest.zig, with supporting fixes in C++/Zig bindings and a regression test suite.

Security Risks

None. This is a serialization correctness fix for a database client library. No auth, crypto, or permissions code is touched.

Level of Scrutiny

Medium. The fix is targeted and the logic is straightforward (detect isDate(), call toISOString(), propagate error on failure). The C++ fix (using caller-provided buf instead of a local stack buffer) is a clear one-liner correction. The Zig ABI fix corrects a broken extern declaration that would have crashed on any call. The codegen tombstone change is mechanical.

Addressed Feedback

All three issues raised in my prior review were addressed: the getDateNowISOString ABI mismatch (fixed in 0ffeaa6), the unconditional dockerCompose.down() in afterAll (fixed in 6dfd18c), and the String.fromJS fallback after Date detection (fixed in 6014efd). The remaining pre-existing issues (.time/.float4/etc. binary-text mismatch) are pre-existing, unrelated to this PR, and correctly deferred.

[claude]

LGTM — previous concerns addressed.

Extended reasoning...

Overview

This PR fixes a real user-impacting bug: JavaScript Date parameters passed to PostgreSQL were serialized via toString() (locale-dependent) instead of toISOString() (ISO 8601), causing parse failures with SQLSTATE 22007. The change is targeted and correct: detect isDate() in the writeBind else-branch and emit ISO 8601 bytes, returning error.InvalidQueryBinding on failure rather than silently falling back.

Security risks

None. The change is in local serialization logic with no user-controlled code paths beyond what was already reachable.

Level of scrutiny

Moderate. This touches production SQL parameter serialization (PostgresRequest.zig) and a C++ binding (bindings.cpp). The core logic change is small and correct. The C++ fix (passing caller's buf to Bun::toISOString instead of a discarded local buffer) is a straightforward one-line correction.

Prior review feedback

All substantive issues from my earlier reviews have been addressed: the ABI mismatch in getDateNowISOString was fixed (0ffeaa6), the afterAll teardown no longer calls dockerCompose.down() (598822b), and the String.fromJS fallback was removed in favor of a hard bind error (6014efd). Pre-existing binary format mismatches for .float4/.time/.numeric/.int4_array are acknowledged out of scope. The only remaining nit (the prepare:true test exercises the text path, not binary) is a test coverage quality issue noted in an inline comment.

[alii]

@robobun rebase main and fix whatever went wrong with the cron stuff. this pr doesn't need to touch cron

[robobun]

✅ Pushed 389dd7cdcf. All review threads resolved. Ready for human review.

CI #49712: 70 jobs passed. Remaining failures are known flakes currently hitting multiple unrelated open PRs (#30041, #28645, #29897, #28862):

• test/cli/run/no-orphans.test.ts — timeout on all three macOS lanes. Process-orphan cleanup from run: rename dieWithParent → noOrphans, add --no-orphans, cover bun run on macOS #29930; does not touch SQL/Date/bindings.
• test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts — core dump on 🐧 3.23 aarch64. BroadcastChannel GC from Fix BroadcastChannel channelToContextIdentifier locking and dispatchMessage lifetime #29441.

test/regression/issue/29010.test.ts passed on every platform including debian-13-x64-asan.

Commits:

• 389dd7cd — cover the three remaining write_buffer rollback sites (both protocol.Sync catch blocks, doRun simple executeQuery). OOM-only; consistency.
• f68c8e6e — WriteBufferSnapshot: snapshot write_buffer.byte_list.len before each PostgresRequest.* call in advance()/doRun and truncate back in the catch, so a mid-writeBind error.InvalidQueryBinding doesn't leave a partial Bind frame that poisons the next query. Two connection-survival tests.
• d29f00cb — std.math.isFinite guard in types.date.fromJS before @intFromFloat → ERR_POSTGRES_INVALID_QUERY_BINDING instead of panic/UB on new Date(NaN) via the binary path.
• e43439ae — JSValue.ISOStringBuffer type; DateNowISOString collapsed to Bun::toISOString.
• aa933ad1 — core fix: writeBind text path serializes Date via toISOString() instead of locale toString().

Gate (local Postgres): USE_SYSTEM_BUN=1 → 8 fail / 1 pass; bun bd → 9 pass / 0 fail.

MySQL DateTime.fromJS/Time.fromJS have the same unguarded @intFromFloat(NaN) — out of scope, noted for follow-up.

[coderabbitai]

♻️ Duplicate comments (1)

src/bun.js/bindings/JSValue.zig (1)

408-423: 🧹 Nitpick | 🔵 Trivial

Deduplicate the 64-byte ISO buffer contract.

*[64]u8 is still repeated across both externs and wrappers here. Please lift it into a single exported alias/constant on JSValue and reuse it so the Zig↔C++ buffer contract cannot drift again.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/bindings/JSValue.zig` around lines 408 - 423, The 64-byte buffer
type `*[64]u8` is duplicated across the externs and wrappers
(JSC__JSValue__toISOString, JSC__JSValue__DateNowISOString and the Zig functions
toISOString and getDateNowISOString); create a single exported alias or constant
on the JSValue type (e.g., ISO_BUFFER or IsoBufType) that defines the `[64]u8`
shape and replace all occurrences in the extern declarations and the two public
functions toISOString and getDateNowISOString to use that alias so the Zig↔C++
buffer contract is centralized and cannot drift.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/bun.js/bindings/JSValue.zig`:
- Around line 408-423: The 64-byte buffer type `*[64]u8` is duplicated across
the externs and wrappers (JSC__JSValue__toISOString,
JSC__JSValue__DateNowISOString and the Zig functions toISOString and
getDateNowISOString); create a single exported alias or constant on the JSValue
type (e.g., ISO_BUFFER or IsoBufType) that defines the `[64]u8` shape and
replace all occurrences in the extern declarations and the two public functions
toISOString and getDateNowISOString to use that alias so the Zig↔C++ buffer
contract is centralized and cannot drift.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d7acc210-1f34-4b60-a94d-450c033f173e

📥 Commits

Reviewing files that changed from the base of the PR and between 598822ba41b8af183c9919bc0ddca51c14e1f578 and 1a22fdd.

📒 Files selected for processing (2)

• src/bun.js/bindings/JSValue.zig
• src/bun.js/bindings/bindings.cpp

[alii]

@robobun adopt

[coderabbitai]

✅ No merge conflicts detected when merging into main.

Your branch is good to go!

[coderabbitai]

✅ No merge conflicts detected when merging into main.

Your branch is good to go!

[claude]

LGTM — all prior review feedback addressed. The NaN issue in date.zig is pre-existing and out of scope for this change.

Extended reasoning...

Overview

This PR fixes Date serialization in the PostgreSQL text-format bind path by calling toISOString() instead of String.fromJS() (which produced locale-dependent toString() output). It also fixes a real bug in the C++ bindings where JSC__JSValue__toISOString was writing to a local stack buffer instead of the caller-provided buffer, and adds JSValue.ISOStringBuffer as a shared named type to prevent future ABI drift.

Security risks

None. This is a data-serialization correctness fix confined to the PostgreSQL bind message path. No auth, crypto, or permissions code is touched.

Level of scrutiny

Moderate. The changes touch C++/Zig cross-language bindings (real ABI risk) and the PostgreSQL wire protocol serialization path (real correctness risk). However, the changes are focused and the PR went through multiple rounds of review where each issue was systematically addressed.

Other factors

All substantive issues I raised in prior rounds have been resolved: the ABI mismatch in getDateNowISOString was fixed, ISOStringBuffer was extracted as a named type, the afterAll Docker teardown was scoped correctly, and the prepare: true test now exercises both the text-format first-execution path and the binary types.date.fromJS second-execution path. The newly-reported NaN panic in date.zig:25 is pre-existing (that file is not part of this PR) and should be addressed in a follow-up.

[Jarred-Sumner]

@robobun resume. Fix the comments. Rebase

[claude]

All prior review threads are resolved and I didn't find anything new, but the scope has grown well beyond the original serialization fix — it now adds a WriteBufferSnapshot rollback mechanism across ~10 call sites in the Postgres connection state machine and reworks the toISOString/DateNowISOString Zig↔C++ ABI, so it's worth a human pass before merging.

Extended reasoning...

Overview

This PR started as a targeted fix for #29010 (serialize JS Date as ISO 8601 in the Postgres text-format bind path) but accreted several related fixes through review:

• PostgresRequest.zig: writeBind else branch now detects value.isDate() and emits toISOString() instead of falling through to String.fromJS → locale toString(). Returns error.InvalidQueryBinding for non-finite dates.
• bindings.cpp / JSValue.zig: Fixed a pre-existing bug where JSC__JSValue__toISOString wrote into a local char buffer[64] and discarded the caller's buffer; fixed the JSC__JSValue__DateNowISOString Zig extern ABI mismatch (f64→pointer, JSValue→c_int); collapsed its body to delegate to Bun::toISOString; introduced JSValue.ISOStringBuffer = [64]u8.
• types/date.zig: Added std.math.isFinite guard before @intFromFloat to avoid Zig Illegal Behavior on new Date(NaN) via the binary path; widened return type to AnyPostgresError!i64.
• PostgresSQLConnection.zig / PostgresSQLQuery.zig: New WriteBufferSnapshot struct that captures write_buffer.byte_list.len before each PostgresRequest.* call and truncates back in the catch block, so a mid-write bind error doesn't leave a partial frame that poisons the connection. Applied at ~10 call sites across advance() and doRun.
• s3/credentials.zig: Comment-only update to reference the new buffer type.
• test/regression/issue/29010.test.ts: 9 tests covering text/binary paths, invalid dates, and connection survival after a caught bind error.

Security risks

None identified. Parameters in the Postgres Bind message are length-prefixed, so the serialization change carries no injection risk. WriteBufferSnapshot.restore only truncates an in-memory buffer length (with debugAssert invariants). The FFI signature changes were verified against the C++ side.

Level of scrutiny

This warrants a human review. While each piece is well-reasoned and every prior review thread is resolved, the PR now touches:

• Zig↔C++ FFI signatures (ABI-sensitive),
• the Postgres wire-protocol writer's error-recovery semantics (a new rollback abstraction threaded through the connection state machine and query enqueue path),
• an error-set widening (bun.JSError!i64 → AnyPostgresError!i64) on a shared helper.

The WriteBufferSnapshot addition in particular is a design-level change to connection error recovery that a maintainer should sign off on — it's correct as far as I can tell, and the connection-survival tests gate it, but it's not the kind of mechanical change I should approve unilaterally.

Other factors

• All 11 prior inline review threads (mine and CodeRabbit's) are marked resolved, with the most recent commit (389dd7c) addressing the last consistency nit.
• CI shows 1 failure in test/bake/dev-and-prod.test.ts on Windows, which is unrelated to this change.
• Two maintainers (alii, Jarred-Sumner) have engaged on the thread but only with directives to the bot; neither has reviewed the final state.