Conversation
|
Updated 9:35 AM PT - Apr 12th, 2026
❌ @robobun, your commit a570c95 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 29216That installs a local version of the PR into your bun-29216 --bun |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughParses MySQL text-protocol date/time strings, enforces component validation, interprets components as UTC when converting to JS timestamps, updates result-set text-path to use the new parser, maps binary zero-date sentinels to NaN, and adds a regression test covering DATETIME/TIMESTAMP round-trips under a non-UTC process timezone. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the one-line binary-protocol swap and the new text-protocol parser both correctly interpret MySQL's naive datetime components as UTC.
Extended reasoning...
Overview
This PR fixes a timezone bug in MySQL DATETIME/TIMESTAMP decoding across two protocol paths in and , plus adds a regression test in .
Security risks
None. This is a pure data-decoding correctness fix with no authentication, permission, or injection surface changes.
Level of scrutiny
The changes are mechanical and focused: a one-function-name swap ( → ) for the binary protocol path, and a new ~50-line parser for the text protocol path. Both target the same clear root cause (naive datetime components being interpreted as local time instead of UTC). The binding was already present in JSGlobalObject.zig and only differs by passing . The parser is straightforward positional parsing with correct fractional-second right-padding logic. The regression test explicitly flips and round-trips through all four protocol×column-type combinations.
Other factors
The two bugs flagged as inline comments: (1) the pre-existing dead-code compile error predates this PR and has no callers, so it has zero runtime impact; (2) the unused import is a trivial nit. Neither warrants blocking the fix. Test coverage for the new behavior is solid.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29208.test.ts`:
- Around line 66-74: Test toggles process.env.TZ and is currently marked
concurrent: true which can cause race conditions; remove the concurrent: true
flag from the describeWithContainer invocation (the block using
describeWithContainer("issue `#29208` (containerized MySQL)", { image:
"mysql_plain", concurrent: true }, ...)) or replace it with concurrent: false to
ensure TZ isolation, or alternatively wrap runRoundTrip to acquire a global TZ
mutex before changing process.env.TZ; update the describeWithContainer options
or add a TZ-lock around runRoundTrip to prevent concurrent execution.
🪄 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: 38916f63-753a-4c13-9bd7-7aa2a45c2f94
📒 Files selected for processing (2)
src/sql/mysql/protocol/ResultSet.zigtest/regression/issue/29208.test.ts
MySQL's DATETIME and TIMESTAMP wire format carries no timezone — the server returns naive year/month/day/hour/minute/second components. Bun fed those components through JSC's local-time constructor, so on any machine whose process TZ was not UTC the returned JS Date was off by the client's UTC offset. - Binary protocol: swap gregorianDateTimeToMS (local) for the UTC variant in DateTime.toJSTimestamp. - Text protocol: parse 'YYYY-MM-DD[ HH:MM:SS[.ffffff]]' directly into a DateTime and route through the same UTC path, replacing the JS Date parser which also treats TZ-less strings as local time. The send path (DateTime.fromUnixTimestamp) was already UTC-correct, so writes stored the right value and only reads were wrong, matching the report. Fixes #29208
robobun
force-pushed
the
farm/1208a173/mysql-datetime-utc
branch
from
c624d3a to
094d9c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/sql/mysql/MySQLTypes.zig`:
- Around line 529-568: The fromText() parser currently only enforces month ∈
1..12 and day ∈ 1..31 so malformed dates like 2024-02-31 or times like 24:00:00
slip through and are later normalized by gregorianDateTimeToMSUTC(); after
parsing year/month/day/hour/minute/second (use the local variables year, month,
day, result.hour, result.minute, result.second) add strict validation: reject if
hour >= 24 or minute >= 60 or second >= 60, and validate day against the
month/Year (Apr/Jun/Sep/Nov max 30, Feb max 28 or 29 when leap year: leap =
(year % 4 == 0) and (year % 100 != 0 or year % 400 == 0)); return
error.InvalidDateTimeText on invalid values so invalid-but-storable MySQL rows
produce Invalid Date instead of being normalized.
🪄 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: 5ca67cef-c2dd-4234-b110-ffee671cea09
📒 Files selected for processing (3)
src/sql/mysql/MySQLTypes.zigsrc/sql/mysql/protocol/ResultSet.zigtest/regression/issue/29208.test.ts
There was a problem hiding this comment.
LGTM — all previously flagged issues resolved in 094d9c0.
Extended reasoning...
Overview
This PR fixes MySQL DATETIME/TIMESTAMP deserialization to use UTC instead of local time on both the binary protocol path (swap gregorianDateTimeToMS → gregorianDateTimeToMSUTC) and the text protocol path (new DateTime.fromText parser replacing the generic JS date parser). Three files changed: MySQLTypes.zig, ResultSet.zig, and a new regression test.
Security risks
None. This is a pure date-decoding correctness fix with no auth, crypto, or permission surface.
Level of scrutiny
Medium — touches production MySQL wire-protocol decoding on both binary and text paths, but the changes are narrow and mechanical. The fix is well-reasoned: the root cause (local-time constructor) is clearly identified, the fix (UTC constructor) is the right approach, and the regression test exercises all four affected scenarios.
Other factors
All three issues I raised in prior reviews were fixed in autofix.ci commit 094d9c0: (1) the zero-date regression (month < 1 or month > 12 / day < 1 or day > 31 guards added to fromText), (2) the dead-code DateTime.toJS signature mismatch, and (3) the unused afterAll import. The one remaining bug report (binary-protocol zero-date returning epoch 0 instead of NaN) is explicitly pre-existing — it predates this PR and lives in DecodeBinaryValue.zig which was not modified here.
- fromText now rejects impossible calendar values (Feb 30, hour >= 24, etc.) via daysInMonth + bounds checks, so permissive MySQL modes that store garbage rows produce Invalid Date rather than normalized epoch offsets. - DecodeBinaryValue zero-length date payload (MySQL's '0000-00-00' sentinel in the binary protocol) now maps to NaN, matching the text-protocol path.
MySQL permissive sql_mode can store partial zero-dates like '2024-00-15' and send them via the binary protocol as non-zero-length payloads, bypassing the zero-length NaN guard. Validate the components inside toJSTimestamp itself so both the binary (DateTime.fromData) and text (DateTime.fromText) paths surface NaN instead of WTF silently wrapping month=0 into December of the prior year.
There was a problem hiding this comment.
LGTM — all issues from prior rounds have been addressed and the fix is correct.
Extended reasoning...
Overview
This PR fixes MySQL DATETIME/TIMESTAMP decoding to interpret wire-format date components as UTC instead of local time (issue #29208). It modifies four files: MySQLTypes.zig (adds fromText, moves to gregorianDateTimeToMSUTC, fixes toJS signature, adds validation in toJSTimestamp), DecodeBinaryValue.zig (zero-length payload → NaN instead of epoch), ResultSet.zig (text path now uses DateTime.fromText + UTC conversion), and a new regression test.
Security risks
No security-sensitive code is touched. This is purely a data-decoding correctness fix with no auth, crypto, or permissions involvement.
Level of scrutiny
Medium — this is protocol-level code, but the changes are narrowly scoped, well-commented, and all edge cases (zero-dates, partial zero-dates, impossible calendar values) have been explicitly addressed through the review iteration. The fix is mechanically straightforward: swap local-time interpretation for UTC interpretation on both text and binary paths, with validation that mirrors the pre-existing behavior.
Other factors
The iterative review process was thorough: zero-date NaN consistency across protocols, partial zero-date validation in both fromText and toJSTimestamp, DateTime.toJS signature correctness, and test TZ isolation were all raised and fixed. The one pre-existing issue declined (pre-1970 date handling in gregorianDate) is genuinely out of scope for this PR. The bug hunting system found no new issues in the final state.
There was a problem hiding this comment.
LGTM — all review feedback has been addressed.
Extended reasoning...
Overview
The PR fixes MySQL DATETIME/TIMESTAMP decoding on both the binary (prepared-statement) and text (simple-query) protocol paths to interpret naive server-sent date components as UTC rather than local time. Changes span four files: MySQLTypes.zig (new fromText parser, UTC switch in toJSTimestamp, component validation, toJS compile fix), DecodeBinaryValue.zig (zero-date NaN), ResultSet.zig (text path routing through DateTime.fromText), and a new regression test.
Security risks
None. The change is confined to date/time decoding logic in the MySQL SQL driver. No auth, crypto, or permission paths are touched.
Level of scrutiny
Medium — the fix is targeted and well-motivated, touches only the MySQL date decoding path, and includes a round-trip regression test covering both protocols under a non-UTC timezone. The validation logic (month/day/hour/minute/second range checks, leap-year-aware daysInMonth) is straightforward and consistent with the pre-existing helpers already in the file.
Other factors
All substantive inline comments raised in earlier review iterations were addressed: zero-date NaN consistency across protocols, component range validation including partial zero-dates (2024-00-15), time-field bounds, the latent toJS compile error, and the concurrent: true TZ-mutation race in the test. The one declined item (gregorianDate() pre-1970 bug on the write path) is genuinely out of scope for a read-path fix and was correctly deferred. CI failures visible in the robobun comment are pre-existing infra flakes unrelated to this PR.
1 participant
Fixes #29208.
Repro
Under
TZ=Asia/Bangkok,d.toISOString()is2024-01-15T05:30:45.678Zbutrow.ts.toISOString()was2024-01-14T22:30:45.678Z— the client's UTC offset (−7h) subtracted.Root cause
MySQL's DATETIME/TIMESTAMP wire format carries no timezone — the server returns naive
year/month/day/hour/minute/second[/microsecond]components. Bun fed those components through JSC's local-time constructor on both protocol paths:src/sql/mysql/MySQLTypes.zigDateTime.toJSTimestampcalledgregorianDateTimeToMS(which passeslocalTime = true).src/sql/mysql/protocol/ResultSet.zigtext-protocol branch calledbun.String.parseDate, which runs the naive"2024-01-15 12:30:45"through JSC's date parser; without aZ/offset that's interpreted as local.The send path (
DateTime.fromUnixTimestamp) was already UTC-correct, so writes stored the right value and only the read path was wrong — matching the reporter'sSELECT DATE_FORMAT(ts,...)check.Fix
gregorianDateTimeToMSforgregorianDateTimeToMSUTCinDateTime.toJSTimestamp.YYYY-MM-DD[ HH:MM:SS[.ffffff]]directly into aDateTimevia a newDateTime.fromTextand route through the same UTCtoJSTimestamp. This replaces the generic JS date parser for MySQL's text date/datetime rows.Both paths now produce the same UTC epoch, independent of the client TZ, and match the server's stored value.
Verification
test/regression/issue/29208.test.tsflipsprocess.env.TZtoAsia/Bangkokmid-process and round-trips aDatethrough:DATETIMETIMESTAMPDATETIMETIMESTAMPAll four must decode to
2024-01-15T05:30:45.678Z. The test auto-wires against a containerized MySQL when Docker is available and falls back to auto-provisioning abun_testuser against a locally-running MariaDB otherwise.Fail-before (
git stashsrc/, rebuild, run):Pass-after: all green, 3 expect() calls.
Notes
The report also mentions PostgreSQL
TIMESTAMP WITHOUT TIME ZONEbeing off by the client offset in the same scenario. The Bun-side PG binary path (src/sql/postgres/types/date.zigfromBinary) decodes microseconds-since-2000 directly into UTC ms, and bothtimestampandtimestamptztags go through that same function. Any residual PG discrepancy is most likely server-side (how the driver sendsDateastimestamptzOID 1184 and how the server coerces that into a TZ-less column under the session timezone) and is better investigated separately with a live PG instance.