sql: retry connect failures while queries are waiting - #32028
Merged
Conversation
When a postgres or mysql server is starting up (e.g. a docker container that is still initializing), connection attempts fail at the socket level: the connection is refused, or an intermediary like the container port proxy accepts the TCP connection and closes it before the handshake completes. Both were reported as the generic ERR_POSTGRES_CONNECTION_CLOSED / ERR_MYSQL_CONNECTION_CLOSED "Connection closed", which is indistinguishable from an established connection dropping and gives no hint that the server was simply not ready. Introduce ERR_POSTGRES_CONNECTION_FAILED / ERR_MYSQL_CONNECTION_FAILED for connections that were never established: - a connect error (e.g. ECONNREFUSED) reports "Failed to connect" - a close while connecting or authenticating reports "Connection closed before the connection was established" ERR_*_CONNECTION_CLOSED now only means an established connection was closed. Server errors sent during startup (e.g. 57P03 "the database system is starting up") were already surfaced correctly and are unchanged. Fixes the misleading-error half of #16691.
A connect failure (ERR_*_CONNECTION_FAILED: refused, or closed before the handshake completed) usually means the server is still starting up - e.g. a docker container running initdb, whose port proxy accepts connections and closes them until the database inside is listening. Previously one round of failed connections failed every query waiting on the pool, so applications racing a database startup saw spurious errors that other clients (postgres.js, pg) ride out. Pooled connections now retry connect failures with exponential backoff (40ms doubling to a 1s cap) until connectionTimeout (default 30s) elapses from the start of the connect cycle, as long as queries are waiting on the pool. A server that becomes ready during that window is invisible to the application. When the budget runs out, the last connect error is reported as before. Only never-established connections are retried: authentication failures, server errors sent during startup (e.g. 57P03), and closes of established connections still fail immediately. Lowering connectionTimeout restores fast failure for callers that depend on it. Built on the CONNECTION_FAILED error classification from the previous commit; together they fix #16691.
Collaborator
|
Updated 2:31 PM PT - Jun 10th, 2026
❌ @robobun, your commit b81a131 has 6 failures in
🧪 To try this PR locally: bunx bun-pr 32028That installs a local version of the PR into your bun-32028 --bun |
Contributor
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
Server ErrorResponse messages sent during startup (like 57P03) surface as ERR_POSTGRES_SERVER_ERROR, not as a connect failure. Reword the table entry so the example matches the socket-level cases the code actually covers.
The postgres connect-failure paths routed constant strings through fail_fmt with the error code inlined, while the mysql side uses an AnyMySQLError::ConnectionFailed variant and the static fail() path. Add the matching variant so both adapters construct the error the same way and the code string has a single source of truth. No behavior change: the JS-visible code and messages are identical.
…l-retry-connect-failures
Fixes for three bugs in the connect-retry change found in review: - A graceful close() (no timeout) deadlocked when it raced a connect retry. The retry timer's pool-closed bail-out parked the slot without draining the waiting queries, and release() only checked onAllQueriesFinished before draining the waiting queue, so nothing ever signaled the close promise. The timer now finalizes the slot through the full close path, and release() re-checks onAllQueriesFinished after draining. - The user-supplied onclose callback fired once per retry attempt. It now fires once per closed connection slot. - An explicit connectionTimeout: 0 (which disables the connect timer) was coerced to a 30s retry budget; it now disables retries. The retry gate also only counts queries actually waiting on the pool instead of any in-flight query on healthy connections, and the timer callback re-checks all retry preconditions before re-dialing. Regression tests: graceful close() during a retry must resolve (hangs without the fix), onclose fires exactly once, connectionTimeout: 0 performs a single attempt.
Member
Author
|
Pushed a follow-up commit fixing three bugs found while reviewing the retry state machine:
Known and accepted: the retry logic is intentionally duplicated between |
Base automatically changed from
ali/postgres-startup-connection-close-race
to
main
June 9, 2026 23:04
…-failures # Conflicts: # test/js/sql/sql-connect-error-reporting.test.ts
Contributor
|
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:
WalkthroughAdds per-connection connect-cycle tracking and backoff reconnect scheduling for pooled MySQL and PostgreSQL connections, introduces distinct ERR_*_CONNECTION_REFUSED codes, cancels scheduled retries during shutdown, and updates tests and Postgres docs to reflect refusal vs accepted-then-closed behaviors. ChangesConnection retry resilience with timeout budget
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
CI showed that retrying refused connections breaks server probes: tests (and healthchecks) that connect to a local port to see whether a server exists expect an immediate error, and retrying turned that into a 30s wait that timed out sql-mysql-bind-blob-borrow.test.ts on every no-docker lane. Split the never-established case in two, matching postgres.js behavior: - ERR_*_CONNECTION_REFUSED (new): nothing is listening; fails immediately and is never retried. - ERR_*_CONNECTION_FAILED: the server accepted the TCP connection but closed it before the handshake completed (the signature of a database still starting up behind a port proxy); retried with backoff until connectionTimeout while queries are waiting. The refused code is new in unreleased main, so renaming the refused case does not affect any shipped release.
Member
Author
|
The Fixed by splitting the never-established case, which also matches postgres.js's behavior:
Verified: the probe pattern now fails in ~50ms with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/internal/sql/mysql.ts (1)
855-861:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPool shutdown drops
onclosefor retry-pending slots.Both dialects cancel the scheduled retry and flip the slot to
closedwithout running the normal close-finalization path. Since the retry path intentionally postponesconnectionInfo.oncloseuntil the slot actually closes, shutting the pool down during backoff suppressesoncloseentirely for that connection. Route this branch through the same finalization path the timer bail-out uses so the callback still fires exactly once and the cleanup stays consistent across shutdown and non-shutdown closes.🤖 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/mysql.ts` around lines 855 - 861, When handling PooledConnectionState.pending in the shutdown path, don’t just call connection.cancelRetry() and set connection.state = PooledConnectionState.closed; instead route the branch through the same close-finalization used by the timer bail-out so connectionInfo.onclose still runs exactly once and cleanup is consistent. Concretely: after connection.cancelRetry() returns true, invoke the same finalizer/cleanup routine that the timer bail-out branch calls (the code that runs connectionInfo.onclose and marks the slot closed) rather than directly mutating connection.state; ensure you guard against double-calling onclose (idempotent or check a flag) and preserve the existing state transitions performed by that finalizer.
🤖 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 `@docs/runtime/sql.mdx`:
- Line 1047: Rewrite the table cell for ERR_POSTGRES_CONNECTION_FAILED to split
the three clauses into a primary description and a short note for clarity: state
the condition first ("Connection was accepted but closed before the handshake
completed (e.g. server still starting up)."), then a second sentence about retry
behavior ("Retries with backoff until connectionTimeout while queries are
waiting."), and finally add a note clarifying the exception for server-sent
errors ("Note: errors the server sends during startup, like 57P03, surface as
ERR_POSTGRES_SERVER_ERROR."). Ensure the error code token
ERR_POSTGRES_CONNECTION_FAILED remains as the table row key and keep punctuation
tight and parallel.
---
Outside diff comments:
In `@src/js/internal/sql/mysql.ts`:
- Around line 855-861: When handling PooledConnectionState.pending in the
shutdown path, don’t just call connection.cancelRetry() and set connection.state
= PooledConnectionState.closed; instead route the branch through the same
close-finalization used by the timer bail-out so connectionInfo.onclose still
runs exactly once and cleanup is consistent. Concretely: after
connection.cancelRetry() returns true, invoke the same finalizer/cleanup routine
that the timer bail-out branch calls (the code that runs connectionInfo.onclose
and marks the slot closed) rather than directly mutating connection.state;
ensure you guard against double-calling onclose (idempotent or check a flag) and
preserve the existing state transitions performed by that finalizer.
🪄 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: e6957600-5d5a-4cb0-b335-0fff50f259cd
📒 Files selected for processing (14)
docs/runtime/sql.mdxsrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/ErrorCode.tssrc/sql/mysql/protocol/AnyMySQLError.rssrc/sql/postgres/AnyPostgresError.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/protocol/any_mysql_error_jsc.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/error_jsc.rstest/js/sql/sql-connect-error-reporting.test.tstest/js/sql/sql-mysql.test.tstest/js/sql/sql.test.ts
👮 Files not reviewed due to content moderation or server errors (1)
- test/js/sql/sql.test.ts
Member
Author
|
@robobun please fix review comments/ci |
The retry logic is intentionally duplicated between the postgres and mysql pool adapters, so behavioral tests need to exist for both copies to catch drift. These two only had postgres coverage.
Collaborator
|
All review comments are addressed in 735f41d (hoisted retry timer callbacks via setTimeout arguments, typed instanceof PostgresError/MySQLError checks); threads resolved and the PR is approved. Final CI state for b81a131 (build 61786): 285 passed, 1 red. The only failure is the darwin 14 aarch64 lane timing out on both attempts; that lane has been failing on every branch since #32033 (its autobahn docker image runs amd64 on the arm64 host). Nothing red traces to this diff. |
alii
commented
Jun 10, 2026
alii
commented
Jun 10, 2026
alii
commented
Jun 10, 2026
Member
Author
|
@robobun fix the comments |
cirospaciari
approved these changes
Jun 10, 2026
alii
enabled auto-merge (squash)
June 10, 2026 20:56
robobun
added a commit
that referenced
this pull request
Jun 10, 2026
Resolve conflicts with #32028 (connect-failure retry) by moving the retry machinery into BasePooledConnection in shared.ts with a per-driver isConnectFailureError hook, replacing the mirrored copies that main added to postgres.ts and mysql.ts.
This was referenced Jun 10, 2026
alii
pushed a commit
that referenced
this pull request
Jun 11, 2026
…ection pool (#32041) Fixes #32037. ### Problem In the postgres and mysql pooled-connection handlers (`#onConnected` / `#onClose` in `src/js/internal/sql/postgres.ts` and `mysql.ts`), the user-provided `onconnect`/`onclose` callbacks ran before the pool updated its own bookkeeping. A throwing callback aborted the handler mid-way: `state` stayed `pending`, `storedError` was never recorded, pending queries were never notified, `onFinish` never ran, and `release()` never ran. Anything awaiting the pool hung forever: ```ts const sql = new SQL({ /* ... */, max: 1, onconnect() { throw new Error("boom"); }, }); await sql`SELECT 1`; // never settles ``` Same for `onclose`: pending queries were never failed on a connect failure, and `sql.end()` never resolved. ### Fix Run the callback in a `try`/`finally` so the bookkeeping always completes, at all four sites (postgres/mysql x onconnect/onclose). The exception still propagates out of the handler afterwards and is reported as an uncaughtException through the same channel as before; only the skipped bookkeeping changes. (sqlite.ts already guards its hooks.) Secondary fix in the same re-entrancy family, also called out in the issue: postgres's `createConnection` catch invoked `onClose` synchronously, which could run the user's `onclose` while the adapter was still filling `this.connections` during pool startup (reachable via a `password` function that throws). Pool methods that scan that array without the hole guard `hasConnectionsAvailable()` has (`flush()`, `isConnected()`, the private `close()`) threw `TypeError: undefined is not an object` when called from inside the callback. The catch now defers via `process.nextTick`, exactly like the identical catch in mysql.ts already did, so the callback never observes a half-filled pool. ### Verification New `test/js/sql/sql-onconnect-onclose-throw.test.ts`. The established-connection scenarios (throwing `onconnect`, throwing `onclose` on `sql.end()`) run against the real docker-compose `postgres_plain` and `mysql_plain` services via `describeWithContainer`, like the other sql tests. The connection-refused scenarios use a real closed port and the synchronous-failure scenario never dials, so those run without docker. On the unfixed build every scenario fails (the docker ones verified manually against real postgres and mariadb: the fixture hangs without the fix and completes with it): <details> <summary>before the fix</summary> ``` (fail) postgres: pool calls from onclose are safe when connecting fails synchronously expected "reentry ok", got "reentry threw: TypeError" (fail) postgres: a throwing onconnect callback does not leave the pool stuck [5000.46ms] ^ this test timed out after 5000ms. (fail) mysql: a throwing onconnect callback does not leave the pool stuck [5000.10ms] ^ this test timed out after 5000ms. (fail) postgres: a throwing onclose callback does not hang sql.end() [5000.06ms] ^ this test timed out after 5000ms. (fail) mysql: a throwing onclose callback does not hang sql.end() [5000.06ms] ^ this test timed out after 5000ms. (fail) postgres: a throwing onclose callback still rejects pending queries on connect failure [5000.06ms] ^ this test timed out after 5000ms. (fail) mysql: a throwing onclose callback still rejects pending queries on connect failure [5000.06ms] ^ this test timed out after 5000ms. ``` </details> With the fix all 7 pass, and the issue's original repro scripts against real postgres and mariadb servers now settle (`query result: [{"x":1}]`, `sql.end()` resolves) while the callback error still surfaces as an uncaughtException. `sql-connect-error-reporting.test.ts`, `sql-mysql-clean-reentry.test.ts` and `sql-mysql-cached-error.test.ts` still pass. Note: this predates #31994 (which dedupes these two files into a shared base class); if that lands first, the same change applies once to its `shared.ts`. ### Rebase note (after #32028 landed) #32028 restructured `#onClose`: connect failures now retry with backoff and the `onclose` callback moved into a new `#finishClose`, which the retry timer also calls. Resolved by putting the `try`/`finally` around the hook in `#finishClose` (covering both callers) and keeping `#onClose`'s retry branch untouched (no user code runs there). `#onConnected` keeps main's `connectStartedAt = 0` reset inside the `finally`. #32028 also split refused connections into `ERR_*_CONNECTION_REFUSED` (fail fast, not retried), so the two refused-port tests assert that code now.
This was referenced Jun 11, 2026
alii
added a commit
that referenced
this pull request
Jun 11, 2026
…gres and mysql (#32135) ## What this does Second slice carved out of #31994 (closed as too big), stacked on #32128 (merged). This one is the `src/sql_jsc/` Rust constructor-args dedup — no JS adapter changes. Net −58 lines across 20 files. Three commits: - **`src/sql/` prerequisites** (46d3fb8) — `QueryStatus` moves from `mysql/` to `shared/` (re-exported under the old `mysql::query_status` path so nothing breaks); new `shared/StatementStatus` with the same 4 variants both drivers already had; `FieldMessage` gains a `payload()` accessor so the only caller no longer needs an external exhaustive match; `NoticeResponse` becomes a type alias of `ErrorResponse` decoded via `decode_notice_internal` (same wire format, body identical to the old `NoticeResponse::decode_internal`). - **`src/sql_jsc/` dedup** (dc96ffd) — the headline change. Extracts the duplicated `createQuery(...)` and `createConnection(...)` argument parsing/validation prologue from `JSMySQLQuery` / `PostgresSQLQuery` and `JSMySQLConnection` / `PostgresSQLConnection` into `src/sql_jsc/shared/{query_ctor_args,connection_ctor_args}.rs`. Also moves the duplicated `dedupe_columns` from `MySQLStatement` / `PostgresSQLStatement` into `shared/SQLDataCell.rs`, switches both Statement types to the shared `StatementStatus`, and drops the now-unused `notice_response_jsc.rs` (replaced by `FieldMessage::payload()`). - **Test coverage for the rewired NoticeResponse path** (159cea4) — adds `NoticeResponse` and degenerate empty-notice cases to the existing async-message framing test in `test/js/sql/postgres-multi-statement-fields.test.ts`. This path had no prior coverage in `test/js/sql`. ## Behavioral equivalence Line-by-line diff of every removed per-driver block against the new shared helper: - All five `query_ctor_args` error messages verbatim (`"query must be a string"`, `"values must be an array"`, `"simple query cannot have parameters"`, `"query is too long"`, `throw_invalid_argument_type("query", "pendingValue", "Array")`); validation order unchanged. - `connection_ctor_args` validation order unchanged; `ssl_mode` decode equivalent for all i32 (negative → Disable, 0–4 → indexed, ≥5 → Disable); `tls` error message verbatim; same per-VM `SSL_CTX*` cache and `as_usockets_for_client_verification()` key; same `SSL_CTX_free` symbol. - `dedupe_columns` iteration order and reserve size identical. `.expect("OOM")` → `.unwrap_or_oom()` routes through `bun_core::handle_oom` per repo convention — same crash on OOM, no observable difference otherwise. - `bun_vm()` vs `sql_vm()`: the latter is `#[inline] fn sql_vm(&self) { self.bun_vm() }`. No user-reachable input produces a different output, error message, validation order, or SSL_CTX cache key. ## Verification - `cargo check -p bun_sql -p bun_sql_jsc` and `cargo clippy --no-deps`: clean, zero warnings. - `cargo fmt`: no diffs. - `bun bd test` mock-server SQL suite (`postgres-binary-numeric`, `postgres-binary-array-bounds`, `postgres-multi-statement-fields`, `sql-mysql-auth-short-nonce`, `sql-connect-error-reporting`): 41 pass / 0 fail (39 prior + 2 new NoticeResponse cases). - No commits on `main` have touched `src/sql_jsc/` since #31994's merge-base, so the per-driver files are a clean checkout with no clobbered work. ## Left for a follow-up The `src/js/internal/sql/shared.ts` JS adapter consolidation from #31994 (the `BasePooledConnection` extraction, +1308/−2273) is intentionally not in this PR — that's the higher-risk piece touching pool/connection lifecycle and the part that conflicted with #32028.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Builds on #32027 (the
ERR_*_CONNECTION_FAILEDerror classification), which has merged; this PR now targetsmainand contains only the retry change.Fixes #16691
What does this PR do?
The behavioral half of #16691. A connect failure (
ERR_*_CONNECTION_FAILED: connection refused, or accepted-then-closed before the handshake completed) usually means the server is still starting up — the canonical case is a postgres/mysql docker container whose port proxy accepts connections and closes them with no data until the database inside is listening (a 2.5+ second window on every first boot, measured). Previously one round of dead sockets failed every query waiting on the pool, so applications racing a database startup saw spurious errors that postgres.js and pg ride out.With this change, a pooled connection that hits a connect failure retries with exponential backoff (40ms doubling to a 1s cap) until
connectionTimeout(default 30s) elapses from the start of the connect cycle, as long as queries are waiting on the pool. A server that becomes ready during that window is invisible to the application. When the budget runs out, the last connect error is reported exactly as before.Scope guard — what is NOT retried: refused connections (
ERR_*_CONNECTION_REFUSED, new code: nothing is listening, so probes and healthchecks keep their instant error), authentication failures, real server errors sent during startup (e.g.57P03 "the database system is starting up"), and closes of established connections all still fail immediately. Only accepted-then-closed-before-handshake connections (ERR_*_CONNECTION_FAILED) retry — matching postgres.js.Behavioral tradeoff, stated plainly: an application that relies on queries failing fast against an unreachable server will now wait up to
connectionTimeout(30s default) when queries are pending. That matches postgres.js (connect_timeout) semantics, andconnectionTimeout: 1restores ~1s failure. This is why the change is its own PR.What changed
src/js/internal/sql/postgres.ts/mysql.ts(mirrored): pooled connections track the connect-cycle start time;#onCloseschedules a backoff retry instead of failing waiters when the error isCONNECTION_FAILED, queries are pending, the pool isn't closing, and the budget hasn't elapsed. Pool close cancels scheduled retries (including the deferred-close window, soclose({ timeout })can't hang on a retry-parked slot).retry()path (reconnect when a new query finds a closed slot) is unchanged; this fills in retry for queries already waiting.connectionTimeout: 1so they keep failing fast.How was this tested?
test/js/sql/sql-connect-error-reporting.test.tsextended (all mock TCP servers, no docker):connect()succeeds, no error visible57P03ErrorResponse: surfaced immediately, exactly 1 connection attempt (not retried)sql.test.ts,sql-mysql.test.ts,sqlite-sql.test.tsproduce the identical pass/fail set as a cleanmaincheckout run side-by-side (remaining failures are pre-existing docker-infra flakes).