Conversation
|
Updated 8:59 PM PT - Aug 27th, 2026
❌ @robobun, your commit d4372bf has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34708That installs a local version of the PR into your bun-34708 --bun |
WalkthroughChangesPostgres int4 parameters now reject non-finite and out-of-range values. Prepared-statement write paths checkpoint and restore the protocol buffer on failure, with tests covering boundaries, overflow cases, and pooled connection reuse. Postgres int4 safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, cause, fix, and verification results. It provides concrete examples and test evidence. Although the headings differ from the repository template, all required information is present. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sql_jsc/postgres/PostgresSQLQuery.rs`:
- Around line 719-727: Add write-buffer rollback guards to the named
Parse/Describe plus Sync path in PostgresSQLQuery and the matching phase-1 write
in PostgresSQLConnection::advance(). Mark the buffer before each operation, and
roll back to that mark whenever PostgresRequest::write_query(), the named
writes, or the follow-up SYNC write fails, preventing partial frames from
remaining queued.
In `@test/js/sql/postgres-int4-param-overflow.test.ts`:
- Around line 117-119: Update the wire assertion in the overflow test to require
lastBoundInt4 to be exactly undefined, replacing both negative saturation checks
while preserving the existing bind/reset behavior.
🪄 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: fce9cb47-55a4-4895-961c-a34ad56c0dda
📒 Files selected for processing (4)
src/sql_jsc/postgres/PostgresRequest.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rstest/js/sql/postgres-int4-param-overflow.test.ts
|
CI build 107235 (after rebase onto main) is complete: 180 of 181 jobs passed. The new test The one red lane is Ready for review. |
|
Verify this behavior is what Postgres.js and other clients do. |
|
Verified against a real PostgreSQL server, inserting into an
postgres.js and node-pg send every parameter as text and let the server reject. Bun behaves a bit differently because of client-side signature detection ( The repro in the issue hits the same arm by having the mock server return So: outcome matches postgres.js / node-pg (reject, never silently store the wrong integer). The difference is that in the server-inferred-int4 case Bun rejects client-side with |
|
Cross-reference: #39452 (Date and object parameters with |
write_bind encoded int4 parameter values with coerce::<i32>, which saturates values outside the i32 range to INT32_MIN/INT32_MAX. Binding 2147483648 or 4294967297 to an int4 column silently wrote 2147483647, whereas every other client sends the value as text and receives 'integer out of range' (22003) from the server. Coerce to i64 first and return AnyPostgresError::Overflow when the value does not fit in an i32, so the query rejects with ERR_POSTGRES_OVERFLOW instead of persisting the wrong integer.
… error Address review feedback on the int4 overflow fix: - NaN was still silently bound as 0 because coerce::<i64> maps NaN to 0 before the i32::try_from range check. Reject it up front so it surfaces as ERR_POSTGRES_OVERFLOW like the other non-finite values. - write_bind streams into the connection's write_buffer directly, so returning Err mid-serialize left a partial 'B...' frontend message in the buffer which register_auto_flusher would later flush to the socket, poisoning the pooled connection for the next query. Snapshot the write buffer before each bind_and_execute / parse_and_bind_and_execute / prepare_and_query_with_signature call and truncate back on Err. Adds NaN to the overflow test table and a connection-reuse test that runs a second query on the same pooled connection after an overflow rejection.
The rollback guarantees the partial Bind is truncated before flushing, so lastBoundInt4 stays undefined. A single toBeUndefined() is stronger than the two not.toBe saturation checks it replaces.
434f71d to
a24fd4b
Compare
|
Closing in favor of #41976 and #34732. With #41976 the |
What
Binding a JS number outside the
i32range (orNaN) to anint4parameter silently saturated toINT32_MIN/INT32_MAX(or0) and stored the wrong value. A real PostgreSQL server rejects the same literal withinteger out of range(SQLSTATE 22003), so every other client path surfaces a loud error while Bun quietly persisted the saturated value.Cause
write_bindencodedint4parameter values viavalue.coerce::<i32>(), whose number fast path is a saturatingf64 as i32cast (and mapsNaNto0):https://github.com/oven-sh/bun/blob/98f664962f/src/sql_jsc/postgres/PostgresRequest.rs#L204-L213
so
2147483648became2147483647,4294967297became2147483647,-2147483649became-2147483648, andNaNbecame0, all without an error.Fix
NaNup front, then coerce toi64andi32::try_fromthe result, returningAnyPostgresError::Overflowwhen it does not fit. In-range values (including thei32boundaries and truncation of fractional parts) behave exactly as before.write_bindstreams directly into the connection'swrite_buffer, so erroring out mid-serialize would have left a partialB...frontend message in the buffer that the auto-flusher later shipped to the socket, poisoning the pooled connection for the next query. Every call site that reacheswrite_bindnow snapshots the write buffer first and truncates back onErr(also covers the pre-existing jsonb / throwing-coercion error paths).Verification
test/js/sql/postgres-int4-param-overflow.test.tsruns a mock server that declares$1as oid 23 and asserts that out-of-range and non-finite values reject withERR_POSTGRES_OVERFLOW(and that no saturated2147483647/-2147483648reaches the wire), thatINT32_MAX/INT32_MIN/0still round-trip, and that a follow-up query on the samemax: 1pooled connection still works after an overflow rejection.[review] gate passed · iteration 3 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 3
evidence per changed file