sql(postgres): decode float8/float4 text 'Infinity'/'-Infinity' as ±Infinity, not NaN - #36239
Conversation
WalkthroughChangesPostgres float decoding
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — two-line swap to a strictly-more-capable parser helper, with a hermetic test that pins the exact wire tokens.
Extended reasoning...
Overview
Two-line production change in src/sql_jsc/postgres/DataCell.rs: the scalar float8/float4 text-decode path swaps bun_core::parse_double for bun_core::fmt::parse_f64. A new test file drives a scripted simple-query backend to send the exact Infinity/-Infinity/NaN/1.5 text tokens Postgres' float8out emits, for both float8 and float4, plus a pin on the already-correct float8[] path.
Verified mechanism
I read both helpers in src/bun_core/fmt.rs. parse_double is a partial-match wrapper over WTF__parseDouble with no special-casing; parse_f64 is the full-match variant that additionally recognises sign-prefixed case-insensitive inf/infinity/nan (fmt.rs:1006-1020). Postgres float8out never emits trailing garbage, so the partial→full-match tightening is safe, and the .unwrap_or(f64::NAN) fallback is preserved. bun_core::fmt::parse_f64 is already the established call at four other sites in the tree.
Security risks
None. No untrusted-size arithmetic, allocation, or FFI surface changes — the input bytes flow through the same code path as before, just into a different pure-parse helper.
Level of scrutiny
Low. This is a focused decoder bugfix with a clear root cause (JS-number grammar rejects Infinity), a two-call-site change, and an in-tree helper that already implements exactly the needed behaviour. The test is hermetic (local net.Server on an ephemeral port, no live Postgres), covers both float widths plus a finite control value, and pins the sibling array path.
Other factors
The one inline nit (unused float4_array OID constant / missing float4[] array case) is cosmetic and doesn't block — the production change applies identically to both float widths and float4[] routes through the same parse_array Infinity arm the float8[] test already pins.
There was a problem hiding this comment.
LGTM — two-line swap to bun_core::fmt::parse_f64 for the scalar float8/float4 text path, with the earlier nit (float4[] array coverage) and comment-cop flag both addressed.
What was reviewed:
- Confirmed
parse_f64wraps the sameWTF__parseDoublecall and only adds case-insensitiveinf/infinity/nanarms; the stricter full-input-consumed check can't regress Postgresfloat8outoutput, which never emits trailing bytes. - Test follows the established
wire-frames.tsscripted-backend pattern used by ~25 other files intest/js/sql/; serial tests so the sharedreplyslot is safe. - Verified the follow-up commits: array test now covers both
float8_arrayandfloat4_array, and the flagged inline comment was dropped.
Extended reasoning...
Overview
The PR changes two call sites in src/sql_jsc/postgres/DataCell.rs — the text-format decode arms for T::float8 and T::float4 — from bun_core::parse_double(bytes) to bun_core::fmt::parse_f64(bytes). It adds test/js/sql/postgres-float-infinity.test.ts, which drives a scripted simple-query backend to send text-format Infinity / -Infinity / NaN / 1.5 in float8, float4, float8[], and float4[] columns.
I read bun_core::fmt::parse_f64 (fmt.rs:995-1025) and bun_core::parse_double (fmt.rs:967-978). Both call WTF__parseDouble. parse_f64 differs in two ways: (1) when count == 0 it falls back to case-insensitive inf/infinity/nan matching with optional +/- — exactly the tokens Postgres float8out emits; (2) for numeric input it requires the whole slice to be consumed (count == s.len()) rather than accepting a prefix. Postgres never emits trailing garbage after a float text value, so (2) cannot regress real wire data and is arguably more correct. The .unwrap_or(f64::NAN) fallback is preserved.
Security risks
None. This is a read-side text-to-double conversion of server-sent bytes; no allocation, no unsafe, no user-controlled sizing. The change narrows accepted input (rejects trailing garbage) rather than widening it.
Level of scrutiny
Low. A two-line helper swap in a well-understood decode path, backed by a test that demonstrably fails on main and passes with the fix (evidence block shows both ASAN-debug and release runs). The array path already handled these tokens the same way, so this brings the scalar path into agreement rather than introducing new behaviour.
Other factors
- My prior review left one nit (unused
float4_arrayOID / missing float4[] array coverage); commit 7bd1ddb addressed it by turning the array test intotest.eachover both widths, and the thread is resolved. - The comment-cop bot flagged an inline comment; commit 2111c02 dropped it, and that thread is also resolved.
- The test reuses the established
wire-frames.tshelpers (listeningServer,pgRowDescription,pgDataRow, etc.) and the same shared-server + per-testnew SQLshape used across thetest/js/sql/directory. Tests are serial so the module-levelreplyslot is race-free; each connection is closed infinallyand the server inafterAll.
There was a problem hiding this comment.
LGTM — the earlier nit (unused float4_array OID → now covered by test.each) and the comment-cop flag are both addressed.
What was reviewed:
- Confirmed
bun_core::fmt::parse_f64wrapsWTF::parseDoubleand adds case-insensitiveinf/infinity/nanarms with sign handling, matching what Postgresfloat8outemits. - Checked the semantic delta vs
parse_double(full-match vs prefix-match) — Postgres never emits trailing bytes in float text output, and theunwrap_or(f64::NAN)fallback is unchanged, so no regression for finite values. - Test follows the established
wire-frames.tsscripted-backend pattern from siblingpostgres-infinity-date.test.ts; sequential tests share the module-levelreplysafely since none aretest.concurrent.
Extended reasoning...
Overview
Two-line source change in src/sql_jsc/postgres/DataCell.rs swapping bun_core::parse_double for bun_core::fmt::parse_f64 in the T::float8 and T::float4 text-format arms of from_bytes. New test file test/js/sql/postgres-float-infinity.test.ts (87 lines) drives a scripted simple-query backend to pin scalar float8/float4 and array float8[]/float4[] decoding of Infinity/-Infinity/NaN/1.5.
Security risks
None. This is output-side parsing of bytes the Postgres server sent; the change replaces one in-tree float parser with a strict superset that additionally recognises the IEEE special-value tokens. No new allocation, no untrusted-length arithmetic, no user-controlled control flow.
Level of scrutiny
Low. The source change is a helper swap where I verified the target helper's implementation (fmt.rs:995-1025): it calls the same WTF__parseDouble and, only when zero characters were consumed, falls through to case-insensitive matching of inf/infinity/nan with optional +/-. The one behavioural difference — parse_f64 requires the full input to be consumed whereas parse_double accepted any nonzero prefix — is irrelevant here because Postgres float8out/float4out never emit trailing bytes, and the .unwrap_or(f64::NAN) fallback is retained so degenerate input can't get worse than before.
Other factors
- The PR description includes fails-on-main / passes-on-PR evidence for both debug-ASAN and release builds.
- My previous nit (unused
float4_arrayOID entry / missing float4[] variant coverage) was addressed in 7bd1ddb by turning the array test intotest.each(["float8_array", "float4_array"]); the comment-cop inline-comment flag was addressed in 2111c02. Both threads are resolved. - The test reuses the shared
wire-frames.tshelpers (listeningServer,pgRowDescription,pgDataRow, etc.) exactly as ~20 sibling tests do, and mirrors the structure ofpostgres-infinity-date.test.tsfor the analogous date-infinity fix. - No CODEOWNERS entry covers
src/sql_jsc/postgres/.
|
The diff itself is green: the new CI builds 84189 and 84221 each show a rotating set of pre-existing flaky tests (spawn-streaming-stdout, napi, fetch-tls, streams-leak, node-sqlite GC, etc.), all tagged |
|
Updated 11:05 AM PT - Jul 29th, 2026
❌ @Jarred-Sumner, your commit 4104585 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36239That installs a local version of the PR into your bun-36239 --bun |
…nfinity, not NaN Postgres emits the literal tokens Infinity / -Infinity / NaN for float8 and float4 values in text format (float8out in src/backend/utils/adt/float.c). The scalar text decoder parsed these with WTF::parseDouble, which follows JS-number grammar and rejects those tokens, so the unwrap_or(NAN) fallback turned every ±Infinity into NaN. The float8[] array text decoder already special-cased the same tokens, so a value decoded as NaN in a scalar column and Infinity inside an array of the same type. Switch the scalar float8/float4 text path to bun_core::fmt::parse_f64, which wraps WTF::parseDouble and adds the inf/infinity/nan arms the array path already has.
4575d51 to
271bec7
Compare
There was a problem hiding this comment.
LGTM — two-line helper swap, and the earlier nit (float4[] array coverage) is addressed.
What was reviewed:
- Confirmed
bun_core::fmt::parse_f64wraps the sameWTF__parseDoubleand only adds theinf/infinity/nanarms; ordinary numeric text is unchanged. parse_f64is full-match vsparse_double's prefix-match, but Postgresfloat8out/float4outnever emit trailing bytes and the.unwrap_or(NaN)fallback is retained.- Test uses the shared
wire-frames.tsscripted backend on an ephemeral port; tests run serially so the sharedreplystate is safe.
Extended reasoning...
Overview
The PR swaps bun_core::parse_double for bun_core::fmt::parse_f64 in the two scalar text-decode arms (T::float8 and T::float4) of src/sql_jsc/postgres/DataCell.rs. parse_f64 calls the same WTF__parseDouble FFI and, when zero bytes are consumed, additionally matches sign-prefixed case-insensitive inf/infinity/nan — exactly the tokens Postgres's float8out emits. The array text path (parse_array) already handled these tokens via explicit starts_with_case_insensitive_ascii checks, so this brings the scalar path into agreement. A new test file drives a scripted simple-query backend from the existing test/js/sql/wire-frames.ts helper to pin both scalar (float8, float4) and array (float8[], float4[]) decoding of Infinity/-Infinity/NaN/1.5.
Security risks
None. This is a pure decode-side change to how a text token from a Postgres server is turned into a JS number. No new allocations, no untrusted-length arithmetic, no user-controllable code paths beyond what already existed. The .unwrap_or(f64::NAN) fallback is preserved, so malformed input still degrades to NaN rather than erroring or panicking.
Level of scrutiny
Low. Two identical one-line changes replacing one in-tree helper with another whose implementation I read (src/bun_core/fmt.rs:995-1020). The only semantic difference beyond the inf/nan arms is that parse_f64 requires a full match while parse_double accepted a numeric prefix — irrelevant here because Postgres's float text output is always a clean token, and the NaN fallback covers anything unexpected. The PR includes fails-on-main / passes-on-PR evidence.
Other factors
My earlier nit (unused float4_array OID entry / missing float4[] variant coverage) was addressed in 7bd1ddb by turning the array test into a test.each over both array widths. The comment-cop flag was resolved in 2111c02. All review threads are marked resolved. The test reuses the established wire-frames.ts scripted-backend pattern (ephemeral-port TCP server, no external network), closes the SQL client in finally and the server in afterAll, and the shared module-level reply state is safe because the tests are not test.concurrent. CI failures noted in the thread are pre-existing flakes unrelated to test/js/sql/.
There was a problem hiding this comment.
LGTM — two-line helper swap with a scripted-backend regression test; the earlier nit (float4_array coverage) is addressed.
What was reviewed:
- Confirmed
bun_core::fmt::parse_f64wraps the sameWTF__parseDoubleasparse_doubleand only adds case-insensitiveinf/infinity/nanarms plus a full-match requirement — no behaviour change for the canonical decimal strings Postgres emits. - Checked the remaining
parse_doublecall in the array numeric arm: it's only reached after the tokenizer has already consumedInfinity/NaN, so no sibling fix needed. - Test imports all resolve against the existing
test/js/sql/wire-frames.tsharness; the sharedreplymutable is safe under sequentialtest.each.
Extended reasoning...
Overview
The source change is exactly two identical lines in src/sql_jsc/postgres/DataCell.rs: the T::float8 and T::float4 text-format arms swap bun_core::parse_double(bytes).unwrap_or(f64::NAN) for bun_core::fmt::parse_f64(bytes).unwrap_or(f64::NAN). I read both helpers in src/bun_core/fmt.rs: they call the same WTF__parseDouble FFI, but parse_f64 (a) requires the whole input to be consumed rather than accepting a prefix, and (b) falls back to case-insensitive inf/infinity/nan with optional sign when WTF consumed zero bytes. Postgres float8out only ever emits a canonical decimal or one of those three tokens, so (a) is a no-op on real wire data and (b) is exactly the bug fix.
The new test file drives a scripted simple-query backend via the existing test/js/sql/wire-frames.ts helpers, sending text-format Infinity/-Infinity/NaN/1.5 for float8, float4, float8[], and float4[]. The evidence block shows it fails on the debug build without the fix and passes with it.
Security risks
None. Safe Rust, identical &[u8] -> Option<f64> shape, no allocation, no user-controlled indexing. The stricter full-match semantics of parse_f64 are, if anything, more defensive against malformed wire bytes than the previous prefix-accepting parse_double.
Level of scrutiny
Low. This is a mechanical helper substitution in a text-decode path with a demonstrated fails-before/passes-after test. The array path in the same file already handled these tokens via its own tokenizer, so the fix brings the scalar path into agreement rather than introducing new behaviour. I checked the one remaining parse_double call in parse_array's numeric arm — it's only reachable for strings starting with [-0-9] after I/i/N have been peeled off, so it never sees infinity/nan and doesn't need changing.
Other factors
My earlier nit about the unused float4_array OID entry was addressed in 7bd1ddb by turning the array test into a test.each over both widths, and the comment-cop flag on the source file was resolved in 2111c02. All inline threads are resolved. The test uses a module-level reply mutable shared across a single listeningServer, which is safe because test.each runs sequentially and each runSimple opens a fresh max: 1 connection; the pattern matches pgMinimalReadyServer in the shared harness. CI on the touched test file is green per the robobun summary.
- fetch protocol types missing http3/h3: oven-sh/bun#39773 - Bun.write(path, archive) ignoring compress: oven-sh/bun#30234 - Postgres float8 text Infinity decoding to NaN: fix pending in oven-sh/bun#36239
Fixes #39777.
What does this PR do?
Postgres emits the literal tokens
Infinity/-Infinity/NaNforfloat8andfloat4columns in text format (float8outinsrc/backend/utils/adt/float.c). The scalar text decoder parsed these withWTF::parseDouble, which follows JS-number grammar and rejects those tokens, so the.unwrap_or(f64::NAN)fallback silently turned every ±Infinity into NaN.The
float8[]array text decoder already special-cased the same tokens, so the same wire value decoded asNaNin a scalar column andInfinityinside an array:Fix
Switch the scalar
float8/float4text path frombun_core::parse_doubletobun_core::fmt::parse_f64, which wrapsWTF::parseDoubleand adds theinf/infinity/nanarms the array path already has.How did you verify your code works?
New test
test/js/sql/postgres-float-infinity.test.tsdrives a scripted simple-query backend that sends text-formatInfinity/-Infinity/NaN/1.5infloat8andfloat4columns. Fails on main (Expected: Infinity, Received: NaN), passes with this change. Also pins the already-correctfloat8[]array behaviour so the two paths stay consistent.[review] gate passed · iteration 8 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 8
evidence per changed file