sql(postgres): stop sending a redundant Sync after a simple Query

[robobun]

What

Bun.SQL (Postgres) silently delivers another query's rows to the wrong query, with no error, when a simple-protocol query runs concurrently with a not-yet-prepared parameterized query on the same connection. A second variant of the same bug makes the third query hang forever.

The simple query protocol ('Q') is used by query.simple(), by sql.unsafe(text) with no parameters (src/js/bun/sql.ts:134), and by the BEGIN/COMMIT/ROLLBACK that sql.begin() issues, so this is reachable without the caller ever opting into simple mode.

Repro

Reproduces on Bun 1.3.14, 1.4.0, and main, against a real Postgres, every run:

const sql = new Bun.SQL({ ...conn, max: 1 });

const [a, b, c] = await Promise.all([
  sql.unsafe(`SELECT 'AAAA'::text AS v`),       // no params -> simple 'Q'
  sql.unsafe(`SELECT $1::text AS v`, ["BBBB"]), // first use -> Parse/Describe round trip
  sql.unsafe(`SELECT 'CCCC'::text AS v`),       // no params -> simple 'Q'
]);
// a -> [{ v: "AAAA" }]   ok
// b -> [{ v: "CCCC" }]   b got C's row
// c -> [{}]              c got B's field-less Bind/Execute row

The same happens with .simple() in place of the no-parameter unsafe() calls. If the third query is parameterized instead, it never resolves.

Cause

PostgresRequest::execute_query wrote Query ('Q') + Flush + Sync for the simple query protocol. A simple Q is its own sync point: the backend always answers it with exactly one ReadyForQuery. The trailing Sync made the server send a second, unaccounted ReadyForQuery per simple query.

The only place IS_READY_FOR_QUERY is set (and WAITING_TO_PREPARE cleared) is the ReadyForQuery handler, and the two flags are otherwise mutually exclusive with the prepare path. So the state machine is correct as long as every ReadyForQuery corresponds to an operation Bun wrote. The extra one breaks that: it arrives right after advance() has written the next query's Parse+Describe+Sync, re-arms IS_READY_FOR_QUERY, clears WAITING_TO_PREPARE, and triggers another advance(). That pass skips the head query (its statement is Parsing) and admits the third query into the prepare's Describe window. From there the FIFO reply dispatch hands each arriving result set to the wrong head-of-queue request.

Wire traces captured through a logging proxy against a real Postgres 17:

before (broken) and after (fixed)

Before: A's extra Sync yields two Z. The second one lets C's Q out between B's Parse/Describe/Sync and B's Bind/Execute, so C's result set (T D C Z) arrives while B is still the current query.

-> Q "SELECT 'AAAA' AS v"
-> H  (Flush)
-> S  (Sync)           <- redundant
<- T D C
<- Z                   <- A's ReadyForQuery
<- Z                   <- extra ReadyForQuery from A's Sync
-> P "SELECT $1 AS v"  <- B: Parse
-> D                   <- B: Describe
-> S                   <- B: Sync
-> Q "SELECT 'CCCC' AS v"   <- C, admitted by the extra Z's advance()
-> H
-> S
<- 1 t T Z             <- B's describe reply
<- T D C Z Z           <- C's result set, delivered to B
-> B E H S             <- B: Bind/Execute
<- 2 D C Z             <- B's result set, delivered to C

After: one Z per simple query, and each query's full exchange completes before the next one is written.

-> Q "SELECT 'AAAA' AS v"
-> H
<- T D C Z
-> P "SELECT $1 AS v"
-> D
-> S
<- 1 t T Z
-> B E H S
<- 2 D C Z
-> Q "SELECT 'CCCC' AS v"
-> H
<- T D C Z

sql.begin() after the fix, for reference (the transaction-status byte in Z tracks I -> T -> T -> T -> I correctly):

-> Q "BEGIN"                       -> H
<- Z status=T
-> P/D "UPDATE ... WHERE id = $1"  -> S
<- 1 t T, Z status=T
-> B E                             -> H -> S
<- 2 D C, Z status=T
-> Q "COMMIT"                      -> H
<- Z status=I

The Flush + Sync after Q dates to the original Zig implementation of executeQuery and was carried through the Rust port unchanged. Flush after a Q elicits no response, so it is left alone.

Fix

Stop writing Sync after a simple Q in execute_query. One ReadyForQuery per operation is the invariant the rest of the connection state machine already assumes.

Test

test/js/sql/postgres-simple-query-pipeline.test.ts, against a real Postgres via describeWithContainer. All three cases fail on the unfixed build:

