Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Updated 1:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 1326309 has some failures in 🧪 To try this PR locally: bunx bun-pr 38231That installs a local version of the PR into your bun-38231 --bun |
|
Status: reproduced on the released binary (1.4.0 canary) against a local postgres with a CI on the current head (build 95374): every job that ran passed, including the new test against the real postgres service on the Linux lanes; the build is marked failed only because the two Related but separate findings from reviewing this: closing the reserved connection from inside a conversion hits the same panic through a different trigger, and |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks re-entrancy in the Postgres connection dispatcher (queue-order vs. wire-order invariants, a new enqueue-before-encode failure path with its own refcount/status handling, and deferred draining/flushing), a human familiar with this state machine should also take a look.
What was reviewed:
IS_DISPATCHINGgating onadvance(),flush_data(), and all three write gates indo_run— every holder drains/flushes after clearing the flag, andscopeguard::defer!clears it on unwind.- Refcount balance on the new
bind_and_executefailure path: the speculativeref_()becomes the queue's ref once enqueued, released bydiscard_requestat head or byadvance()'s Fail sweep otherwise;poll_ref/this_valueupgrade are correctly not taken before the early return. - Test fixture: subprocess-per-scenario with a watchdog that outlives the test timeout, concurrent pipe drain,
describeWithContainer, and exact-value assertions on both outer and nested query results plus conversion count.
Extended reasoning...
Overview
This PR fixes a process-level panic (and the adjacent wire-corruption / hang failure modes) that occurs when a Postgres query parameter's valueOf() / toString() / toJSON() synchronously dispatches another query on the same connection while the outer query's Bind message is still being encoded. It adds ConnectionFlags::IS_DISPATCHING, held by advance() for its whole drain and by do_run's enqueue-time Bind, so that a nested dispatch only enqueues and leaves draining/flushing to the encoder. do_run's prepared-statement fast path is reordered to enqueue (as Binding) before encoding, and a new discard_failed_request helper disposes of the request on encode failure the same way advance() does, then dispatches whatever the user JS enqueued behind it. Nine subprocess-based test scenarios cover both encode sites, nested new/prepared/same statements, a mixed burst, second-order nesting, transactions, prepare: false, and a conversion that dispatches then throws.
Security risks
None identified. The change is defensive against hostile user JS re-entering the dispatcher, but it doesn't touch auth, TLS, or input parsing.
Level of scrutiny
High. This is a re-entrancy fix in the Postgres wire-protocol state machine, where correctness depends on the queue-order == wire-order invariant, intrusive-refcount balance across a new early-return path, and the guarantee that every deferred drain/flush is picked up by whoever holds the flag. The PR description's correctness argument is detailed and internally consistent, and I traced the flag's set/clear (RAII via scopeguard::defer!), the three !dispatching && gates, and the new failure path's refcount handling — but the interaction surface (pending_requests accounting, can_pipeline, has_query_running, the advance_impl cleanup sweep, ReadyForQuery re-driving the loop) is broad enough that a maintainer who knows this subsystem should confirm nothing else relies on flush_data() or advance() acting immediately from inside an encoder.
Other factors
The test coverage is thorough and follows repo conventions (subprocess per scenario since failures abort/hang, describeWithContainer against postgres_plain, watchdog < test timeout so hangs report as structured JSON with stderr, concurrent stdout/stderr/exited drain, test.concurrent, exact-value assertions on every query result and on conversion count). The PR body reports the full sql.test.ts postgres block plus seven adjacent postgres regression suites pass. No prior human review on the timeline.
|
On the one open question above (whether anything relies on
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Because it reworks the postgres dispatch state machine (queue-before-encode in the Prepared fast path, a new re-entrancy guard that gates advance()/flush_data(), and a new error-recovery path that dispatches nested queries), a human look is still warranted.
What was reviewed:
while_dispatchingusesscopeguard::defer!soIS_DISPATCHINGis cleared on every exit, including whenbind_and_executereturnsErr.- Refcount balance on the new encode-failure path: the speculative
this.ref_()is released bydiscard_requestwhen the request heads the queue, or by the laterdefer_cleanup!sweep when it doesn't (matchingadvance()'s own failure disposal). dispatchinggates all three write sites indo_run(simple, Prepared fast path, Parse/prepare path) and the!enqueuedguard prevents double-enqueue.- The 9 subprocess scenarios drain both stdout and stderr concurrently, use
describeWithContaineragainst a local postgres, and assert exact result shapes.
Extended reasoning...
Overview
Adds ConnectionFlags::IS_DISPATCHING, held by advance() and by do_run's enqueue-time Bind, so a query dispatched from user JS inside a parameter's valueOf()/toString() only enqueues rather than re-entering the encoder. flush_data() and advance() become no-ops while the flag is set. The Prepared fast path now enqueues before encoding and, on encode failure, pops the request via discard_failed_request (which then dispatches whatever was queued during the encode) and rethrows the taken exception. Ships a 9-scenario subprocess test against postgres_plain.
Security risks
None — this is internal dispatch ordering; no new parsing of untrusted input, no auth/crypto changes.
Level of scrutiny
High. This is the postgres wire-protocol dispatch state machine: queue ordering vs. wire ordering, pending_requests/pipelined_requests accounting, refcount release on a new error path, and a re-entrancy guard whose correctness depends on every holder draining and flushing afterwards. The PR description's argument for why deferred flushes are never lost is convincing but non-local (spans on(), drain_internal, advance_and_flush, the auto-flusher, and do_run's tail).
Other factors
- Six unresolved comment-cop inline comments remain on the current head (3f82a0f); the author consolidated the explanation at
while_dispatchingbut the linter still fires on the doc comments there and atdiscard_failed_request/advance(). Whether those are acceptable is a maintainer call. - The new error path in the Prepared fast path no longer calls
release_statement()immediately (the oldrelease_query_ref()did); the statement ref now lives until the query'sDrop. It's also held by the connection's statements map so this isn't a leak, but it is a behavioural difference from the previous error path worth a second pair of eyes. - Test coverage is thorough (both encode sites, nested prepared/new/same statement, burst, second-level nesting, transaction,
prepare: false, throw-after-dispatch) and asserts one conversion per query, which is the observable form of the fix.
|
On the statement ref held across the new failure path: that is deliberate. |
…oder Encoding a Bind converts each parameter through user JS (valueOf, toString, toJSON), which can synchronously dispatch another query on the same connection. The nested run() either wrote its own frames into the middle of the half-encoded Bind or re-entered advance(), which bound the still-Pending head request a second time and then flushed the buffer out from under the outer encoder, whose LengthWriter offsets pointed into it (panic in Writer::pwrite; pending_requests underflow in debug builds). Add ConnectionFlags::IS_DISPATCHING, held across advance() and across the enqueue-time bind_and_execute in PostgresSQLQuery::do_run. While it is set, do_run only enqueues, advance() returns immediately and flush_data() is a no-op; the dispatching caller drains and flushes afterwards. The enqueue-time Bind now puts its request on the queue before encoding, so requests dispatched from inside the encode queue up behind it in the order their replies will arrive; if that encode fails, the request is disposed of the way advance() disposes of its failures and anything queued behind it is dispatched before the error is rethrown.
3f82a0f to
ef872de
Compare
ef872de to
1326309
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the postgres dispatch/encode ordering (enqueue-before-encode on the prepared fast path, a new connection-wide IS_DISPATCHING gate on advance()/flush_data(), and a new failure-path cleanup that re-enters advance() while an exception is held), a maintainer familiar with the connection state machine should sign off.
What was reviewed:
while_dispatchingguard: cleared viascopeguard::defer!, callers only enter when!is_dispatching()(bothadvance()anddo_run's fast path), so the debug_assert holds.- Failure path in
do_run: request enqueued asBinding(counter = None, not counted inpending_requests), on encode error markedFailthen popped-if-head / swept-later — matchesadvance_impl's own encode-failure disposal; the statement ref now releases viaDroplike every other enqueued request. discard_failed_request→advance_and_flush(): exception is taken off the VM first; when the failed request was pipelined behind an in-flight one,IS_READY_FOR_QUERYis false so the drain is deferred to that request's ReadyForQuery — covered by the last test scenario.flush_data()no-op inside the guard: every caller ofadvance()flushes or registers the auto-flusher after it returns, andflush_data_and_reset_timeout()insideadvance_implonly registers (doesn't flush directly).
Extended reasoning...
Overview
Fixes a re-entrancy crash in the postgres client: a query parameter whose valueOf()/toString()/toJSON() synchronously dispatches another query on the same connection (any max: 1 pool or transaction) panics in Writer::pwrite or wedges the connection. The fix adds ConnectionFlags::IS_DISPATCHING, held for the duration of any Bind encode (while_dispatching), during which advance() and flush_data() early-return and do_run only enqueues. The prepared-statement fast path in do_run now enqueues the request before encoding it so nested dispatches queue behind it, with a new discard_failed_request cleanup on encode failure. Five files: three native (ConnectionFlags.rs, PostgresSQLConnection.rs, PostgresSQLQuery.rs) and two new test files (10 subprocess scenarios against the postgres_plain container).
Security risks
None. The change only affects internal write-buffer/queue ordering on a single JS thread; no new user-controlled input reaches a parser or allocation, and no auth/TLS/permission code is touched.
Level of scrutiny
High. This is native re-entrancy handling in the postgres wire-protocol dispatch loop, with intrusive-refcount lifetime implications. The change:
- Reorders enqueue-before-encode on the fast path, which changes the failure-path cleanup contract (statement ref released via the query object's
Droprather thanrelease_query_ref(); theref_()onthisis released viadiscard_request'sderefinstead of the local closure). - Adds a connection-wide flag that gates three hot-path functions.
- Has
discard_failed_requestcalladvance_and_flush()(which can re-enter user JS viabind_and_execute) while the caller is holding a taken exception to rethrow.
I traced each of these and they hold up (see the bullets in the message), but this is exactly the shape of change where a maintainer who knows the connection state machine should confirm the invariants — particularly the interaction between the new Binding-before-encode state and finish_request/cleanup_and_fail_all_requests if the connection is torn down mid-encode.
Other factors
- The bug-hunting system found nothing.
- The test matrix is thorough (both encode sites × nested new/prepared/same/burst/recursive, transaction,
prepare: false, throw-after-dispatch at head and behind an in-flight request), runs each in a subprocess, and the PR shows the expected 8/10-fail-without / 10/10-pass-with evidence on both debug+ASAN and release. - All comment-cop inline comments are resolved (comments consolidated at
while_dispatchingwith pointers elsewhere). - No prior
claude[bot]review on this PR.
|
On the teardown-mid-encode interaction raised above, traced for the enqueue-time path: if the connection is closed synchronously from inside a conversion (only reachable through |
|
Heads-up for the fixture here: #41976 (the consolidated |
#34732) ### 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. <details><summary>Notes</summary> **Wire captures on unfixed main (367d939), 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 (9812be4) 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 #34732 (comment). **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. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 2 · 4 files touched <details><summary>fails on main (without fix)</summary> ```console 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 (f42e980) 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 (f42e980) 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) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console 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 (f42e980) 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) ``` </details> <details><summary>diff hotspot</summary> ``` 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(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` 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 ``` </details> <!-- robobun:evidence:end -->
|
Closing in favour of #43918. It carries this fix on current main, stacked on #43892, and its tests include the ten scenarios of this PR. It uses a field of the postgres connection, because the flag bit |
1 participant
Problem
valueOf()/toString()/toJSON()dispatches another query on the same connection (anymax: 1pool, reserved connection or transaction) crashes the process on the first execution of the statement:Writer::pwrite(src/sql_jsc/postgres/PostgresSQLConnection.rs); debug builds trippending_requests underflowfirst.valueOf()is also observably called twice.advance()callsPostgresRequest::bind_and_executefor the head request while it is stillPending, andwrite_bindcalls into user JS per parameter. The nestedexecute()reachesPostgresSQLQuery::do_runsynchronously (src/js/bun/sql.tsonQueryConnected->handle.run), which enqueues and callsadvance_and_flush(): the nestedadvance()binds the head request a second time (two Bind+Execute+Sync for one request) andflush_data()then emptieswrite_buffer. When the outerwrite_bindresumes, itsLengthWriterpatches an offset into the buffer that was just flushed.do_runwrites the Bind itself (PostgresSQLQuery.rs,StatementStatus::Preparedfast path) before the request is on the queue. A nested dispatch sees an idle connection, writes its own frames into the middle of the half-encoded Bind and enqueues itself first, so the server's error for the torn message is delivered to the nested query and the outer one never settles. A nested query reusing a prepared statement takes the same fast path from insideadvance()too (pending_requestswas already decremented for the request being encoded), corrupting the wire the same way.Fix
ConnectionFlags::IS_DISPATCHING, held byadvance()for the whole drain and bydo_run's enqueue-time Bind for the duration ofbind_and_execute(PostgresSQLConnection::while_dispatching). While it is set,do_runonly enqueues (all three of its write gates),advance()returns immediately andflush_data()is a no-op.do_run's fast path now enqueues the request (asBinding, with itsRequestCounterstillNone, so a cleanup during the encode is a no-op for it) before encoding it. If the encode fails,discard_failed_requestdisposes of it exactly asadvance()disposes of its own failures (dropped at the head, otherwise leftFailfor the sweep), dispatches anything user JS queued behind it, and the original exception is rethrown. The statement ref it holds is released when the query object is dropped, as for every other enqueued request;release_query_ref()remains the cleanup for requests that never reached the queue.do_runthat nothing observes, and a few flag reads/writes per query.advance()loop re-reads the queue length each iteration and reaches the nested request in the same pass, or the ReadyForQuery of the request it just wrote triggers the nextadvance();ReadyForQuery/drain_internal/advance_and_flush/do_runall flush after the encoder returns). Flushes skipped inside the window only ever concern bytes the holder flushes itself.test/js/sql/postgres-dispatch-during-bind.test.ts(10 scenarios, each in a subprocess against thepostgres_plainservice: both encode sites, nested new / prepared / same statement, a burst including a simple query, a nested query that dispatches again from its own Bind, a transaction,prepare: false, and a conversion that dispatches then throws, both with the request at the head of the queue and behind an in-flight request). Without the fix 8 of 10 fail (the subprocess panics, or the wedged-connection shapes time out), on both the release binary and a debug build; the two throw scenarios are regression guards for the new failure path. With the fix all 10 pass, locally and on the ASAN CI lane against the real service.test/js/sql/sql.test.ts(postgres block, 853 tests) against a local server on the debug build: no new failures; the four timing-based tests that failed also fail on an unmodified debug build.test/js/sql/postgres-prepared-pipeline-reorder,postgres-split-prepare-reorder,postgres-simple-query-pipeline,postgres-finish-request-underflow,postgres-failed-connection-resurrection,postgres-error-then-datarow,postgres-frame-boundary,sql-prepare-false: pass.Background
advance()is the drain loop that turns queued requests into bytes; it runs from the ReadyForQuery handler and from the socket's writable callback.do_runis the native side ofquery.execute(); for a statement that is already prepared it skips the queue walk and writes the Bind at enqueue time.write_bindreserves the 4 bytes, remembers their offset intowrite_buffer, and patches them in place afterwards (LengthWriter). Flushing the buffer in between both sends a message with a zero length and invalidates the remembered offset.coerce->valueOf,from_js->toString,json_stringify_fast->toJSON, plus any getters on the binding array), so arbitrary user code runs in the middle of encoding. The pool's dispatch path is synchronous, so that code can land back indo_runfor the same connection before the encoder returns.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file