sql(postgres): pin DateStyle=ISO in StartupMessage so server defaults cannot corrupt dates - #35112
Conversation
Bun decoded text-format date/timestamp values via JS Date.parse, which assumes ISO input. Postgres emits date text in whatever DateStyle the session has, and a database or role can default to a non-ISO style such as 'SQL, DMY'. With that default, '2026-04-03'::date came over the wire as '03/04/2026', Date.parse read it as MDY, and Bun silently returned 4 March instead of 3 April (or null for any day > 12). Send DateStyle=ISO, MDY in the startup packet alongside client_encoding. A startup-packet parameter has GUC source PGC_S_CLIENT, which outranks postgresql.conf / ALTER DATABASE / ALTER ROLE defaults, so the server always emits the ISO form the decoder expects. node-postgres and postgres.js pin DateStyle the same way.
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — thanks for moving pgParameterStatus into wire-frames.ts.
What was reviewed:
StartupMessage.rs: thez_field_countaddition and the twowriter.string()calls mirror theclient_encodingpair one line above, so the length prefix stays correct.- Confirmed
ISO, MDYmatches what node-postgres/postgres.js send; a later userSET datestylestill overrides it, so no user-visible regression. - Mock-server test parses the startup packet and asserts the param; container test forces
SQL, DMYat the database level and round-trips both an ambiguous (04-03) and a day>12 (07-22) date on simple + extended paths, with cleanup infinally.
Extended reasoning...
Overview
The runtime change is two lines in src/sql/postgres/protocol/StartupMessage.rs: add z_field_count(b"DateStyle", b"ISO, MDY") to the length computation and emit the corresponding writer.string() pair. This is byte-for-byte the same pattern as the existing client_encoding = UTF8 entry immediately above it, so the message length stays consistent with what is written. src/sql_jsc/postgres/DataCell.rs is a comment-only change documenting the invariant the startup packet now establishes. test/js/sql/wire-frames.ts gains a pgParameterStatus frame builder (my earlier nit, now addressed), and test/js/sql/postgres-datestyle.test.ts is a new test file with a mock-server protocol assertion plus a describeWithContainer end-to-end test.
Security risks
None. The new bytes are a compile-time constant in the outbound startup packet; no user input flows into it and no parsing changes.
Level of scrutiny
Low-to-medium. The fix is mechanical (mirrors an adjacent, already-proven line) and matches the behavior of node-postgres and postgres.js, so ecosystem compatibility is well-established. It corrects silent date corruption without changing what users observe (they receive JS Date objects, not the wire text). A user who explicitly runs SET datestyle in-session still wins because SET has higher GUC precedence than PGC_S_CLIENT, so this cannot break intentional overrides. I checked z_field_count in zHelpers.rs — it returns z_count(prefix) + z_count(slice) for a non-empty value, which matches the two writer.string() calls emitted, so the Int32 length prefix is correct.
Other factors
The tests are solid: the mock-server test asserts the actual bytes in the StartupMessage (fails on main because the parameter is absent), and the container test uses the established describeWithContainer("postgres", { image: "postgres_plain" }) harness pattern seen in sibling files, forces a hostile SQL, DMY database default, and asserts exact ISO strings on both simple and extended query paths — including a day-22 date that would decode to null on main. Cleanup runs in finally. My previous nit about pgParameterStatus placement was resolved in 48a5621. No outstanding reviewer comments remain.
There was a problem hiding this comment.
LGTM — the prior nit (moving pgParameterStatus into wire-frames.ts) is addressed.
What was reviewed:
StartupMessage.rs: theDateStyle/ISO, MDYpair mirrors the existingclient_encoding/UTF8pattern exactly, andz_field_countcounts both key and value so the length prefix stays correct.DataCell.rschange is comment-only.- Container test:
ALTER DATABASE ... SET datestyleis reset infinally, and with the fix in place the startup-packet override means concurrent Bun connections against the sharedpostgres_plaincontainer aren't affected by the temporary DB default.
Extended reasoning...
Overview
Two lines added to the StartupMessage length calculation and two writer.string() calls appending DateStyle = ISO, MDY to the Postgres startup packet, immediately after and identically shaped to the existing client_encoding = UTF8 pair. The DataCell.rs change is a comment rewrite documenting the invariant now established. wire-frames.ts gains a pgParameterStatus helper (my prior nit, now addressed), and a new test file covers both a protocol-level assertion on the StartupMessage bytes and an end-to-end container test that forces a non-ISO database default and verifies dates round-trip on both simple and extended query paths.
Security risks
None. This adds a fixed literal key/value pair to an outbound protocol message; no user input flows into it. The length arithmetic uses the same z_field_count helper as the neighboring client_encoding line, so the declared packet length stays consistent with what's written.
Level of scrutiny
Low-to-moderate. The runtime change is mechanical — copy the line above, change two string literals — and the approach (pin DateStyle in the startup packet, GUC source PGC_S_CLIENT) is exactly what node-postgres and postgres.js do. I confirmed z_field_count(b"DateStyle", b"ISO, MDY") returns z_count(key) + z_count(value) for a non-empty value, matching the two writer.string() calls. No CODEOWNERS cover src/sql/.
Other factors
Test coverage is solid: the mock-server test parses the actual StartupMessage bytes and asserts params.DateStyle matches /^ISO\b/ (fails on main where the key is absent), and the container test exercises both an ambiguous date (2026-04-03, swaps under MDY heuristics) and a day-> 12 date (2026-07-22, becomes null) on both .simple() and extended paths. The ALTER DATABASE mutation is reset in finally, and since the fix makes every Bun connection override the DB default anyway, it can't leak into concurrent tests sharing the container. The describeWithContainer("postgres", { image: "postgres_plain" }) + await container.ready shape matches sibling tests in test/js/sql/.
|
CI is green on everything this diff touches. The remaining red on build 77752 is unrelated to this change:
Ready for review. |
…ity (#35121) ## Problem Postgres represents unbounded dates and timestamps as the special values `'infinity'` / `'-infinity'`. On every decode path Bun returned them as `Invalid Date` (a `Date` whose `getTime()` is `NaN`): ```js await sql`select 'infinity'::date as a, '-infinity'::date as b` // [{ a: Invalid Date, b: Invalid Date }] sign lost, indistinguishable from a parse failure ``` That is data loss: `+infinity`, `-infinity`, and a genuinely unparseable value all collapse to the same `NaN`, so user code cannot tell whether a row means "no upper bound" or "no lower bound". ## Cause Three independent paths, all ending at the same NaN: - **scalar text** (`date` always, `timestamp`/`timestamptz` on `.simple()`): routed through JS `Date.parse('infinity')` → `NaN`. - **scalar binary** (`timestamp`/`timestamptz` on the extended protocol): Postgres sends `PG_INT64_MAX` / `PG_INT64_MIN` (`DT_NOEND` / `DT_NOBEGIN`). `from_binary` does `i64::MAX as f64 / 1000 + 946684800000` ≈ 9.224e15 ms, past JS Date's ±8.64e15 range, and `DateInstance::create`'s `timeClip` maps it to `NaN`. - **array text** (`{infinity,-infinity}::timestamp[]`): the parser already produced `SQLDataCell::date(f64::INFINITY)`, but `SQLClient.cpp` hands that to `DateInstance::create`, whose `timeClip` maps every non-finite input to `NaN`. So the existing code that tried to preserve ±Infinity never took effect. ## Fix Return the JS Number `±Infinity` for these values, matching node-postgres (`pg-types` via `postgres-date`, for oids 1082/1114/1184). `postgres.js` does not special-case this and returns `Invalid Date`, but that loses information; node-postgres' behaviour is the one that round-trips. - `types/date.rs` `from_binary`: recognise `i64::MAX` / `i64::MIN` and return `±f64::INFINITY`. - `types/date.rs` `parse_infinity`: shared helper for the `infinity` / `-infinity` text spellings. - `types/date.rs` `from_js`: map `±Infinity` back to `i64::MAX` / `i64::MIN` so the encode/decode pair is symmetric and the `(ms - epoch) * 1000` arithmetic never overflows on the value the decoder now produces. - `DataCell.rs`: check `parse_infinity` before `Date.parse` on the scalar text path and the unquoted-`date[]` array path (the `timestamp[]`/`timestamptz[]` array path already produced `date(±INFINITY)`). - `SQLClient.cpp` `toJS`: when the Date cell carries ±Infinity, return `jsDoubleNumber(±Infinity)` instead of a `timeClip`'d `DateInstance`. Finite dates are unchanged and remain `Date` instances. MySQL shares the `toJS` path but never produces ±Infinity there (zero DATETIME is `NaN`, which `std::isinf` is false for). This is a behaviour change (the result type for these two values changes from `Date` to `number`), but the previous value was unusable: `.toISOString()` throws on it, `.getTime()` is `NaN`, and the sign is gone. ## Verification `test/js/sql/postgres-infinity-date.test.ts` drives a scripted v3 backend and covers every path: - scalar text: `date` / `timestamp` / `timestamptz` - scalar binary: `timestamp` / `timestamptz` (`PG_INT64_MAX` / `PG_INT64_MIN` on the wire) - array text: `date[]` / `timestamp[]` / `timestamptz[]` - bind: `±Infinity` bound to a `timestamp` / `timestamptz` parameter writes `PG_INT64_MAX` / `PG_INT64_MIN` in the `Bind` body Each case asserts `±Infinity` for the special values and a `Date` instance for an adjacent finite value. All ten fail on main; all pass here. The three `sql.test.ts` array tests and `test/regression/issue/21311.test.ts` that pinned the old `Invalid Date` behaviour are updated. `sql-postgres-datetime-roundtrip.test.ts`, `postgres-datarow-overrun.test.ts`, `postgres-binary-float-nan-box.test.ts`, and `wire-frames.test.ts` are unchanged and pass. Follows on from the note at the bottom of #35112. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
@robobun fix conflicts |
|
Merged main; conflict was with #35121 in |
There was a problem hiding this comment.
LGTM — the earlier nit (moving pgParameterStatus into wire-frames.ts) is addressed and the merge with #35121 kept parse_infinity intact.
What was reviewed:
StartupMessage.rs: length count and write are symmetric —z_field_count(b"DateStyle", b"ISO, MDY")matches the twowriter.string()calls, same pattern asclient_encoding.DataCell.rs: comment-only after the merge;parse_infinityguard from #35121 is still present before the ISO-decode path.- Container test's
ALTER DATABASE ... SET datestyleis reset infinally, and even if it leaked the startup-packet pin now overrides it for every other Bun.SQL connection, so no cross-test contamination.
Extended reasoning...
Overview
Three-line production change in src/sql/postgres/protocol/StartupMessage.rs adding DateStyle = ISO, MDY to the Postgres startup packet (one z_field_count term in the length calculation plus two writer.string() calls), mirroring the existing client_encoding = UTF8 pinning immediately above it. src/sql_jsc/postgres/DataCell.rs is a comment-only change documenting the invariant the startup packet now establishes; after the merge with #35121 the parse_infinity check is preserved and only the comment block differs. test/js/sql/wire-frames.ts gains a pgParameterStatus builder next to its siblings (my prior nit, now addressed). test/js/sql/postgres-datestyle.test.ts is new and covers both a protocol-level mock (asserts the parameter is present in the StartupMessage bytes) and an end-to-end container test (ALTER DATABASE ... SET datestyle = 'SQL, DMY', reconnect, verify dates round-trip on both simple and extended paths).
Security risks
None. The change adds a fixed literal parameter to an outbound handshake packet; no user input flows into it, no parsing of untrusted data changes, and the length prefix is computed with the same z_field_count helper used for the neighbouring client_encoding field so there is no hand-rolled arithmetic to get wrong.
Level of scrutiny
Low-to-moderate. The production surface is tiny, follows the exact pattern already in the file, and matches what node-postgres and postgres.js do (cited in the PR and comment). The behaviour is a strict correctness improvement — a startup-packet GUC (PGC_S_CLIENT) outranks server/database/role defaults but is still overridable by an explicit in-session SET, so users who deliberately change DateStyle mid-session are unaffected. I checked z_field_count in zHelpers.rs to confirm the length term matches what writer.string() emits (key + NUL + value + NUL), so the declared packet length stays correct.
Other factors
The evidence block shows the new test fails on main (parameter absent, dates corrupted) and passes with the fix on both debug-ASAN and release. CI on the touched test file is green across all lanes; remaining red is unrelated pre-existing flakes. The container test resets datestyle in finally, and even a transient leak cannot affect other Bun.SQL tests because this very change makes the startup packet override any database-level default. The merge conflict with #35121 was in the same comment block only and was resolved by keeping both the parse_infinity guard and the updated comment — verified in the preloaded DataCell.rs. No outstanding reviewer comments.
2 participants
Problem
Bun.SQLdecodes text-formatdate/timestamp/timestamptzvalues via JSDate.parse, which only handles ISO input unambiguously. Postgres emits those values in whateverDateStylethe session has, and a server, database, or role can default to a non-ISO style such as'SQL, DMY'(a common EU configuration). Bun never setDateStyleon connect, so the server-side default leaked through and dates were silently corrupted:The extended protocol on the same connection is equally affected because
dateuses text format there too. This is silent data corruption: no error is raised, the values are just wrong.Fix
Send
DateStyle=ISO, MDYin theStartupMessagealongside the existingclient_encoding=UTF8. A startup-packet parameter has GUC sourcePGC_S_CLIENT, which outrankspostgresql.conf,ALTER DATABASE, andALTER ROLEdefaults, so the server always emits the ISO form the decoder expects regardless of how the cluster is configured. This matches whatnode-postgresandpostgres.jsdo.The text-decode comment in
DataCell.rsis updated to document the invariant the startup packet now establishes.Verification
test/js/sql/postgres-datestyle.test.tsStartupMessagecarriesDateStyle=ISO(fails on main: the parameter is absent).ALTER DATABASE bun_sql_test SET datestyle = 'SQL, DMY', reconnect, read'2026-04-03'::dateand'2026-07-22'::dateon both the simple and extended paths. On main the session reportsSQL, DMYand the dates come back wrong/null; with the fix the session reportsISO, MDYand both dates round-trip correctly.test/js/sql/sql-postgres-datetime-roundtrip.test.ts,wire-frames.test.ts,postgres-*protocol tests all pass.Not in this PR
'infinity'::date/'-infinity'::datestill decode toInvalid Dateon both the text and binary paths (JSC'sDateInstanceconstructor appliestimeClip, which maps non-finite values to NaN). That is a separate design question (node-postgres returns the NumberInfinity, not aDate) and is unchanged here.[review] gate passed · iteration 3 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 3
evidence per changed file