(fail) postgres > a simple query does not steal the rows of an in-flight prepare
  expected b: [{v:"BBBB"}] c: [{v:"CCCC"}], received b: [{v:"CCCC"}] c: [{}]
(fail) postgres > unsafe() with no parameters does not steal the rows of an in-flight prepare
  same misattribution through the sql.unsafe() entry point
(fail) postgres > a second prepare queued behind an in-flight prepare still settles
  ^ this test timed out after 5000ms.

and pass with the fix. The existing test/js/sql/ Postgres suites (postgres-multi-statement-fields, sql-prepare-false, wire-frames, the binary/onconnect/close ones) still pass.

[robobun]

^{Updated 10:46 AM PT - Jun 26th, 2026}

❌ @robobun, your commit b78fd1a has 1 failures in Build #65051 (All Failures):

• test/cli/install/bun-install.test.ts - code 1 on 🪟 2019 x64

---

🧪   To try this PR locally:

bunx bun-pr 32772

That installs a local version of the PR into your bun-32772 executable, so you can run:

bun-32772 --bun

[github-actions]

Found 2 issues this PR may fix:

1. SQL: connection pool stalls under concurrent begin() + parameterized queries (idle connections never handed to pending queries) #32004 - Pool stalls under concurrent begin() + parameterized queries — the redundant Sync causing an extra ReadyForQuery prematurely re-arms IS_READY_FOR_QUERY and triggers advance() out of turn, which can leave transaction connections stuck in a state the pool never reclaims
2. Bun.sql pegs a CPU while doing approximately nothing #22040 - CPU pegs at 100% with excessive __recvfrom on simple periodic SELECT 1 — the extra ReadyForQuery from the redundant Sync could cause the client state machine to spin in a busy-loop processing unexpected messages

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #32004
Fixes #22040

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2022b384-284a-4dd4-8ec9-5c2127eda64f

📥 Commits

Reviewing files that changed from the base of the PR and between 0bd7af5 and b78fd1a.

📒 Files selected for processing (1)

• src/sql_jsc/postgres/PostgresRequest.rs

---

Walkthrough

Simple Postgres queries now omit an extra SYNC after write_query, and new regression tests cover concurrent prepare/query ordering on a single SQL connection with max: 1.

Changes

Postgres simple-query pipeline

Layer / File(s)	Summary
Simple-query write path src/sql_jsc/postgres/PostgresRequest.rs	execute_query writes the query with protocol::write_query(...), flushes without SYNC, and adds comments about the extra ReadyForQuery case.
Pipeline regression tests test/js/sql/postgres-simple-query-pipeline.test.ts	Adds postgres-simple-query-pipeline regression tests that run concurrent SQL operations on one connection and check row and field ownership across simple-query and prepare-order cases.

Suggested reviewers

• cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states the main fix: removing a redundant Sync after a simple Postgres query.
Description check	✅ Passed	It covers the change, repro, cause, fix, and verification, though it uses custom headings instead of the template.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

The one-line removal looks correct per the Postgres protocol spec (simple Query already elicits its own ReadyForQuery), and the wire traces and regression tests are convincing — but since this touches the connection state machine for the production Postgres driver, it's worth a quick human sanity-check before merging.

Extended reasoning...

Overview

This PR removes a single writer.write(&protocol::SYNC)? line from execute_query in src/sql_jsc/postgres/PostgresRequest.rs, which writes the simple-protocol Q message. It adds a new test file with two regression tests against a real Postgres container via describeWithContainer.

The root-cause analysis is well-documented and matches the PostgreSQL protocol spec: a simple Query message is always answered with a terminal ReadyForQuery, and Sync (an extended-protocol message) independently elicits another ReadyForQuery. Sending both yields two Z messages per simple query, and the connection's IS_READY_FOR_QUERY / WAITING_TO_PREPARE state machine assumes one Z per operation. The PR includes before/after wire traces from a logging proxy that confirm the duplicate Z and the resulting misrouted result sets.

Security risks

None identified. This is a wire-protocol sequencing fix; no auth, crypto, input parsing, or trust-boundary changes.

Level of scrutiny

Moderate-to-high. The diff is one line, and the protocol reasoning is provably correct against the spec. But execute_query is on the hot path of the production Postgres driver, and the surrounding pipelining/state-machine logic in PostgresSQLConnection.rs (advance(), can_pipeline, can_prepare_query, nonpipelinable_requests) is intricate enough that a maintainer familiar with it should confirm nothing else implicitly depended on the second ReadyForQuery (e.g., error recovery or transaction-state transitions). Note that simple-protocol error handling does not require Sync — the backend sends ErrorResponse then ReadyForQuery on its own — so that path should be unaffected.

