sql: fix pool stall when sql.begin() runs concurrently with pooled queries - #32006
sql: fix pool stall when sql.begin() runs concurrently with pooled queries#32006robobun wants to merge 1 commit into
Conversation
|
Updated 10:02 PM PT - Jul 9th, 2026
❌ @robobun, your commit 0809bdd has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32006That installs a local version of the PR into your bun-32006 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR prevents Postgres pool deadlocks by deferring prepared-statement bind/execute to FIFO advancement, gating advance/flush until no backpressure and no pending prepare, and excluding reserved connections from concurrent flush selection. It adds fixtures and deterministic mock-server integration tests validating the fix and pipelining behavior. ChangesPostgreSQL Pool Concurrency Fix
Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Checked each of the three suggested issues against this branch before adding any Fixes lines:
Leaving the PR scoped to #32004. |
|
CI analysis across the builds on this PR (the diff is green; every failure so far has been unrelated flake or infra):
No SQL-related test has failed on any lane in any of the eight builds; |
08dce70 to
6ec27fb
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/PostgresSQLConnection.rs`:
- Around line 1389-1400: The in-flight close branch calls fail(...) (which via
fail_with_js_value() → ref_and_close() adds an extra poll_ref.ref_()), but only
unrefs once, leaving a dangling ref and potentially pinning the event loop;
update this branch (the block guarded by vm().is_shutting_down() == false &&
status matching Connecting | SentStartupMessage) to use the pre-connected close
path that does not call the ref-adding code or otherwise fully balance refs:
either replace the fail(...) call with a close variant that does not invoke
ref_and_close/ref_ (e.g., a ref_and_close_no_ref or a direct pre-connected close
helper), or, if retaining fail(...), ensure you call poll_ref.with_mut(|r|
r.unref(self.vm_ctx())) one additional time to match the extra ref added by
ref_and_close; reference fail, fail_with_js_value, ref_and_close, and
poll_ref.ref_()/poll_ref.with_mut(|r| r.unref(...)) when making the change.
In `@test/js/sql/postgres-pool-transaction-stall.test.ts`:
- Around line 199-205: The pump(conn: Conn) function can leave conn.busy true if
handleFrame rejects; change the implementation to set conn.busy = true, then run
the frame-processing loop inside a try/finally so conn.busy is always reset to
false, and ensure any errors from handleFrame are not swallowed (remove or
change the surrounding .catch(() => {}) so the rejection is propagated/rewrapped
and causes the test to fail); specifically update pump and the related
data-event handler to rethrow or forward handleFrame errors instead of silencing
them so failures surface.
- Around line 282-311: The test currently sorts the entire milestones list
before comparison (building expect({ steps:
stdout.split(...).filter(...).sort().join("\n"), ...
}).toEqual({...}.sort().join("\n"))), which removes ordering guarantees;
instead, preserve the original sequence from stdout and assert the exact
ordering for all milestones except the one scheduler-dependent pair ("STEP
released" vs "STEP victim resolved"). Change the assertion to use the unsorted
steps (from stdout -> steps) and verify the full sequence matches the expected
milestones in order, but allow the two ambiguous markers by either asserting
that their relative order is one of the two permitted permutations or by
asserting their indices relative to other milestones (e.g., check indexOf("STEP
released") and indexOf("STEP victim resolved") are adjacent and between "STEP
body gate" and "STEP fast resolved"); keep checks for stderr and exitCode
unchanged.
🪄 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: baf3815b-3c26-4af3-b51c-b050f1d488b9
📒 Files selected for processing (6)
src/js/internal/sql/shared.tssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rstest/js/sql/postgres-pool-pipeline-flag-fixture.tstest/js/sql/postgres-pool-transaction-stall-fixture.tstest/js/sql/postgres-pool-transaction-stall.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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/PostgresSQLConnection.rs`:
- Around line 1389-1400: The in-flight close branch calls fail(...) (which via
fail_with_js_value() → ref_and_close() adds an extra poll_ref.ref_()), but only
unrefs once, leaving a dangling ref and potentially pinning the event loop;
update this branch (the block guarded by vm().is_shutting_down() == false &&
status matching Connecting | SentStartupMessage) to use the pre-connected close
path that does not call the ref-adding code or otherwise fully balance refs:
either replace the fail(...) call with a close variant that does not invoke
ref_and_close/ref_ (e.g., a ref_and_close_no_ref or a direct pre-connected close
helper), or, if retaining fail(...), ensure you call poll_ref.with_mut(|r|
r.unref(self.vm_ctx())) one additional time to match the extra ref added by
ref_and_close; reference fail, fail_with_js_value, ref_and_close, and
poll_ref.ref_()/poll_ref.with_mut(|r| r.unref(...)) when making the change.
In `@test/js/sql/postgres-pool-transaction-stall.test.ts`:
- Around line 199-205: The pump(conn: Conn) function can leave conn.busy true if
handleFrame rejects; change the implementation to set conn.busy = true, then run
the frame-processing loop inside a try/finally so conn.busy is always reset to
false, and ensure any errors from handleFrame are not swallowed (remove or
change the surrounding .catch(() => {}) so the rejection is propagated/rewrapped
and causes the test to fail); specifically update pump and the related
data-event handler to rethrow or forward handleFrame errors instead of silencing
them so failures surface.
- Around line 282-311: The test currently sorts the entire milestones list
before comparison (building expect({ steps:
stdout.split(...).filter(...).sort().join("\n"), ...
}).toEqual({...}.sort().join("\n"))), which removes ordering guarantees;
instead, preserve the original sequence from stdout and assert the exact
ordering for all milestones except the one scheduler-dependent pair ("STEP
released" vs "STEP victim resolved"). Change the assertion to use the unsorted
steps (from stdout -> steps) and verify the full sequence matches the expected
milestones in order, but allow the two ambiguous markers by either asserting
that their relative order is one of the two permitted permutations or by
asserting their indices relative to other milestones (e.g., check indexOf("STEP
released") and indexOf("STEP victim resolved") are adjacent and between "STEP
body gate" and "STEP fast resolved"); keep checks for stderr and exitCode
unchanged.
🪄 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: baf3815b-3c26-4af3-b51c-b050f1d488b9
📒 Files selected for processing (6)
src/js/internal/sql/shared.tssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rstest/js/sql/postgres-pool-pipeline-flag-fixture.tstest/js/sql/postgres-pool-transaction-stall-fixture.tstest/js/sql/postgres-pool-transaction-stall.test.ts
🛑 Comments failed to post (3)
src/sql_jsc/postgres/PostgresSQLConnection.rs (1)
1389-1400:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease balancing is incomplete on the in-flight
close()branch.Line 1395 calls
fail(...), which goes throughfail_with_js_value()→ref_and_close()and takes an extrapoll_ref.ref_()(Lines 1527-1532). Line 1399 only unrefs once, and this branch explicitly assumes no socket close callback will run, so the extra keepalive ref is left behind. This can pin the event loop afterclose().Please use a pre-connected close path that does not add the extra
poll_refref, or otherwise fully balance both refs in this no-callback path.As per coding guidelines: “Every error/abort/timeout path actively completes the operation.”
🤖 Prompt for 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. In `@src/sql_jsc/postgres/PostgresSQLConnection.rs` around lines 1389 - 1400, The in-flight close branch calls fail(...) (which via fail_with_js_value() → ref_and_close() adds an extra poll_ref.ref_()), but only unrefs once, leaving a dangling ref and potentially pinning the event loop; update this branch (the block guarded by vm().is_shutting_down() == false && status matching Connecting | SentStartupMessage) to use the pre-connected close path that does not call the ref-adding code or otherwise fully balance refs: either replace the fail(...) call with a close variant that does not invoke ref_and_close/ref_ (e.g., a ref_and_close_no_ref or a direct pre-connected close helper), or, if retaining fail(...), ensure you call poll_ref.with_mut(|r| r.unref(self.vm_ctx())) one additional time to match the extra ref added by ref_and_close; reference fail, fail_with_js_value, ref_and_close, and poll_ref.ref_()/poll_ref.with_mut(|r| r.unref(...)) when making the change.Source: Coding guidelines
test/js/sql/postgres-pool-transaction-stall.test.ts (2)
199-205:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t let mock-server frame-handler failures silently wedge the connection.
If
handleFrame()rejects once,conn.busynever flips back tofalse, and the.catch(() => {})on Line 243 turns that into a silent permanent stall on every laterdataevent for that connection. This makes protocol/parser bugs look like generic test hangs instead of actionable failures.Suggested hardening
async function pump(conn: Conn) { if (conn.busy) return; conn.busy = true; - while (conn.frames.length > 0) { - await handleFrame(conn, conn.frames.shift()!); - } - conn.busy = false; + try { + while (conn.frames.length > 0) { + await handleFrame(conn, conn.frames.shift()!); + } + } finally { + conn.busy = false; + } } ... - pump(conn).catch(() => {}); + pump(conn).catch(err => { + socket.destroy(err instanceof Error ? err : new Error(String(err))); + });As per coding guidelines, tests should “wire EVERY failure event … to reject” and stay hermetic instead of degrading failures into hangs.
Also applies to: 219-243
🤖 Prompt for 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. In `@test/js/sql/postgres-pool-transaction-stall.test.ts` around lines 199 - 205, The pump(conn: Conn) function can leave conn.busy true if handleFrame rejects; change the implementation to set conn.busy = true, then run the frame-processing loop inside a try/finally so conn.busy is always reset to false, and ensure any errors from handleFrame are not swallowed (remove or change the surrounding .catch(() => {}) so the rejection is propagated/rewrapped and causes the test to fail); specifically update pump and the related data-event handler to rethrow or forward handleFrame errors instead of silencing them so failures surface.Source: Coding guidelines
282-311:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t sort away the ordering guarantees this regression is supposed to prove.
The comment says only
"STEP released"vs"STEP victim resolved"is scheduler-dependent, but Lines 285-311 sort the entire milestone list before comparing. That lets unrelated reorderings pass unnoticed, which weakens the regression substantially.Suggested tightening
- expect({ - steps: stdout - .split(/\r?\n/) - .filter(line => line.startsWith("STEP ") || line === "DONE") - .sort() - .join("\n"), + const steps = stdout + .split(/\r?\n/) + .filter(line => line.startsWith("STEP ") || line === "DONE"); + + const expectedA = [ + "STEP prepared", + "STEP armed", + "STEP p0 done", + "STEP body gate", + "STEP released", + "STEP victim resolved", + "STEP fast resolved", + "STEP slow resolved", + "STEP tx resolved", + "STEP pool alive", + "DONE", + ]; + const expectedB = [ + "STEP prepared", + "STEP armed", + "STEP p0 done", + "STEP body gate", + "STEP victim resolved", + "STEP released", + "STEP fast resolved", + "STEP slow resolved", + "STEP tx resolved", + "STEP pool alive", + "DONE", + ]; + + expect({ + steps, stderr: stderr.includes("WATCHDOG") ? "WATCHDOG" : "", exitCode, }).toEqual({ - steps: [ - "STEP prepared", - "STEP armed", - "STEP p0 done", - "STEP body gate", - "STEP released", - "STEP victim resolved", - "STEP fast resolved", - "STEP slow resolved", - "STEP tx resolved", - "STEP pool alive", - "DONE", - ] - .sort() - .join("\n"), + steps: expect.toSatisfy( + actual => JSON.stringify(actual) === JSON.stringify(expectedA) || JSON.stringify(actual) === JSON.stringify(expectedB), + ), stderr: "", exitCode: 0, });As per coding guidelines, “Every assertion must be able to fail, and must assert the strongest invariant.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const steps = stdout .split(/\r?\n/) .filter(line => line.startsWith("STEP ") || line === "DONE"); const expectedA = [ "STEP prepared", "STEP armed", "STEP p0 done", "STEP body gate", "STEP released", "STEP victim resolved", "STEP fast resolved", "STEP slow resolved", "STEP tx resolved", "STEP pool alive", "DONE", ]; const expectedB = [ "STEP prepared", "STEP armed", "STEP p0 done", "STEP body gate", "STEP victim resolved", "STEP released", "STEP fast resolved", "STEP slow resolved", "STEP tx resolved", "STEP pool alive", "DONE", ]; expect({ steps: steps.join("\n"), stderr: stderr.includes("WATCHDOG") ? "WATCHDOG" : "", exitCode, }).toEqual( expect.objectContaining({ steps: expect.stringMatching( new RegExp( `^(${expectedA.join("|").replace(/\./g, "\\.")})$|^(${expectedB.join("|").replace(/\./g, "\\.")})$`, ), ), stderr: "", exitCode: 0, }), );const steps = stdout .split(/\r?\n/) .filter(line => line.startsWith("STEP ") || line === "DONE"); const stepsStr = steps.join("\n"); const expectedA = [ "STEP prepared", "STEP armed", "STEP p0 done", "STEP body gate", "STEP released", "STEP victim resolved", "STEP fast resolved", "STEP slow resolved", "STEP tx resolved", "STEP pool alive", "DONE", ].join("\n"); const expectedB = [ "STEP prepared", "STEP armed", "STEP p0 done", "STEP body gate", "STEP victim resolved", "STEP released", "STEP fast resolved", "STEP slow resolved", "STEP tx resolved", "STEP pool alive", "DONE", ].join("\n"); expect(stepsStr === expectedA || stepsStr === expectedB).toBe(true); expect(stderr.includes("WATCHDOG") ? "WATCHDOG" : "").toBe(""); expect(exitCode).toBe(0);🤖 Prompt for 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. In `@test/js/sql/postgres-pool-transaction-stall.test.ts` around lines 282 - 311, The test currently sorts the entire milestones list before comparison (building expect({ steps: stdout.split(...).filter(...).sort().join("\n"), ... }).toEqual({...}.sort().join("\n"))), which removes ordering guarantees; instead, preserve the original sequence from stdout and assert the exact ordering for all milestones except the one scheduler-dependent pair ("STEP released" vs "STEP victim resolved"). Change the assertion to use the unsorted steps (from stdout -> steps) and verify the full sequence matches the expected milestones in order, but allow the two ambiguous markers by either asserting that their relative order is one of the two permitted permutations or by asserting their indices relative to other milestones (e.g., check indexOf("STEP released") and indexOf("STEP victim resolved") are adjacent and between "STEP body gate" and "STEP fast resolved"); keep checks for stderr and exitCode unchanged.Source: Coding guidelines
|
Re the three CodeRabbit findings (the inline threads failed to post, so answering here):
|
There was a problem hiding this comment.
Both follow-ups from my earlier pass (the can_pipeline() gate in the Running/Binding arm and the auto-flusher batching) are applied in 7e2903c and covered by the new pipeline-flag test; nothing further from me, but this reworks the native queue's write-ordering gates enough that a maintainer should sign off.
Extended reasoning...
Overview
The PR fixes a pool stall in Bun.SQL postgres when sql.begin() runs concurrently with pooled prepared queries. JS side (shared.ts): two small, clearly-correct lines — release() removes the connection from readyConnections before handing it to a reserved waiter, and flushConcurrentQueries() filters reserved as well as preReserved. Rust side: PostgresSQLQuery::do_run() no longer writes Bind+Execute inline for already-prepared statements and instead defers to advance(); advance_and_flush()'s gate widens from IS_READY_FOR_QUERY to !WAITING_TO_PREPARE and now schedules the deferred auto-flusher; the Running/Binding arm in advance() now early-returns on !can_pipeline(). Three new test files add a scripted mock-postgres server and two deterministic regression tests.
Security risks
None identified. No auth, crypto, permissions, or untrusted-input parsing is touched. The change affects client-side write ordering and pool bookkeeping; the worst failure mode is a hang or query running inside the wrong transaction (the bug being fixed), not an exposure.
Level of scrutiny
High. This is concurrency-sensitive wire-ordering logic in a production database driver. The advance_and_flush() gate change is a semantic widening (it now runs while pipelined requests are in flight), and removing the do_run() fast-path shifts every reused-prepared-statement execution onto the advance() state machine. The reasoning in the PR description and code comments is sound and I traced the new ordering invariants (FIFO write order, can_pipeline() subsuming the old WAITING_TO_PREPARE/nonpipelinable checks, auto-flusher restoring same-tick batching), but the interaction surface — simple queries, named vs unnamed prepared statements, the disable-pipelining flag, backpressure — is large enough that a maintainer familiar with this queue should review it rather than auto-approve.
Other factors
Since my earlier inline comments, 7e2903c applied both suggested fixes exactly as proposed and added a dedicated test asserting the feature flag keeps one query in flight (verified can_pipeline() at line 1595 does cover WAITING_TO_PREPARE + nonpipelinable_requests, so the consolidated gate at line 2296 is equivalent-or-stricter). cddacc5 hardened the mock-server assertions per CodeRabbit. No CODEOWNERS entry covers these paths. CI failures on the latest build are infra (musl LTO link, agent provisioning), not test failures. The bug-hunting pass on the current head found nothing.
cddacc5 to
e9dfcbb
Compare
|
Rebased onto main to resolve the conflict with #32464 (postgres: fix do_run error-path leaks). The conflict was in Re-verified on the new base: |
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 `@src/js/internal/sql/shared.ts`:
- Around line 201-207: Add a null/undefined guard for the non-array INSERT
helper path before dereferencing items[column]. After the closing brace of the
if ($isArray(items)) block, add an else clause or additional guard that checks
if items is null or undefined and throws the same SyntaxError with the message
"Cannot use null or undefined as an item in INSERT helper" to ensure consistency
and prevent raw TypeErrors when a single null/undefined item is passed instead
of an array.
🪄 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: 284d8331-89b9-4f2b-8585-32489c71be5c
📒 Files selected for processing (3)
src/js/internal/sql/shared.tssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rs
💤 Files with no reviewable changes (2)
- src/sql_jsc/postgres/PostgresSQLQuery.rs
- src/sql_jsc/postgres/PostgresSQLConnection.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@src/js/internal/sql/shared.ts`:
- Around line 201-207: Add a null/undefined guard for the non-array INSERT
helper path before dereferencing items[column]. After the closing brace of the
if ($isArray(items)) block, add an else clause or additional guard that checks
if items is null or undefined and throws the same SyntaxError with the message
"Cannot use null or undefined as an item in INSERT helper" to ensure consistency
and prevent raw TypeErrors when a single null/undefined item is passed instead
of an array.
🪄 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: 284d8331-89b9-4f2b-8585-32489c71be5c
📒 Files selected for processing (3)
src/js/internal/sql/shared.tssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rs
💤 Files with no reviewable changes (2)
- src/sql_jsc/postgres/PostgresSQLQuery.rs
- src/sql_jsc/postgres/PostgresSQLConnection.rs
🛑 Comments failed to post (1)
src/js/internal/sql/shared.ts (1)
201-207:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the non-array INSERT helper item too.
Line 201adds array-item null validation, but the single-item path still reachesLine 222and dereferencesitems[column]whenitems == null, causing a rawTypeErrorinstead of your intended helperSyntaxError.Suggested fix
function buildDefinedColumnsAndQuery<T>( columns: (keyof T)[], items: T | T[], escapeIdentifier: (name: string) => string, ): { definedColumns: (keyof T)[]; columnsSql: string } { const definedColumns: (keyof T)[] = []; let columnsSql = "("; const columnCount = columns.length; if ($isArray(items)) { for (let j = 0; j < items.length; j++) { if (items[j] == null) { throw new SyntaxError("Cannot use null or undefined as an item in INSERT helper"); } } + } else if (items == null) { + throw new SyntaxError("Cannot use null or undefined as an item in INSERT helper"); }🤖 Prompt for 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. In `@src/js/internal/sql/shared.ts` around lines 201 - 207, Add a null/undefined guard for the non-array INSERT helper path before dereferencing items[column]. After the closing brace of the if ($isArray(items)) block, add an else clause or additional guard that checks if items is null or undefined and throws the same SyntaxError with the message "Cannot use null or undefined as an item in INSERT helper" to ensure consistency and prevent raw TypeErrors when a single null/undefined item is passed instead of an array.
|
Re the CodeRabbit finding on That code is in The observation is fair as a standalone follow-up to #32156 (extend the same SyntaxError to the non-array path), but adding an unrelated helper-validation change to a focused concurrency bugfix would just widen the blast radius of this PR. Leaving it out. |
|
Additional symptom of the same const sql = new Bun.SQL({ url, max: 1 });
await sql.unsafe("SELECT 'warm'");
const busy = sql.unsafe("SELECT 'busy'").execute(); // only connection is now busy
const tx = sql.begin(async t => { // queued on reservedQueue
await t.unsafe("SELECT 1");
await Bun.sleep(200);
throw new Error("app error"); // -> ROLLBACK
});
await Bun.sleep(150);
const intruder = sql.unsafe("INSERT INTO audit_log VALUES ('x')").execute();
await Promise.allSettled([busy, tx, intruder]);The intruder INSERT is dispatched onto the open transaction's connection (it is still in The |
A postgres/mysql pool could hand pooled queries to a connection a sql.begin() transaction owned: release() gave a drained connection to a waiting reserved acquirer without removing it from readyConnections (unlike connect()'s reserved path), and flushConcurrentQueries() only filtered preReserved connections. Pooled queries then executed inside the transaction, and in combination with the native queue's write-ordering bug (fixed separately in #33627) the pool wedged permanently with every connection idle. release() now removes the connection from readyConnections when handing it to a reserved waiter, and flushConcurrentQueries() skips reserved connections. The regression test scripts a mock postgres server that holds one query's response until a control query arrives, deterministically forcing the interleaving; without the fix, pooled queries are written to the transaction's connection between BEGIN and COMMIT. A second test covers BUN_FEATURE_FLAG_DISABLE_SQL_AUTO_PIPELINING keeping one query in flight per connection. Fixes #32004
e9dfcbb to
0809bdd
Compare
|
Rebased onto main again; this one changed the PR's scope, so summarizing here (the description is updated to match). Main landed #33627, which fixes the native write-ordering half of this bug its own way: the enqueue-time Bind+Execute fast path is kept but gated on Re-verified on the new base: with For reviewers who saw earlier revisions: claude[bot]'s sign-off concerns about the native queue rework no longer apply, since that rework is gone. The remaining diff is the two-hunk pool fix both bots already reviewed without findings. |
There was a problem hiding this comment.
No issues found — the two shared.ts hunks are small and mirror the existing reserved path in connect() — but this changes SQL pool distribution in a way that affects transaction isolation for both postgres and mysql, so it's worth a human sign-off.
Extended reasoning...
Overview
The current diff is much narrower than earlier revisions: the native Rust changes to PostgresSQLConnection.rs/PostgresSQLQuery.rs were dropped after #33627 landed the ordering fix on main, leaving only two small hunks in src/js/internal/sql/shared.ts (the pool shared by postgres and mysql) plus ~530 lines of new test/fixture code with a scripted mock postgres server.
The source change: (1) flushConcurrentQueries() now filters out reserved connections in addition to preReserved, and (2) release() calls this.readyConnections.delete(connection) before handing a drained connection to a waiting reservedQueue entry — the exact same bookkeeping connect()'s direct reserved path already does. Both are clearly consistent with the existing invariant that reserved connections must not be visible to the concurrent query distributor.
Security risks
None in the traditional sense (no auth/crypto/untrusted input), but the bug being fixed has data-integrity implications: pooled queries could execute inside a transaction they don't belong to and be silently rolled back. That raises the stakes on getting the fix right rather than lowering them.
Level of scrutiny
Medium-high. The change is mechanically simple and obviously mirrors an established pattern, but it's in the core connection-pool distribution path shared across all SQL adapters, the failure mode is a concurrency/ordering bug (historically hard to reason about from a diff alone), and the PR's scope has shifted substantially since it was opened (Rust half dropped, one test now covers behavior owned by #33627 rather than this PR). A maintainer familiar with the pool state machine should confirm the final scope and the mock-server test approach.
Other factors
All prior review threads (my two inline comments on the since-dropped Rust changes, and the CodeRabbit findings) are resolved. The bug-hunting system found nothing on the current head. CI is reported green modulo tracked flakes. No CODEOWNERS entry covers this path.
|
Acknowledged, nothing to address from that pass. Final state for whoever picks this up: the diff is the two-hunk pool fix in |
…potent (#34756) ### What `PostgresSQLConnection::finish_request` decremented `nonpipelinable_requests` / `pipelined_requests` based on the request's `simple` / `pipelined` flags at fire time, from three call sites: the `ReadyForQuery` handler, the `ErrorResponse` handler, and connection-close cleanup. None of them record that a given request's contribution has been consumed, so under connection-failure timing the same request can be finished twice and the `u32` counter goes past zero. Debug builds panic: ``` panic: attempt to subtract with overflow PostgresSQLConnection::finish_request PostgresSQLConnection.rs:1812 (nonpipelinable_requests.get() - 1) PostgresSQLConnection::on::<StackReader> PostgresSQLConnection.rs:2535 (ReadyForQuery 'Z' arm) postgres_request::on_data PostgresSQLConnection::on_data uws on_data / us_internal_dispatch_ready_poll ``` Release builds silently wrap the counter to `u32::MAX`, after which `advance()`'s `nonpipelinable_requests.get() > 0` guard returns early forever and that pool connection stops dispatching queued queries. This is one of the mechanisms behind #32004's permanent pool stall (the other is the JS-side reserved-connection handoff, tracked in #32006). ### Fix Add a per-request `counted` bit to the query flags. Each of the five increment sites (two in the enqueue fast paths in `PostgresSQLQuery.rs`, three in `advance()` in `PostgresSQLConnection.rs`) sets it alongside the increment. `finish_request` only decrements when the bit is set and clears it afterwards, so a second call is a no-op. The decrement also gains a `debug_assert!(n > 0)` and uses `saturating_sub` so a future call site that forgets to set the bit trips the assert in debug and degrades to a counter leak (not a wrap) in release. ### Reproduction and fail-before The underflow was caught by syscall fault injection on the client's Postgres socket against a real loopback server: the panic above reproduces at roughly 3% of 400-iteration runs under injection, always with a `PostgresError: Connection closed` rejection printed in the same tick immediately before the panic. I could not reduce it to a zero-fault reproduction. The known server-driven path (ErrorResponse, then a late CommandComplete flipping the failed request back to PartialResponse so ReadyForQuery decrements again) was closed by #33989's `status == Fail` skip in the result-message handlers, and the `on_data` dispatch loop bails once `fail_with_js_value` has run, which rules out the straightforward re-entrancy windows. <details><summary>multi-iteration probe on current main (debug+ASAN, no fault injection)</summary> 30-round #32004 workload (`sql.begin()` interleaved with pooled parameterized queries, `max: 4`) against a local Postgres 17: ``` run 1: WATCHDOG wedge run 2: WATCHDOG wedge run 3: WATCHDOG wedge run 4: WATCHDOG wedge run 5: WATCHDOG wedge ``` The wedge persists with this fix applied (it is the #32006 JS-side pool bug). No `attempt to subtract with overflow` panic in any of 5 runs, with or without this fix, so the remaining trigger is fault-gated. The same workload on a debug build that predates #33989 panics 3/3 at `finish_request`, which is what #33989's commit message documented; the server-driven path is what that PR closed, the fault-timed one is what this PR closes. </details> A directed hostile-server probe (CommandComplete before the first ReadyForQuery, to push a never-dispatched Pending query through `on_result(false)` into PartialResponse) does not reach the decrement either: the JS adapter parks queries until after startup and the resulting `ERR_POSTGRES_EXPECTED_REQUEST` fails the connection before `finish_request` runs. The decrement is on an internal per-request counter, not a wire field, so there is no single server byte sequence that reaches it twice. Given the above, the race is not fail-before-provable without instrumenting `src/` (the gate strips `src/` for fail-before). `test/js/sql/postgres-finish-request-underflow.test.ts` is therefore a regression guard rather than a fail-before test: it drives a single connection through the `Z`, `E`, and `E`-then-late-`C`+`Z` paths back-to-back against a scripted mock server and asserts a follow-up query still dispatches afterwards. A leaked-high or wrapped counter would stall that follow-up query until the fixture's watchdog fires, and a violated invariant trips the new `debug_assert`. ### Verification - `bun bd test test/js/sql/postgres-finish-request-underflow.test.ts`: passes - Existing `test/js/sql/` Postgres suites (`postgres-error-then-datarow`, `postgres-simple-query-pipeline`, `postgres-prepared-pipeline-reorder`, `postgres-split-prepare-reorder`, `postgres-multi-statement-fields`, `postgres-failed-connection-resurrection`, `postgres-datarow-overrun`, `wire-frames`) still pass with the fix. - `cargo clippy -p bun_sql_jsc`: clean
|
The reported hang in #32004 no longer reproduces on main after #34756 (idempotent If this PR's JS-side pool-handoff change addresses a case those two PRs don't cover, a separate repro would help; otherwise it may no longer be needed. |
1 participant
Fixes #32004
Repro
From the issue: a pool (
max: 4) runningsql.begin()transactions concurrently with plain parameterizedsql.unsafe()queries intermittently stalls forever.pg_stat_activityshows every connection idle atClientRead(one leftidle in transaction, its COMMIT never sent), no lock waits, while severalsql.unsafe()calls wait for a connection that never comes back. Reproduced on the released build: 4 hangs in 15 runs of the issue's script; 0 in 30 runs with the fix.Cause
Two cooperating bugs. The second has since been fixed on main by #33627, so this PR now carries only the first:
Pool handoff leaves the connection visible to the distributor (this PR).
connect(reserved=true)'s direct path removes the connection fromreadyConnections, but therelease()path that hands a drained connection to a waitingsql.begin()does not, andflushConcurrentQueries()only filteredpreReserved(notreserved) connections. Pooled queries therefore kept getting distributed onto a connection a transaction owned, and executed inside that transaction.Native queue could write out of order (fixed by sql(postgres): gate enqueue-time Bind+Execute on queued requests being written #33627). The enqueue-time pipelining fast path wrote a prepared query's Bind+Execute while an earlier queued request was still unwritten; responses are matched FIFO, so the unwritten request (the transaction's COMMIT) stole the pipelined query's result, the transaction "committed" without COMMIT reaching the server (the
idle in transactionconnection),nonpipelinable_requestsunderflowed, and the connection wedged. Main now gates that fast path onpending_requests == 0; earlier revisions of this PR fixed it by removing the fast path, and that half was dropped in the rebase in favor of main's version.Fix
src/js/internal/sql/shared.ts(the pool shared by the postgres and mysql adapters):release()removes the connection fromreadyConnectionswhen handing it to a reserved waiter (mirroringconnect()), andflushConcurrentQueries()skipsreservedconnections.Verification
test/js/sql/postgres-pool-transaction-stall.test.tshas two tests against a scripted mock postgres server (no Docker/services). The first forces the issue's interleaving deterministically: a transaction acquires its connection through therelease()handoff while pooled prepared queries are in flight. Without this fix it fails its wire-order assertion (pooled queries are written to the transaction's connection between BEGIN and COMMIT; before #33627 landed, the same fixture wedged the pool outright). With the fix, no pooled query touches the transaction's connection and everything completes. The second test assertsBUN_FEATURE_FLAG_DISABLE_SQL_AUTO_PIPELININGkeeps at most one query in flight per connection, coverage main's #33627 tests do not include.Also ran the issue's original repro in a loop against real Postgres 17: 30/30 clean with the fix (debug build), 4/15 hangs without it (released build at the time of the original investigation).
MySQL's native queue has a narrower variant of the ordering window; tracked in #32005. The pool fix here applies to the MySQL adapter too and removes the transaction-driven trigger there.
[review] gate passed · iteration 11 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 11
evidence per changed file