sql(postgres): ref poll_ref only after request is enqueued in do_run - #32273
sql(postgres): ref poll_ref only after request is enqueued in do_run#32273robobun wants to merge 3 commits into
Conversation
PostgresSQLQuery::do_run refed the connection's poll_ref KeepAlive before any validation. KeepAlive is a two-state flag, not a counter, so when the query was the only in-flight work the call flipped Inactive -> Active. Every synchronous error return after that point (bad binding, Signature::generate failure, statements.get_or_put OOM, cached-statement failure, bind_and_execute / write_query failure, requests.write_item OOM, ...) undid the speculative self-ref but never the poll_ref, leaving the event loop pinned and the process hung until the connection closed. Move the poll_ref.ref_() to after the request has been written into connection.requests on both the simple-query and prepared-statement success paths. On every error path the keepalive is now untouched. The regression test connects to a mock server, lets the connection go idle (poll_ref Inactive), then issues a query with a boxed Boolean binding that is rejected synchronously by the Postgres type mapper in Signature::generate. The fixture must print the rejection and exit on its own; without the fix it hangs and is killed by the test timeout.
|
Updated 2:17 AM PT - Jun 15th, 2026
✅ @robobun, your commit 1718b23818e506f7609b2909736b6f13216896d7 passed in 🧪 To try this PR locally: bunx bun-pr 32273That installs a local version of the PR into your bun-32273 --bun |
Walkthrough
Changespoll_ref KeepAlive Fix and Regression Test
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/sql/sql-postgres-run-error-pollref.test.ts`:
- Line 27: In the expect() assertion on line 27, remove the hard-coded empty
stderr check that makes the test flaky. Instead of expecting both stdout and
stderr to match an exact object, only verify the stdout output contains the
expected error message "rejected:ERR_INVALID_ARG_TYPE\n" and remove the stderr
assertion entirely, allowing benign debug/sanitizer noise in stderr without
failing the test.
🪄 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: d27eb6f0-3c29-4a71-9a3a-c6f010658c25
📒 Files selected for processing (3)
src/sql_jsc/postgres/PostgresSQLQuery.rstest/js/sql/sql-postgres-run-error-pollref-fixture.tstest/js/sql/sql-postgres-run-error-pollref.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…rror arms Three pre-existing error returns in the prepared-statement branch of do_run skipped the Self::deref(this_ptr) cleanup that every sibling arm performs: statements.get_or_put OOM, writer.write(SYNC) failure, and the trailing requests.write_item OOM (which also left the freshly allocated this.statement pinned). These are allocator-OOM paths so the practical impact is small, but they are the same cleanup class as the poll_ref leak and sit directly adjacent to it.
There was a problem hiding this comment.
Thanks — 1718b23 addresses all three error arms I flagged. The change looks correct to me, but since it reorders keepalive/refcount lifecycle across a dozen unsafe error paths in do_run, it's worth a quick look from someone who owns this driver.
Extended reasoning...
Overview
This PR fixes an event-loop hang in PostgresSQLQuery::do_run: the connection's poll_ref KeepAlive (a two-state flag, not a counter) was being ref'd before any argument validation, so every synchronous error return left it stuck Active and pinned the process. The fix moves the poll_ref.ref_() call to after connection.requests.write_item() succeeds on both the simple-query and prepared-statement branches. While auditing, three error arms (statements.get_or_put OOM, writer.write(SYNC) failure, prepared-branch requests.write_item OOM) were also found to be missing the Self::deref(this_ptr) that every sibling arm performs to undo the speculative this.ref_(); these are swept up too. A new mock-server subprocess test asserts the process exits cleanly after a synchronous do_run failure.
Security risks
None. This is internal refcount/keepalive bookkeeping in the Postgres driver; no auth, input parsing, or privilege boundaries are touched.
Level of scrutiny
Medium-high. The change is conceptually simple (defer the ref until enqueue succeeds; release what you took on error), and the on_data epilogue at PostgresSQLConnection.rs:1048-1055 plus update_ref() confirm the PR description's analysis of how the leaked ref escapes correction. The three new Self::deref(this_ptr) additions are mechanical copies of the pattern on every adjacent error arm. But do_run is ~400 lines of manual intrusive refcounting with a dozen early returns, several unsafe blocks, and a raw *mut stashed in a long-lived FIFO — the kind of function where a misplaced cleanup is a UAF rather than a leak. The newly added release_statement() on the prepared-branch write_item OOM, for example, drops the query's ref on a statement that (in the named branch) is also held by connection.statements, which is correct but worth a second pair of eyes.
Other factors
My earlier inline comment about the three missing Self::deref calls was addressed exactly as suggested in 1718b23, and the coderabbit stderr-assertion nit was fixed in cfbbe89. The new regression test is well-constructed (mock server, explicit setImmediate to leave on_data's drain, await using for subprocess cleanup, exit-code asserted last). CI shows two musl build failures that look like unrelated LTO/infra flakes ("Linking two modules of different data layouts"), not code issues. coderabbit suggested cirospaciari as a domain reviewer, which seems right for this area.
…t refs) (#32464) Supersedes #32426 and #32273 — combines both, plus the `release_statement()` at the SYNC-failure site that #32273 missed. ## Repro (the user-visible part) ```js import { SQL } from "bun"; const sql = new SQL({ url: "postgres://...", max: 1, idleTimeout: 0, maxLifetime: 0 }); await sql.connect(); await new Promise(r => setImmediate(r)); await sql`SELECT ${new Boolean(true)}`.catch(e => e); // ERR_INVALID_ARG_TYPE // process hangs here instead of exiting ``` ## Cause `PostgresSQLQuery::do_run` did two pieces of speculative setup before validating its arguments: 1. `connection.poll_ref.ref_()`. `KeepAlive` is a two-state flag (`src/io/keep_alive.rs`), not a refcount, so when this query is the only in-flight work the call flips Inactive → Active. Every synchronous error return after that point left it stuck Active; nothing else on an idle connection touches `poll_ref` until the next server message, which never comes because nothing was written. The hang is masked when `do_run` runs inside the connection's `on_data` microtask drain (whose epilogue re-derives `poll_ref` from the queue), so it only shows up for queries issued on a later turn — the normal case for any query after the first on a pooled connection. 2. `this.ref_()`. The simple-query `execute_query` failure path correctly released it, but three other error exits did not: `statements.get_or_put` failure, `writer.write(SYNC)` failure, and the final `requests.write_item` failure. The latter two also leaked the just-allocated statement ref. ## Fix - Move `poll_ref.ref_()` to after `connection.requests.write_item(this_ptr)` succeeds, on both the simple-query and prepared-statement branches. On every error path the keepalive is now untouched; on the success path the request is enqueued so the ref is balanced by `on_data`/`update_ref()` when the server responds. Matches `JSMySQLQuery::do_run`, which never pre-refs. - Extract the per-exit cleanup into two closures (`release_query_ref`, `throw_write_error`) and apply them at all eight error-return sites — the three previously-leaking sites now release what they took, and the five existing sites lose their copy-pasted blocks. Net −74 in `PostgresSQLQuery.rs`. ## Verification Regression test added to `sql-onconnect-onclose-throw.test.ts`'s `describeWithContainer("postgres", ...)` block: a fixture connects to real Postgres, waits a tick so `do_run` runs outside the `on_data` drain, issues the boxed-Boolean query, prints the rejection and falls through. Without the fix the child hangs and the test times out; with the fix it prints `rejected:ERR_INVALID_ARG_TYPE` and exits 0. `cargo check -p bun_sql_jsc` and `cargo clippy --no-deps` clean. `postgres-multi-statement-fields.test.ts` and `sql-connect-error-reporting.test.ts` (20 tests, both query-protocol paths) still pass.
2 participants
Repro
new Boolean(true)is rejected by the Postgres binding type mapper insideSignature::generate, soPostgresSQLQuery::do_runreturns an error before the request is ever enqueued.Cause
do_runrefed the connection'spoll_refKeepAliveup front, before validating its arguments:KeepAliveis a two-state flag (Active/Inactive), not a reference count (src/io/keep_alive.rs). When this query is the only in-flight work, the call flipsInactive -> Active, and every synchronous error return after that point (non-object target,Signature::generatefailure,statements.get_or_putOOM, cached statement in Failed state,bind_and_execute/prepare_and_query_with_signature/write_queryfailure,requests.write_itemOOM) leaves thepoll_refActive. Nothing else on an idle connection touchespoll_refuntil the next server message, which never comes because nothing was written, so the event loop stays pinned and the process never exits.The hang is masked when
do_runruns inside the connection'son_datamicrotask drain (the usual first-query path):on_data's epilogue re-derivespoll_reffrom the request queue afterwards. It shows up whenever the failing query is issued on a later turn, which is the normal case for any query after the first on a pooled connection.Fix
Move the
poll_ref.ref_()to afterconnection.requests.write_item(this_ptr)succeeds, on both the simple-query and prepared-statement branches. On every error path the keepalive is now untouched; on the success path the request is enqueued so the ref is balanced byon_data/update_ref()when the server responds. This matchesJSMySQLQuery::do_run, which never pre-refs.While auditing the error arms, three of them (
statements.get_or_putOOM,writer.write(SYNC)failure, and the prepared-branchrequests.write_itemOOM) were also missing theSelf::deref(this_ptr)that every sibling arm performs to undo the speculativethis.ref_()taken near the top of the function; the last of these also left the freshly allocatedthis.statementpinned. Those are swept up here as well so every error return now releases what it took.Verification
test/js/sql/sql-postgres-run-error-pollref.test.tsspawns a fixture that connects to a mock Postgres server (AuthenticationOk + ReadyForQuery), waits a tick, issues the boxed-Boolean query, prints the rejection and falls through. Without the fix the child hangs and is killed by the 5s test timeout; with the fix it printsrejected:ERR_INVALID_ARG_TYPEand exits 0.Also ran locally with the fix:
postgres-multi-statement-fields.test.ts,sql-close-pending-connection.test.ts,sql-connect-error-reporting.test.ts,sql-prepare-false.test.ts(33 tests covering both simple and prepared query paths against mock servers), all pass.