Other factors

• No bugs found by the automated review.
• Test follows the established describeWithContainer("postgres", { image: "postgres_plain" }, ...) pattern used by neighboring tests.
• PR description states the existing test/js/sql/ Postgres suites still pass.
• No CODEOWNERS entry for this path.
• CI build was still in progress at review time (#65034).

[robobun]

Looked into both of the suggested issues before adding any Fixes lines. Neither belongs on this PR.

#22040 is Bun.sql with a tagged template (sql`SELECT 1`). That goes through the extended protocol (Parse/Bind/Execute), which never reaches execute_query, so this change is a no-op for it. The pegged CPU and the __recvfrom volume point at the socket read loop, not the SQL state machine.

#32004 looked plausible, because sql.begin()'s BEGIN and COMMIT do go through the simple protocol and therefore did hit the redundant Sync. So I ran its repro against this branch instead of guessing: it still reproduces. On a debug build of this branch the repro aborts 14/90 runs, and on a debug build with this PR's one-line change reverted it aborts 9/90, so the rates are not distinguishable. The failure is the same in both:

panic: attempt to subtract with overflow
  PostgresSQLConnection::finish_request
  src/sql_jsc/postgres/PostgresSQLConnection.rs:1787   (nonpipelinable_requests: u32, 0 - 1)

In a release build that subtraction silently wraps to u32::MAX, after which advance() always returns early and never writes anything again. That is exactly #32004's "every connection idle, nothing progresses" stall, it is pre-existing on main, and it matches the analysis already in #32006, which targets it directly (the pool handoff and the run() fast-path write ordering, both independent of the redundant Sync). Leaving #32004 to that PR.

While verifying this I found that the original description understated the reach of this bug: the simple query protocol, and therefore the redundant Sync, is used not only by query.simple() but by sql.unsafe(text) with no parameters and by the BEGIN/COMMIT/ROLLBACK of every sql.begin() (all via src/js/bun/sql.ts). I updated the description and added a third test covering the sql.unsafe() entry point; it fails the same way on the unfixed build.

On the automated review's question of whether anything depended on the second ReadyForQuery: the post-fix sql.begin() wire trace is in the description's details block. The transaction status byte in ReadyForQuery tracks I -> T -> T -> T -> I correctly with one ReadyForQuery per operation, and simple-protocol errors already terminate with their own ReadyForQuery without needing a Sync, so the error path is unaffected.

[coderabbitai]

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/postgres-simple-query-pipeline.test.ts`:
- Around line 43-58: The new regression test is in the wrong place and should be
moved into the existing Postgres SQL test coverage instead of living in a
separate standalone suite. Relocate the `unsafe() with no parameters does not
steal the rows of an in-flight prepare` test to the established Postgres SQL
test file that already covers `SQL.unsafe()` and related query behavior, keeping
it alongside the existing `SQL`/`sql.unsafe(...)` tests. Preserve the same test
logic and assertions, but insert it near the related Postgres SQL cases so the
coverage stays discoverable and organized.

🪄 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: 293eebf0-153a-45d8-8cc2-e751dba699b4

📥 Commits

Reviewing files that changed from the base of the PR and between e179cdb and 0bd7af5.

📒 Files selected for processing (1)

• test/js/sql/postgres-simple-query-pipeline.test.ts

[claude]

Thanks for addressing the comment nit in b78fd1a — the change looks correct and the wire-trace verification is thorough, but since this alters Postgres wire-protocol sequencing on a data-correctness path I'd like a human (the SQL maintainer) to give it a final look.

Extended reasoning...

Overview

The functional change is a single line: execute_query in src/sql_jsc/postgres/PostgresRequest.rs no longer writes Sync after a simple-protocol Query ('Q'). A three-line comment documents the invariant (a simple Q is its own sync point) and the deliberate deviation from sibling writers, which all end Flush+Sync. A new test file adds three regression tests against a real Postgres via describeWithContainer, covering .simple(), sql.unsafe() with no parameters, and a second prepare queued behind an in-flight prepare.

My one prior inline nit (rephrase the past-tense bug-history sentence as a forward-looking invariant) was addressed in b78fd1a and the thread is resolved. CodeRabbit's test-placement suggestion was withdrawn after a well-reasoned rebuttal. The bug-hunting system found nothing.

Security risks

None. No auth, crypto, permissions, or untrusted-input parsing is touched. The change only removes one outbound protocol byte sequence; it does not change how inbound data is parsed or trusted.

Level of scrutiny

This is where I land on deferral rather than approval. The diff is tiny and the Postgres protocol spec is unambiguous that a simple Q always concludes with its own ReadyForQuery, so the fix is protocol-correct. The PR description is exceptionally rigorous: before/after wire traces against Postgres 17, verification that sql.begin() transaction-status tracking (I→T→T→T→I) still works, confirmation that the simple-protocol error path already terminates with its own ReadyForQuery, and a careful negative result on the two suggested linked issues.

However, the effect of this one line is a change to how the connection state machine's IS_READY_FOR_QUERY / WAITING_TO_PREPARE sequencing interacts with request pipelining — the exact area where the original bug caused silent row misattribution. The redundant Sync dates back to the original Zig implementation, so this is removing long-standing (if incorrect) behavior. That's the kind of change where the SQL maintainer's domain knowledge of edge cases (PgBouncer, pipelined error recovery, COPY, etc.) is worth more than my read of the spec.

Other factors

No CODEOWNERS entry covers src/sql_jsc/. Tests follow the established test/js/sql/postgres-*.test.ts + describeWithContainer pattern and the author confirmed the existing Postgres suites still pass. The trailing Flush after Q is also technically a no-op per the protocol but was deliberately left alone, which is appropriately conservative for this PR's scope.

[claude]

Thanks for the comment reword — that resolves my only nit. The one-line fix and wire traces look correct to me, but since this changes the simple-protocol write path that every .simple(), parameterless unsafe(), and sql.begin() BEGIN/COMMIT flows through, I'd like a Postgres-driver maintainer to give the final sign-off.

Extended reasoning...

Overview

The PR removes a single writer.write(&protocol::SYNC)?; from execute_query() in src/sql_jsc/postgres/PostgresRequest.rs and adds a three-line invariant comment explaining why. A new test file test/js/sql/postgres-simple-query-pipeline.test.ts adds three regression tests (via describeWithContainer) covering the three observed failure modes: row misattribution via .simple(), the same via parameterless sql.unsafe(), and a hung second prepare.

The root cause is well-established by the Postgres protocol: a simple Query (Q) always concludes with its own ReadyForQuery, so an appended Sync produces a second, unaccounted ReadyForQuery that re-arms advance() mid-prepare. The author captured before/after wire traces against Postgres 17, verified sql.begin() transaction-status tracking (I→T→T→T→I) and the simple-query error path are unaffected, and confirmed the existing test/js/sql/ Postgres suites still pass. My one prior nit (past-tense comment phrasing) was addressed in b78fd1a and the thread is resolved; CodeRabbit's test-placement suggestion was withdrawn.

Security risks

None identified. This is a wire-protocol write-ordering fix; no auth, crypto, input-parsing, or trust-boundary changes.

Level of scrutiny

High. Although the diff is one functional line, it sits on the Postgres simple-protocol write path that every .simple() query, every parameterless sql.unsafe(), and every BEGIN/COMMIT/ROLLBACK from sql.begin() flows through. The bug it fixes is silent row misattribution — a data-correctness failure — and the surrounding connection state machine has other known subtle issues nearby (the author found a nonpipelinable_requests underflow in finish_request while investigating #32004). Protocol-level changes to a DB driver warrant a maintainer with domain context confirming nothing else depends on the second ReadyForQuery.

Other factors

• No CODEOWNERS entry for src/sql_jsc/ or test/js/sql/.
• The one CI failure (test/cli/install/bun-install.test.ts on Windows x64) is unrelated to this change.
• CodeRabbit suggested cirospaciari as a reviewer, which aligns with deferring to a Postgres-driver maintainer.
• Bug-hunting system found nothing; all inline review threads are resolved.

[robobun]

Status for a reviewer: the diff is complete, all review threads are resolved, and this needs a maintainer.

On CI:

• The only red lane was test/cli/install/bun-install.test.ts (the Git URL tests) on Windows Server 2019 x64 in build 65051 at b78fd1ac. The failing test received @bar/baz, which is the workspace fixture name from a different test in that same file, so it is a test-isolation flake internal to bun-install.test.ts. This diff (one function in the Postgres wire client plus one test/js/sql file) cannot reach it, and the merge base 05895486df (main build 64901) passed it.
• The other two failures in that build (hot.test.ts and dev-and-prod.test.ts, both Windows) auto-retried to green and were annotated as flaky by CI.
• The ci: retrigger commit on the current head (c019ee5) is empty, and Buildkite does not create a build for a push with an unchanged tree, so the head has no Buildkite build. The GitHub Actions checks on it (Format, Lint JavaScript, cargo clippy) all pass. Kicking a Buildkite build on the branch will produce a clean run.

The repro, protocol citation, before and after wire traces, and the fail-before / pass-after output for all three tests are in the PR description.