fix(sql): send Parse+Bind+Execute atomically for unnamed prepared statements - #27952
Conversation
…tements When `prepare: false` is used (unnamed prepared statements), the driver previously split the protocol flow into two phases: 1. Parse+Describe+Sync → wait for ParameterDescription 2. Bind+Execute+Sync The Sync in phase 1 causes PostgreSQL to send ReadyForQuery, which signals to PgBouncer (in transaction mode) that the transaction is complete. PgBouncer then releases the server connection. When phase 2 arrives, it may be routed to a different backend where the unnamed prepared statement either doesn't exist or was set to a different query by another client — causing queries to silently return wrong data or modify wrong rows. Additionally, for subsequent executions of the same query, the driver only sent Bind+Execute (skipping Parse entirely), assuming the unnamed statement still existed. With PgBouncer connection multiplexing, this assumption is incorrect. Fix: For unnamed prepared statements, always send all protocol messages (Parse + [Describe] + Bind + Execute + Flush + Sync) as a single atomic batch. On first execution, Describe is included to learn parameter types for caching. On subsequent executions, Parse is always included (without Describe) to ensure the statement exists on the current backend. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Updated 6:16 PM PT - Mar 13th, 2026
❌ Your commit 🧪 To try this PR locally: bunx bun-pr 27952That installs a local version of the PR into your bun-27952 --bun |
WalkthroughAdded a public Parse+Bind+Execute atomic writer ( Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this changes Postgres wire protocol message sequencing for unnamed prepared statements — a data-correctness-critical code path that warrants human review, especially since it was AI-generated.
Extended reasoning...
Overview
This PR modifies the PostgreSQL extended query protocol handling in Bun's SQL driver. It adds a new parseAndBindAndExecute function in PostgresRequest.zig that sends Parse+[Describe]+Bind+Execute+Flush+Sync as a single atomic batch, and modifies two code paths in PostgresSQLConnection.zig's advance() method to use this new function when use_unnamed_prepared_statements is true. A new test file with 8 tests covers prepare: false scenarios.
Security risks
No direct security risks. The change is about wire protocol message ordering, not authentication or authorization. The fix actually improves correctness by preventing queries from silently executing against the wrong prepared statement when using PgBouncer.
Level of scrutiny
This deserves careful human review. The change modifies how PostgreSQL wire protocol messages are sequenced — getting this wrong can cause silent data corruption (queries returning results from the wrong prepared statement). The PR is AI-generated (labeled claude) and touches protocol-level logic in two distinct code paths within advance(). While the implementation follows established patterns and the logic appears sound, the interaction between Parse/Describe/Bind/Execute message ordering, connection state flags (waiting_to_prepare, is_ready_for_query, pipelined_requests), and PgBouncer's connection multiplexing behavior is subtle enough to warrant expert review.
Other factors
- The PR includes good test coverage for
prepare: falsescenarios, but the tests run against a direct Postgres connection, not PgBouncer — the actual failure scenario. The PR description acknowledges this limitation. - There is notable code duplication in the error handling branches (the unnamed vs named paths in the
preparedstate case duplicate ~20 lines of error handling). This is consistent with the existing codebase style but a human reviewer may want to consider factoring it. - The
canPipeline()method already returnsfalsefor unnamed prepared statements, which is consistent with the new behavior. - Named prepared statements (the default
prepare: truepath) are completely unaffected by this change.
There was a problem hiding this comment.
This PR modifies the PostgreSQL wire protocol handling in the connection driver to fix a real PgBouncer incompatibility issue. The logic looks sound, but given it touches protocol-level database code where correctness directly impacts data integrity, a human should verify the approach and the interaction with the enqueue path.
Extended reasoning...
Overview
This PR adds a new parseAndBindAndExecute() function to PostgresRequest.zig that atomically sends Parse+Describe+Bind+Execute+Flush+Sync as a single message batch. PostgresSQLConnection.zig is updated with two new conditional branches in advance(): one for .prepared statements (re-executing) and one for .pending statements (first execution) when use_unnamed_prepared_statements is enabled. A new test file covers the prepare: false mode.
Security risks
The PR addresses a data integrity issue where PgBouncer could route Bind+Execute to a different backend than Parse, causing queries to execute against wrong prepared statements. The fix itself does not introduce new security concerns — it sends the same protocol messages, just batched together. No injection vectors or auth changes.
Level of scrutiny
This warrants careful human review. The PostgreSQL wire protocol handling is production-critical code that directly affects query correctness. The PR modifies state machine transitions (req.status, statement.status, flags.waiting_to_prepare, pipelined_requests) which need to be verified against all possible message orderings from the server. The error handling blocks are duplicated between the unnamed and named paths, which increases maintenance risk.
Other factors
The enqueue path in PostgresSQLQuery.zig (lines 423-451) was not updated by this PR, leading to a minor performance issue (double Parse + extra round-trip for parameterized unnamed queries on idle connections) flagged as a nit. The new test suite covers basic scenarios but tests against a plain PostgreSQL container rather than PgBouncer, so the core fix scenario (connection multiplexing) is not directly validated. The PR description acknowledges this gap. CodeRabbit found no actionable issues.
cirospaciari
left a comment
cirospaciari
left a comment
There was a problem hiding this comment.
the code looks good, lets see if CI would be green I believe it should be green, but we should fix the double parsing issue first before merging
|
@robobun adopt |
|
Looking at the double-Parse issue flagged by @cirospaciari. Will fix the enqueue path in |
…ments When a parameterized unnamed query was enqueued on an idle connection, the enqueue path in PostgresSQLQuery.zig sent Parse+Describe+Sync before advance() ran. This caused double-Parse (once from enqueue, once from advance's atomic parseAndBindAndExecute) and was still vulnerable to PgBouncer splitting the non-atomic enqueue Parse from the subsequent Bind+Execute. Fix: skip writeQuery+Sync in the enqueue path when use_unnamed_prepared_statements is true. Set statement status based on did_write (not can_execute) so the statement stays .pending for advance() to pick up. Call advance()+flushData() directly when the connection is idle to process the pending request immediately. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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 the current code and only fix it if needed.
Inline comments:
In `@src/sql/postgres/PostgresSQLConnection.zig`:
- Around line 1093-1101: The advanceAndFlush function currently only checks
this.flags.is_ready_for_query before calling advance() and flushData(); update
its guard to also ensure there is no backpressure (i.e., check
!this.flags.has_backpressure) so it mirrors drainInternal's behavior and avoids
the unnecessary call into advance() when backpressure is present; keep the rest
of the logic (calling this.advance() then this.flushData()) intact and reference
advanceAndFlush, drainInternal, has_backpressure, advance, flushData, and
flags.is_ready_for_query when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95c23205-151e-48a4-887c-b75607298846
📥 Commits
Reviewing files that changed from the base of the PR and between 616b1d9 and d73e80a7f7daefd5e7ef81046aa45149414dfa02.
📒 Files selected for processing (2)
src/sql/postgres/PostgresSQLConnection.zigsrc/sql/postgres/PostgresSQLQuery.zig
Address review feedback: keep advance() private and expose a public advanceAndFlush() helper that checks is_ready_for_query before calling advance()+flushData(). Use this from the enqueue path instead of directly calling advance(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d73e80a to
621ab3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sql/postgres/PostgresSQLConnection.zig`:
- Around line 1196-1240: The unnamed prepared-statement branch guarded by
flags.use_unnamed_prepared_statements appears unreachable because prepare: false
never inserts into connection.statements and the state machine moves requests
from .pending directly to .binding, so req.status == .pending &&
statement.status == .prepared won't occur; either remove the unnamed branch (the
block using PostgresRequest.parseAndBindAndExecute) or justify it by adding a
focused unit/integration test that demonstrates the reuse path (and document why
connection.statements can contain unnamed statements), and if you keep it ensure
the behavior is covered by tests referencing
flags.use_unnamed_prepared_statements, PostgresRequest.parseAndBindAndExecute,
PostgresRequest.bindAndExecute, connection.statements, req.status and
statement.status so the state transition is proven.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f68816b-9b7e-439d-8981-f70820d0932e
📥 Commits
Reviewing files that changed from the base of the PR and between d73e80a7f7daefd5e7ef81046aa45149414dfa02 and 621ab3d.
📒 Files selected for processing (2)
src/sql/postgres/PostgresSQLConnection.zigsrc/sql/postgres/PostgresSQLQuery.zig
Verification ReportHEAD: Gate 1: CI ✅
Gate 2: ClassificationBug fix — PgBouncer incompatibility with Gate 3: Test proof
Gate 4: Diff ✅
Gate 5: Bot convergence ✅
Gate 6: Hygiene ✅
StatusWaiting on @cirospaciari to re-review. The CHANGES_REQUESTED from commits |
Verification summary —
|
| Gate | Result |
|---|---|
| CI | ✅ All 30 build+test checks pass. darwin-14-aarch64-test-bun failure is a pre-existing flake (also fails on unrelated PRs #28076, #28072). upload-benchmark is infra. |
| Test proof | ✅ New test file test/js/sql/sql-prepare-false.test.ts with 8 tests. Verified: fails on baked binary, passes on PR binary. |
| Diff | ✅ No TODOs/FIXMEs. Fix matches root cause (atomic Parse+Bind+Execute for unnamed stmts). |
| Bot convergence | ✅ All 6 review threads resolved. |
| Hygiene | ✅ |
Status: All gates pass. Waiting on @cirospaciari to re-review — all three review comments were addressed in 621ab3d (advanceAndFlush instead of making advance public, deduped Parse in enqueue path).
|
@cirospaciari All review feedback has been addressed — |
Verification ReportHead: Gate 1: CI — ✅ PASS
Gate 2: Classification — Bug fixPgBouncer incompatibility with Gate 3: Test proof — ⏭️ SKIPNew test file Gate 4: Diff — ✅ PASSNo TODO/FIXME/HACK in added lines. Fix atomizes Gate 5: Bot convergence — ✅ PASS
Gate 6: Hygiene — ✅ PASSBranch Recommendation: Ready for human review. |
Verification SummaryHead: CI (Gate 1) — PASS60/62 Buildkite checks green. Two failures are pre-existing flakes:
Test Proof (Gate 3) — PASS
Review Threads (Gate 5) — PASSAll 6 review threads resolved, including @cirospaciari's feedback:
Diff (Gate 4) — PASSNo TODO/FIXME. Fix matches the root cause described in the PR body. @cirospaciari — all your requested changes have been addressed in the latest commits. Ready for re-review when you have a moment. |
|
Friendly ping @cirospaciari — all your feedback has been addressed (advance() is private again with advanceAndFlush() helper, enqueue path skips writeQuery+Sync for unnamed stmts). All review threads are resolved. Could you take another look when you get a chance? 🙏 |
|
All review threads are resolved and CI is green (2 failures are pre-existing: |
Verification ReportGate 1: CI ✅Build #39496: 58/61 Buildkite checks passed. 3 failures:
GitHub Actions: Lint ✅, Format ✅, Mintlify ⏭️ (skipped) Gate 2: ClassificationBug fix — Gate 3: Test proof
|
Verification ReportHead: Gate 1: CI ✅58/61 checks pass. Three failures are pre-existing flakes — identical failures appear on recently merged PRs #28085, #28084, #28082:
Gate 2: ClassificationBug fix — Gate 3: Test proof
|
|
I downloaded the In the screenshot below, you can see whereas the current Bun 1.3.10 release read data from the wrong table 495 of 1000 times, the Bun build from this PR did not read data from the wrong table: 
Two other tests cases relating to inserting and deleting data from the wrong table are also passing with this new build. Happy to grant access to my reproduction repo (or just make it public) if useful; just let me know. Thank you for fixing! |
|
The |
… on the darwin agent where docker hangs (#33986) ## What Fixes `test/js/sql/sql-prepare-false.test.ts` going red on darwin x64 CI with `timeout` (and occasionally `crash reported`) since the `darwin-x64-mini-1` agent joined the `test-darwin` queue around build 70660. Seen in builds 70820, 70844, 70855, 70866, 70909, 70964, 70976, 71002, 71019, 71040, 71172, 71187, 71234, 71293, 71443, 71453, 71495, 71701, 71806, 71828, 71934 (always on `darwin-x64-mini-1-1`, passes on every other darwin agent). ## Cause `sql-prepare-false.test.ts` was the only file in `test/js/sql/` that called `dockerCompose.ensure("postgres_plain")` directly from an async `describe` instead of going through the `isDockerEnabled()` / `describeWithContainer` guard that every other container-backed sql test uses. On `darwin-x64-mini-1` the docker client is on PATH but talking to it blocks: the valkey tests' `Bun.spawnSync([docker, "info"], {timeout: 5_000})` hits its 5s timeout on the same agent, while `isDockerEnabled()` (which goes through node `execSync`) returns `false` in under 100ms. So `ensure()`'s `Bun.spawn(["docker", "version"])` / `compose up` path hung until the runner's 180s per-file timeout killed the process with nothing printed after the `bun test` banner. From [build 71293's darwin-x64-mini-1 shard](https://buildkite.com/bun/bun/builds/71293#019f4a02-883c-4d23-aee1-565728de91a4): ``` --- [927/1066] test/js/sql/sql-prepare-false.test.ts bun test v1.4.0-canary.1 (0809bdd) --- [927/1066] test/js/sql/sql-prepare-false.test.ts - timeout ... 4 attempts, each exactly 180s ... ``` while on the same shard every neighboring `describeWithContainer`-guarded file skips in under 100ms: ``` --- [913/1066] test/js/sql/postgres-binary-numeric.test.ts Ran 0 tests across 1 file. [68.00ms] --- [924/1066] test/js/sql/sql-mysql.test.ts Ran 0 tests across 1 file. [75.00ms] ``` In [build 71934](https://buildkite.com/bun/bun/builds/71934) the non-zero exit also tripped the runner's crash-report drain, so stale intentional crashes from `run-crash-handler.test.ts` (`crashByPanic`, `0xDEADBEEF`, `outOfMemory`) and a bundler `native-plugin` crash were attributed to this file and it surfaced as `crash reported` instead of `timeout`. That is the known limitation commented at [`scripts/runner.node.mjs:1453`](https://github.com/oven-sh/bun/blob/main/scripts/runner.node.mjs#L1453). ## Fix Move the file onto `describeWithContainer("...", { image: "postgres_plain" }, container => ...)`, the same pattern used by `postgres-binary-numeric.test.ts`, `postgres-prepared-pipeline-reorder.test.ts`, `postgres-simple-query-pipeline.test.ts` and the other recent postgres tests. That helper already short-circuits on `!isDockerEnabled()` with a `describe.todo`, which is what every other sql test on that agent does today. Also drops the `afterAll(dockerCompose.down())` which tore down the whole compose project (all services) for later files on the same shard; `describeWithContainer` intentionally leaves the shared containers up. ## Verification All 8 cases still pass against a real postgres: ``` $ bun bd test test/js/sql/sql-prepare-false.test.ts Container ready via docker-compose: postgres_plain at 127.0.0.1:5432 (pass) PostgreSQL prepare: false > basic parameterized query (pass) PostgreSQL prepare: false > multiple parameterized queries sequentially (pass) PostgreSQL prepare: false > same query repeated with different params (pass) PostgreSQL prepare: false > concurrent queries with different tables return correct results (pass) PostgreSQL prepare: false > parameterized query with multiple params (pass) PostgreSQL prepare: false > query without params still works (pass) PostgreSQL prepare: false > transactions with parameterized queries (pass) PostgreSQL prepare: false > concurrent parameterized queries with high concurrency 8 pass 0 fail ``` #31671 also touches this file for a different reason (making `isDockerEnabled()` throw on macOS CI when docker is absent); that change stacks cleanly on top of this one since `describeWithContainer` already routes through `isDockerEnabled()`. The test was added in #27952; the hang was exposed when `darwin-x64-mini-1` joined the fleet. <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 0 · 1 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/js/sql/sql-prepare-false.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/sql/sql-prepare-false.test.ts info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (c992b92) test/js/sql/sql-prepare-false.test.ts: Container ready via docker-compose: postgres_plain at 127.0.0.1:5432 (pass) PostgreSQL prepare: false > basic parameterized query [243.39ms] (pass) PostgreSQL prepare: false > multiple parameterized queries sequentially [70.17ms] (pass) PostgreSQL prepare: false > same query repeated with different params [110.18ms] (pass) PostgreSQL prepare: false > concurrent queries with different tables return correct results [138.41ms] (pass) PostgreSQL prepare: false > parameterized query with multiple params [41.32ms] (pass) PostgreSQL prepare: false > query without params still works [30.85ms] (pass) PostgreSQL prepare: false > transactions with parameterized queries [122.23ms] (pass) PostgreSQL prepare: false > concurrent parameterized queries with high concurrency [248.81ms] 8 pass 0 fail 73 expect() calls Ran 8 tests across 1 file. [3.46s] Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/js/sql/sql-prepare-false.test.ts | 63 ++++++++++++++++------------------- 1 file changed, 28 insertions(+), 35 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/js/sql/sql-prepare-false.test.ts 1 1 0 ``` </details> <!-- robobun:evidence:end -->
4 participants
Summary
prepare: false(unnamed prepared statements) that could cause queries to silently execute against the wrong prepared statement, returning incorrect data or modifying wrong rowsprepare: falseis used with parameterized queries, the driver previously split the extended query protocol into two round-trips (Parse+Describe+Sync→ wait →Bind+Execute+Sync). The intermediateSynccauses aReadyForQueryresponse, which PgBouncer interprets as the end of the transaction and releases the server connection. The subsequentBind+Executecould then be routed to a different backend where the unnamed statement doesn't exist or was prepared with a different queryParseentirely and sent onlyBind+Execute, assuming the unnamed statement still existed — incorrect when PgBouncer multiplexes connectionsParse + [Describe] + Bind + Execute + Flush + Syncas a single atomic message batch. Named prepared statements (prepare: true, the default) are completely unaffectedTest plan
test/js/sql/sql-prepare-false.test.tswith 8 tests covering parameterized queries, concurrent queries, transactions, and high concurrency withprepare: false— all pass🤖 Generated with Claude Code