sql(postgres): discard a partial Bind when a parameter fails to encode

Merged
RO
2mo ago
+512-94

Problem

  • write_bind (src/sql_jsc/postgres/PostgresRequest.rs:52) writes into connection.write_buffer and calls JS per parameter. When a parameter fails (a throwing toString, since #41889 a bad bytea value), the query rejects but the partial Bind stays: 42 00 00 00 00 (length 0).
  • The next flush sends it, even with nothing queued: P D S B(len=0), or P D B(len=0) with prepare: false. PostgreSQL drops the connection. The next query, pipelined siblings and open transactions get ERR_POSTGRES_CONNECTION_CLOSED.

Fix

  • NewWriter::atomically records offset(), runs the body, and truncates back on failure. It wraps the three batch writers that reach write_bind: a rejected query writes nothing.
  • It truncates only when write_epoch is unchanged. The connection bumps it for every Writer it hands out and every drain or free of write_buffer. A conversion that starts a query with .execute() changes it, and the connection then fails as on main.
  • Writer::pwrite now adds head (the bytes already sent) to the index, like offset().
  • Verified: postgres-bind-encode-throw.test.ts, postgres-bytea-bind.test.ts (unfixed main fails 4/5 and 6/12). Other suites: Notes.

Background

  • prepare: false writes Parse, Describe, Bind, Execute, Flush, Sync as one batch, so the rollback drops them too.
  • This is the interim fix for 1.4.3. The follow-up converts every parameter before the first write, so no rollback is needed.

Downsides

  • Success path, per batch: two reads and one branch. Per Writer: one increment. Per length patch: one add.
  • A conversion that dispatches a query and then fails still loses the connection, as on main. Without the failure both hang (#38231). The follow-up fixes both.
Notes

Wire captures on unfixed main (367d939d9), byte-capturing mock server. Each line is the list of frontend messages after the startup packet.

  • Lone rejected query, named statement: P D S B(len=0). The bytes: 42 00 00 00 00 00 50 73 65 6c 65 63 74 ... (Bind, length 0, empty portal name, then the statement name).
  • Lone rejected query, prepare: false: P D B(len=0). The bytes: 42 00 00 00 00 00 00 00 02 00 00 00 00 00 02 00 00 00 01 61. The Parse and Describe have no Sync behind them.
  • Nothing is queued behind the rejected query in both captures. The auto-flusher sends the buffer at the end of the tick.
  • With the fix: the named case sends P D S and no Bind. The prepare: false case sends nothing for the rejected query.

Paths covered by the tests.

  • bind_and_execute from advance (the Bind follows the statement's Parse/Describe round trip): both files, real server and mock.
  • bind_and_execute from run (statement already prepared, Bind written at query time): postgres-bytea-bind.test.ts runs each rejected query twice. The second run takes this path.
  • parse_and_bind_and_execute (prepare: false): real server with a queued sibling, real server with a lone query and a backend pid check, and the mock that asserts the exact message list.
  • prepare_and_query_with_signature has no parameters, so no parameter can throw there. It is wrapped for the TooManyParameters path.
  • The bytea case (#41889): a number[] bound to bytea rejects with ERR_INVALID_ARG_TYPE. On unfixed main the next query, the pipelined siblings and the open transaction then fail with ERR_POSTGRES_CONNECTION_CLOSED.

The mock server in postgres-bind-encode-throw.test.ts drops the connection when it reads a length below 4, as PostgreSQL does. On unfixed main the mock tests fail at once with ["B(len=0)"]. They do not wait for a timeout.

Nested dispatch from inside a conversion (found in review, confirmed on a real PostgreSQL). query.execute() calls into the connection synchronously (await/.then() defer by one microtask and are not affected). When a parameter's toString creates a query for a prepared statement on the same connection and calls .execute() on it, the nested query writes its Bind/Execute/Sync inside the outer query's open Bind.

  • Conversion does not throw, main and this PR: the outer query never settles, the nested one rejects with 08P01 insufficient data left in message, and later queries on the connection hang. #38231 is the open fix.
  • Conversion throws after the nested dispatch, main and this PR: B(len=0) goes out, the server drops the connection, the nested query rejects with ERR_POSTGRES_CONNECTION_CLOSED, the pool reconnects and the next query resolves.
  • An earlier head of this PR (9812be4b06) rolled back without the write_epoch check. It removed the nested query's frames too: the nested query hung, or a later query's row resolved it (nested: resolved [{"nested":"LATER-VALUE"}]). The test a query dispatched from inside a conversion that then fails never gets another query's row times out on that head and passes now.
  • Checked 5 cells against main on a real server (outer statement already prepared or on its first execution, nested query prepared, new, simple, or unsafe with parameters): the outcomes are the same as on main in every cell. A sixth cell (first execution, nested new statement) aborts the debug build with panic: pending_requests underflow, a debug assertion that this diff does not touch. The release build of main has no panic in that cell.

head in pwrite and truncate. offset() is relative to head, so both now add head. No test reaches head != 0 during a write, and the public API does not reach it without a flush from inside a conversion. OffsetByteList::consume leaves head > 0 only when a socket write accepted less than half of the pending bytes. That write sets HAS_BACKPRESSURE. Under backpressure advance() does not run its loop, and every write gate in do_run needs !has_query_running() or can_pipeline(), which are both false while a request with pending bytes is in the queue. A flush from inside a conversion bumps write_epoch, so no rollback follows it.

Follow-up. A design review of six candidates picked a different long-term shape: convert every parameter before the first byte of the batch is written (no user code runs while a frame is open), plus a guard so that a query dispatched during a conversion only enqueues. It is planned as three PRs. This PR is the interim fix. See https://github.com/oven-sh/bun/pull/34732#issuecomment-5815314380.

Suites run with the debug build: the test/js/sql/postgres-*.test.ts files, sql-prepare-false.test.ts, sql-pool-transaction-isolation.test.ts, wire-frames.test.ts and sql.test.ts (29 files): 236 of 236 tests pass, with postgres-string-leak.test.ts run apart. The two tests in postgres-string-leak.test.ts time out at the default 5 s in this environment: the fixture alone takes 8.6 s under the debug ASAN build here (0.46 s under release). Its RSS delta is 9.65 MiB, inside the test's 80 MiB bound.

History. The July version of this PR had the same design. It was refreshed on current main after the ArrayList writer context was removed. The test file postgres-bind-throw-torn-frame.test.ts was replaced by postgres-bind-encode-throw.test.ts. The head now also carries the postgres-bytea-bind.test.ts additions from the branch robobun/44cd9150/postgres-bind-rollback.

MySQL is not affected. bind_and_execute_impl converts every parameter to a native Vec<Value> before it touches the writer.

[human-review] gate passed · iteration 2 · 4 files touched

fails on main (without fix) 
ASAN without fix: 3 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/sql/postgres-bind-encode-throw.test.ts
bun test v1.4.3 (f42e98025)

test/js/sql/postgres-bind-encode-throw.test.ts:
Container ready via docker-compose: postgres_plain at 127.0.0.1:5432
204 |   return `{${values.map(arrayValueSerializer.bind(this, type, isPostgresNumericType(type), isPostgresJsonType(type))).join(delimiter)}}`;
205 | }
206 | function wrapPostgresError(error) {
207 |   if (Error.isError(error)) {
208 |     return error;
209 |   return new PostgresError(error.message, error);
               ^
PostgresError: Connection closed
 code: "ERR_POSTGRES_CONNECTION_CLOSED"

      at wrapPostgresError (internal:sql/postgres:209:10)
      at handleClose (internal:sql/shared:463:27)
(fail) postgres > a parameter that throws while encoded does not break the queries pipelined behind it [308.18ms]
204 |   return `{${values.map(arrayValueSerializer.bind(this, type, isPostgresNumericType(type), isPostgresJsonType(type))).join(delimiter)}}`;
205 | }
206 | function wrapPostgresError(error) {
207 |   if (Error.isError(error)) {
208 | 
... (truncated)

release without fix: 3 FAILED
bun test v1.4.3-canary.1 (f42e98025)

test/js/sql/postgres-bind-encode-throw.test.ts:
Container ready via docker-compose: postgres_plain at 127.0.0.1:5432
171 |   let delimiter = type === "BOX" ? ";" : ",";
172 |   return `{${values.map(arrayValueSerializer.bind(this, type, isPostgresNumericType(type), isPostgresJsonType(type))).join(delimiter)}}`;
173 | }
174 | function wrapPostgresError(error) {
175 |   if (Error.isError(error))
176 |   return new PostgresError(error.message, error);
               ^
PostgresError: Connection closed
 code: "ERR_POSTGRES_CONNECTION_CLOSED"

      at wrapPostgresError (internal:sql/postgres:176:10)
      at handleClose (internal:sql/shared:372:27)
(fail) postgres > a parameter that throws while encoded does not break the queries pipelined behind it [13.17ms]
171 |   let delimiter = type === "BOX" ? ";" : ",";
172 |   return `{${values.map(arrayValueSerializer.bind(this, type, isPostgresNumericType(type), isPostgresJsonType(type))).join(delimiter)}}`;
173 | }
174 | function wrapPostgresError(error) {
175 |   if (Error.isError(error))
176 |   return new PostgresError(error.message, error);
               ^
PostgresError: Connection cl
... (truncated)
passes on PR (with fix) 
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/sql/postgres-bind-encode-throw.test.ts
bun test v1.4.3 (f42e98025)

test/js/sql/postgres-bind-encode-throw.test.ts:
Container ready via docker-compose: postgres_plain at 127.0.0.1:5432
(pass) postgres > a parameter that throws while encoded does not break the queries pipelined behind it [297.21ms]
(pass) postgres > a throwing parameter on the first execution of a statement does not break the next query [38.94ms]
(pass) postgres bind encode failure (mock server) > no partial Bind reaches the wire when a parameter throws [368.75ms]

 3 pass
 0 fail
 6 expect() calls
Ran 3 tests across 1 file. [3.42s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 771ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/38] gen generated_host_exports.rs
generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 242 extern-C blocks audited
[2/38] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
  - ResolveMessage (15 fields)
  - BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
  - Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
  - ResourceUsage (8 fields)
  - Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
  - CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
  - FileSystemRouter (5 fields)
  - FrameworkFileSystemRouter (2 fields)
  - MatchedRoute (8 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts
  - Glob (5 fields)
Found 1 classes from /workspace/bun/src/runtime/api/h2.classes.ts
  - H2FrameParser (32 fields)
Found 9 
... (truncated)
diff hotspot 
src/sql/postgres/protocol/NewWriter.rs         |  15 ++
 src/sql_jsc/postgres/PostgresRequest.rs        | 192 +++++++++++++------------
 src/sql_jsc/postgres/PostgresSQLConnection.rs  |  12 ++
 test/js/sql/postgres-bind-encode-throw.test.ts | 182 +++++++++++++++++++++++
 4 files changed, 308 insertions(+), 93 deletions(-)

gate history · 2 passed · 0 rejected · iteration 2

evidence per changed file 
file                                            reads  edits  tests
src/sql/postgres/protocol/NewWriter.rs              2      4      0
src/sql_jsc/postgres/PostgresRequest.rs             2      3      1
src/sql_jsc/postgres/PostgresSQLConnection.rs       2      1      0
test/js/sql/postgres-bind-encode-throw.test.ts      0      0      0