sql(postgres): reject only the query whose row the client cannot decode - #43187
Conversation
A DataRow can be framed correctly and still hold a value the client cannot turn into a JS value (a binary int4[] with two dimensions, a text array with explicit bounds, a jsonb text that is not JSON). The DataRow handler returned that error to on_data, which failed the connection and rejected every queued query with the first query's error. Pipelined queries had already been executed by the server at that point. The handler now reads the frame to its end and rejects the current request alone. The request stays in flight at the head of the queue until its ReadyForQuery, and the rest of its response is skipped, so advance() cannot pop it early and hand its remaining rows to the next request. Framing errors still fail the connection.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughPostgreSQL row decoding now separates client decode failures from protocol errors. Affected queries reject while remaining in flight, skip their remaining results, and finish at ChangesPostgreSQL row decode rejection
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to A PostgreSQL query whose result row cannot be decoded now fails only that query and leaves the connection usable for pipelined and subsequent queries, with documentation noting that the query may already have executed on the server. The earlier concern about a crash on a split network read does not apply, since rows are only decoded once fully received. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fix and tests are pushed. How I reproduced it, on main b52d513 and on canary 1.4.3 (c6b7fcb), against a real postgres with const q = v => sql`select ${v}::int4[] as v`;
await q("{0}"); // prepares the statement, so the next four are pipelined
await Promise.allSettled([q("{1}"), q("{{1,2},{3,4}}"), q("{2}"), q("{3}")]);
Test: |
|
Updated 8:42 PM PT - Sep 18th, 2026
✅ @robobun, your commit aa08aa17961ef1936d2f0a2bc75271e0fcdaf1c6 passed in 🧪 To try this PR locally: bunx bun-pr 43187That installs a local version of the PR into your bun-43187 --bun |
…h failure paths in the mock tests The rejected query may be an INSERT ... RETURNING that the server already stored, and the message is the same "Failed to read data" a dead socket gives. The rejection now carries a hint that says only the decoding of the result failed. The mock tests run once with a text[] cell (fails while the DataRow is read) and once with a jsonb cell (fails when the row becomes a JS object).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline note, I also checked two paths that could double-settle a discard_response request: a server ErrorResponse (or clean_up_requests on socket close) arriving after the local rejection goes through on_js_error -> reject, which is a no-op once get_target(.., true) has cleared the cached target, and finish_request is idempotent via RequestCounter::None. The remaining ungated handlers (RowDescription, BindComplete, NoData, ParseComplete) only mutate statement/status state and never resolve the promise, so they need no is_rejected() gate.
Extended reasoning...
This run's inline finding is a test-helper duplication nit; the native change itself is a query-lifecycle state-machine change in the Postgres driver (a rejected request kept at the queue head until ReadyForQuery), which is not simple enough to approve without a human look, and one further verified finding is unposted. I traced the new discard_response flag through every consumer (DataRow, CommandComplete, CloseComplete, EmptyQueryResponse, ReadyForQuery), confirmed advance() treats the still-Running request as in flight, confirmed a second rejection via ErrorResponse or clean_up_requests is harmless because reject early-returns once the cached target is zeroed and this_value is downgraded, and confirmed postgres_error_to_js_with_hint on JSError takes the pending exception before decode_result? can propagate a framing error. Nothing concrete beyond the inline finding turned up.
… error tests The echo mock of postgres-row-decode-error.test.ts repeated the startup and frame loop of pgMockServer to be able to keep back part of a reply. pgMockServer now accepts pgHold in a reply and returns release(), and the test mock is a responder on top of it.
There was a problem hiding this comment.
I re-reviewed the latest revision (including the pgHold/release() refactor that replaced the duplicated mock loop) and found no bugs, but because this changes the request lifecycle of the Postgres state machine, a human look is still worthwhile before merging.
What was reviewed:
- Every former
status == Failskip site now goes throughis_rejected();ReadyForQueryis the only consumer that clears the in-flight request, andfinish_requeststays idempotent there (counter reset toNone,Failarm is a no-op) if anErrorResponsefor the same query already ran it. - Double-rejection paths (server
ErrorResponseafter the local rejection,clean_up_requestson close):rejectclearstargeton the first call, so the secondrun_callbackis skipped and the counters are only decremented once. undecodable_row_errortakes theJSErrorcell's pending exception beforedecode_result?can return a framing error; the cell-cleanup guard still runs on both newreturn Ok(())exits.- Mock tests:
pgBindParametersoffsets match the Bind body layout;release()only writes frames that were actually held.
Extended reasoning...
Overview
The PR changes how PostgresSQLConnection::on handles a DataRow whose cell cannot be decoded (src/sql_jsc/postgres/PostgresSQLConnection.rs): instead of propagating the error with ? (which made on_data call fail() and close the socket, rejecting every pipelined query), the cell error is captured in an Option, DataRow::decode finishes consuming the frame, and only the current request is rejected via the new PostgresSQLQuery::on_undecodable_row, which sets a new Flags::discard_response bit while leaving status in flight. is_rejected() (Fail or discard_response) replaces the four status == Fail checks. ReadyForQuery finishes such a request and flips it to Fail so advance() can pop it. error_jsc.rs gains a hint-carrying variant of postgres_error_to_js. Tests add a container-backed file plus mock fault-injection cases, and wire-frames.ts gains pgBindParameters and a hold/release facility on pgMockServer.
Security risks
None specific to this change. The wire parsing itself (DataRow::decode, framing checks) is unchanged; framing errors still propagate and fail the connection. The new code only changes which promise is rejected and when the request leaves the queue. No new allocations from untrusted lengths, and the JSError path consumes the pending exception through the existing take_exception route in postgres_error_to_js_with_hint.
Level of scrutiny
High: this is a request-lifecycle change in a pipelined protocol state machine where a wrong pop order silently hands one query's rows to the next. I traced: (1) advance() and clean_up_requests treat the discard_response request as Running, so it is not popped early and, on connection close, gets finish_request + a no-op second reject (target already cleared by get_target(.., true)); (2) ErrorResponse arriving after the local rejection calls finish_request (decrements counters, sets counter to None) and on_js_error (no-op reject), and the later ReadyForQuery discard branch calls finish_request again on a now-Fail request, which is a no-op — counters are balanced once; (3) simple multi-statement queries in PartialResponse take the discard branch before the PartialResponse branch, so on_result(is_last) is not fired for an already-rejected query; (4) the on_js_error refactor moves status.set(Fail) before ref_guard() acquisition, which is a plain Cell write and cannot run JS, so it is behavior-preserving; (5) the scopeguard::defer! cell cleanup is declared before both new early returns, and putter.count is read after decode returns as before.
Other factors
The mock tests use an in-process net server, port: 0, describe.concurrent, and await using/finally for cleanup; the pgBindParameters parser reads portal, statement, format-code count, and parameter count in the documented Bind order. The container tests exercise the real binary int4[] and text-bounds decoders. I could not run the suite in this environment (no debug build and test execution was unavailable), so correctness of the test expectations rests on reading; the PR author reports all six new tests fail without the fix. The earlier duplicated startup-loop nit was addressed in the last commit by moving hold/release into pgMockServer. The MySQL sibling defect is explicitly deferred to a paired PR, which the description names.
…ecode error tests The cell loop of the DataRow handler is the same as before again: two monomorphized DataRow::decode calls, and a failed cell is marked with inspect_err. DataRow::decode skips the rest of the row when its callback fails, so the frame is still consumed whole. Tests: the undecodable rows now have a cell after the bad one (without the skip these fail), a row that is wider than the inline cell buffer, a real-server case where the rest of the rejected query's rows arrives in later reads while the client walks its queue and collects the query wrapper, and an INSERT ... RETURNING that is stored although it rejects. The hint names the workaround, and the docs say the same.
… and write nothing past a statement being parsed The ErrorResponse arm released the counter of the failed request and marked it Fail before the ReadyForQuery of its Sync. With IS_READY_FOR_QUERY still set from an earlier pipelined query, the next enqueue popped the failed request and wrote a Parse. The late ReadyForQuery then let advance() step over that request and write the Bind of the next one first. Replies go to the requests in queue order, so each of the two queries resolved with the rows of the other. The request now rejects through reject_in_flight, which is the on_undecodable_row of #43187 with a new name. It keeps its status, its counter and the head of the queue until its ReadyForQuery. advance() stops at a request whose statement is being parsed, as it did in #20986.
2 participants
Problem
Bun.SQL(postgres): a reply holds a value the client cannot decode, such as'{{1,2},{3,4}}'::int4[]in a parameterised query. Every other query on that connection then rejects with that query's error,ERR_POSTGRES_MULTIDIMENSIONAL_ARRAY_NOT_SUPPORTED_YET(Failed to read data). With pipelining, the server already executed them.DataRowarm ofPostgresSQLConnection::on(src/sql_jsc/postgres/PostgresSQLConnection.rs) returned the decode error with?.on_datathen callsfail(): the socket closes and the whole queue rejects.Fix
DataRow::decodeskips the rest of the row when a cell fails. The arm rejects only the current request: the same error, plus ahintthat the query may have run. APutter::to_jserror takes the same path.ReadyForQuery. The newdiscard_responseflag skips the rest of its reply.Failat once,advance()popped it early and the next query resolved with its leftover row. Three tests cover this.test/js/sql/postgres-row-decode-error.test.ts(5 tests on a real postgres, 4 on a mock, all fail without the fix), and everytest/js/sql/postgres-*.test.ts. Self-reviewed: 6 concerns raised, 6 addressed (Notes).Background
Bun.SQLpipelines: it writes several prepared queries before the first reply arrives. Each reply message goes to the request at the head of a FIFO queue.ReadyForQuery.advance()then pops finished requests and writes queued ones.advance()can also run mid-reply, and it pops a head with statusFail. After anErrorResponsethat is harmless: the server sends nothing more.Notes
Reproduction against a real postgres,
max: 1, before the fix:After the fix, only query 2 rejects, and
pg_backend_pid()is the same before and after. The same happens with.simple()queries and'[0:2]={1,2,3}'::int4[](ERR_POSTGRES_UNSUPPORTED_ARRAY_FORMAT), and with a jsonb text that is not JSON (JSON Parse error: Unexpected identifier "not", mock only).Values from a healthy server that reach this path (each one fails only its own query now):
pg_index.indoption::int2[]and otherint2vectorcasts, arrays with explicit bounds,byteawithbytea_output = escapein a simple query, binaryint4[]/float4[]with a NULL element or two dimensions.postgres.js does the same: a parser that throws in
DataRowrejects the current query at once, and the query stays current untilReadyForQuery. The MySQL adapter has the same defect in another form: it rejects only the request, but then hands the rest of that request's response to the next one, and ato_jserror closes the connection. #43323 is the same fix there, and the two PRs are a pair.What this does not change: the query that owns the bad row still rejects, also when it is an
INSERT ... RETURNINGthat the server already stored. Its message is the sameFailed to read dataas before, so the rejection now has ahint: "The query may have run on the server. The client could not decode a value in its result. Cast that column to text, or use .raw()."docs/runtime/sql.mdxsays the same under Data Type Errors, and one test pins it: a rejectedINSERT ... RETURNINGis stored. The write side is separate: a Bind parameter that fails to encode still makes the server close the connection for the pipelined neighbours, and #34732 fixes that.Other shapes I ran by hand on the ASAN debug build against a real postgres:
Bun.gc(true)in the rejection handler while the rest streams. The same with the bad row second to last.sql.begin(): the callback throws on the rejection andROLLBACKruns on the same connection. A callback that catches the rejection continues in the same transaction..values()rejects the same way..raw()does not decode and resolves.ErrorResponse(division by zero) that arrives after the local rejection, for the same query. The query rejects once, with the decode error.A framing error (a
DataRowwhose lengths do not agree with the message length) still fails the connection. A pending termination exception still propagates as a connection failure, as before (undecodable_row_errorreturns the error whenhas_pending_termination_exception()is true).A cell can fail with a JS exception pending (
Date.parseof a 1 GiB text throws out of memory).decodereturns at once on a cell error, andundecodable_row_errorthen takes the exception, so no exception stays pending.The cell loop is the same as before on the success path: two monomorphized
DataRow::decodecalls, and a failed cell is marked on the error edge only (inspect_err). An earlier revision of this PR added two branches per cell, and the self-review measured about 5% more client CPU on a wide select with many NULLs.Self-review, 6 concerns, all addressed: (1) a real-server test where the rest of the rejected query's rows arrives in later reads, with the query wrapper collected in that window (added). (2) and (3) the MySQL adapter has the same defect: named here, fixed in #43323. (4) no test had a cell after the bad one: the undecodable rows are now multi-column, and there is a row wider than the inline cell buffer. (5) the per-cell cost above. (6) the failing query can itself have run: hint, docs and the
INSERT ... RETURNINGtest.Mutation checks on the debug build: with the skip in
decoderemoved, the four multi-column tests fail. With the request markedFailat once, the real-server multi-read test and thetext[]hold test fail.The accounting stays as it was:
pipelined_requestsandnonpipelinable_requestsare released byfinish_requestatReadyForQuery, socan_pipeline()andcan_prepare_query()see the request as in flight while the rest of its reply streams.The mock tests exist because a healthy server validates json on input and does not stop in the middle of a reply on demand. They run with two column kinds, because a row can fail in two places: a
text[]cell with bounds fails while theDataRowis read (Putter::put), and a jsonb cell that is not JSON fails when the row becomes a JS object (Putter::to_js). The mock holds back the rest of the rejected query's reply until the test's rejection handler has enqueued a statement that is not prepared yet. That enqueue callsadvance()at that point.Test runs on the debug build:
test/js/sql/postgres-*.test.tsgives 184 pass and 2 fail. Both failures are the RSS leak tests inpostgres-string-leak.test.ts, which exceed the 5 s default timeout under ASAN. Their fixtures pass when run directly (9.2 s and 5.9 s), and the file passes on the release build. A local run ofsql.test.tsagainst the local postgres has the same failures as the unfixed build, plus tests that time out under the debug build (reserve connectionfails the same way on the unfixed debug build).[human-review] gate passed · iteration 0 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file