Deduplicate SQL driver internals across postgres/mysql/sqlite#31994
Deduplicate SQL driver internals across postgres/mysql/sqlite#31994alii wants to merge 10 commits into
Conversation
|
Updated 5:02 PM PT - Jun 10th, 2026
❌ @robobun, your commit 47c520c has some failures in 🧪 To try this PR locally: bunx bun-pr 31994That installs a local version of the PR into your bun-31994 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
@robobun adopt |
…ice response binding
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughCentralizes SQL normalization, pooled-connection bases, and StackReader/statement-status into shared modules; adapters (MySQL/Postgres/SQLite) and JSC bindings now delegate to shared normalization, pooling, and ctor-parsing helpers. ChangesSQL Infrastructure Consolidation
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 |
|
Adopted. Verified on a debug (ASAN) build: audited the Rust and JS diffs against main for behavioral drift (none found; the PR is a pure move, so the added NoticeResponse framing tests are equivalence guards), ran test/js/sql, and ran a live smoke check of helpers, transactions, error codes, notices, and duplicate columns against real postgres and mariadb servers plus sqlite, matching the released bun. All review threads are resolved; the three pre-existing defects surfaced during review are tracked in #32035, #32037, and #32038. Latest: resolved the merge conflict with #32028 (connect-failure retry) by hosting the retry machinery once in BasePooledConnection in shared.ts with a per-driver isConnectFailureError hook, instead of the mirrored copies main added to each driver (details in the PR description). All 15 of the #32028 tests pass on the merged branch, and full test/js/sql is 518 pass with the same 2 pre-existing sqlite debug timeouts as before. |
|
@claude review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/mysql.ts`:
- Around line 268-270: The isUpsertUpdate function currently checks only for the
exact uppercase suffix; change its logic to perform a case-insensitive check
(e.g., normalize query by trimming and lowercasing or use a case-insensitive
regex) and then test for the suffix "on duplicate key update" so variants like
"on duplicate KEY Update" are correctly detected; update isUpsertUpdate to use
that case-insensitive comparison.
In `@src/js/internal/sql/shared.ts`:
- Around line 998-1013: The pool methods isConnected(), flush(), and `#close`()
currently assume each entry in this.connections is non-null and access
connection.state unguarded; mirror the defensive check used in
hasConnectionsAvailable() by verifying the slot isn't an unassigned hole before
reading its state (e.g. ensure connection is truthy and only then check
connection.state !== PooledConnectionState.closed), and apply the same guard in
every re-entrant scan path to avoid throws when onconnect/onclose callbacks
re-enter during startup; update isConnected, flush, and `#close` to skip
null/undefined slots the same way hasConnectionsAvailable does.
- Around line 607-635: handleConnected/handleClose currently invoke user hooks
(connectionInfo.onconnect / connectionInfo.onclose) before updating internal
bookkeeping, so exceptions from those hooks can abort and leave the pool in an
inconsistent state; modify both handleConnected and handleClose to either (1)
move all internal updates (this.storedError, this.flags adjustments, this.state,
this.queryCount reset, and the eventual this.adapter.release call) to occur
before calling the user hook, or (2) wrap the user hook invocation in a
try/catch so that any thrown error is caught and logged/ignored and does not
prevent the subsequent updates and release; ensure references to this.onFinish
and connection?.close are also executed regardless of hook exceptions so adapter
invariants are preserved.
- Around line 1097-1125: The guard `if (timeout)` treats 0 as falsy so
close({timeout: 0}) falls through; change the branch condition to detect
presence of the option instead (e.g., use `if (options?.timeout !== undefined)`
or `if (Object.prototype.hasOwnProperty.call(options, 'timeout'))`) so the code
that validates Number(timeout), handles `timeout === 0` immediate-close, and
sets up the timer/onAllQueriesFinished logic still runs for a provided 0 value;
keep the existing Number(timeout) conversion and NaN/range checks and leave uses
of `this.closed`, `this.hasPendingQueries()`, `this.#close()`,
`Promise.withResolvers`, `timer.unref()` and `this.onAllQueriesFinished`
unchanged.
In `@src/sql_jsc/mysql/JSMySQLConnection.rs`:
- Around line 473-476: The constructor currently calls
ConnectionCtorArgs::<SSLMode>::parse(...) directly, which causes tls: null or
tls: false to be rejected; update the shared parser ConnectionCtorArgs::parse to
treat JS values null and false as "no TLS" (equivalent to absent) when ssl_mode
!= Disable before validating true/object, and also update the sibling Postgres
constructor that uses the same helper so both MySQL and Postgres accept tls:
null/false consistently with the createConnection contract; ensure the parser
returns the same parsed structure for absent/null/false and that callers (e.g.,
the MySQL constructor and the Postgres constructor) handle that result without
special-casing.
In `@src/sql_jsc/shared/SQLDataCell.rs`:
- Around line 403-406: Replace the panic-on-OOM call in the dedupe code: instead
of calling seen_fields.get_or_put(name.slice()).expect("OOM") use the
OOM-handling helper (either .unwrap_or_oom() on the Result or call
bun_core::handle_oom) so allocation failures follow Bun’s controlled-OOM path;
update the expression around seen_fields.get_or_put(name.slice()) to propagate
or convert the AllocError via unwrap_or_oom()/handle_oom() before accessing
.found_existing.
🪄 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: 32135b11-d4f1-49b9-a251-ce9a00ed22ea
📒 Files selected for processing (28)
src/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/shared.tssrc/js/internal/sql/sqlite.tssrc/sql/lib.rssrc/sql/mysql/Capabilities.rssrc/sql/mysql/protocol/NewReader.rssrc/sql/mysql/protocol/StackReader.rssrc/sql/postgres/protocol/ErrorResponse.rssrc/sql/postgres/protocol/FieldMessage.rssrc/sql/postgres/protocol/NoticeResponse.rssrc/sql/postgres/protocol/StackReader.rssrc/sql/shared/QueryStatus.rssrc/sql/shared/StackReader.rssrc/sql/shared/StatementStatus.rssrc/sql_jsc/lib.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/JSMySQLQuery.rssrc/sql_jsc/mysql/MySQLStatement.rssrc/sql_jsc/postgres.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rssrc/sql_jsc/postgres/PostgresSQLStatement.rssrc/sql_jsc/postgres/protocol/error_response_jsc.rssrc/sql_jsc/postgres/protocol/notice_response_jsc.rssrc/sql_jsc/shared/SQLDataCell.rssrc/sql_jsc/shared/connection_ctor_args.rssrc/sql_jsc/shared/query_ctor_args.rs
💤 Files with no reviewable changes (3)
- src/sql_jsc/postgres.rs
- src/sql_jsc/postgres/protocol/notice_response_jsc.rs
- src/sql/mysql/protocol/NewReader.rs
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.
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)
130-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the native handle during pending MySQL connects.
BaseSQLAdapter.#close()now treatspendingconnections as abortable by callingconnection.connection?.close()before awaitingonFinish(src/js/internal/sql/shared.ts, Lines 1164-1177). This path drops the handle returned bycreatePooledConnectionHandle()and only assignsthis.connectionafterhandleConnected()succeeds, soclose()cannot cancel an in-flight MySQL dial/handshake and can stall until the native connect timeout fires.Suggested fix
- protected startConnection() { - createPooledConnectionHandle( + protected async startConnection() { + this.connection = await createPooledConnectionHandle( createMySQLConnection, this.connectionInfo, this.handleConnected.bind(this), this.handleClose.bind(this), true, ); }🤖 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 130 - 137, The current startConnection calls createPooledConnectionHandle but drops its returned native handle so BaseSQLAdapter.close cannot abort an in-flight MySQL dial; modify startConnection (and related logic) to capture and preserve the handle returned by createPooledConnectionHandle (call to createMySQLConnection) immediately (e.g., assign to this.connection or this.pendingConnectionHandle) before handleConnected runs, and ensure handleConnected/handleClose update/replace that stored handle when the connection fully succeeds or closes so close() can call connection.connection?.close() to cancel pending connects.
🤖 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 779-782: The on_connect_error method is incorrectly collapsing all
socket/connect failures into AnyPostgresError::ConnectionRefused; update it to
preserve the original connect failure semantics by using
AnyPostgresError::ConnectionFailed (or explicitly propagate the real socket
error) inside on_connect_error when calling handle_socket_failure -> this.fail,
unless you also plumb the real OS/socket error through and map only true
ECONNREFUSED cases to ConnectionRefused; specifically modify
PostgresSQLConnection::on_connect_error (and where the connect error is dropped)
to either pass the actual error through to fail or change the enum variant used
to ConnectionFailed to avoid misclassifying DNS/timeouts/unix-socket-missing
errors.
---
Outside diff comments:
In `@src/js/internal/sql/mysql.ts`:
- Around line 130-137: The current startConnection calls
createPooledConnectionHandle but drops its returned native handle so
BaseSQLAdapter.close cannot abort an in-flight MySQL dial; modify
startConnection (and related logic) to capture and preserve the handle returned
by createPooledConnectionHandle (call to createMySQLConnection) immediately
(e.g., assign to this.connection or this.pendingConnectionHandle) before
handleConnected runs, and ensure handleConnected/handleClose update/replace that
stored handle when the connection fully succeeds or closes so close() can call
connection.connection?.close() to cancel pending connects.
🪄 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: b490ee98-cb95-4fca-8dba-d084111a2784
📒 Files selected for processing (5)
src/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/shared.tssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/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
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)
130-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the native handle during pending MySQL connects.
BaseSQLAdapter.#close()now treatspendingconnections as abortable by callingconnection.connection?.close()before awaitingonFinish(src/js/internal/sql/shared.ts, Lines 1164-1177). This path drops the handle returned bycreatePooledConnectionHandle()and only assignsthis.connectionafterhandleConnected()succeeds, soclose()cannot cancel an in-flight MySQL dial/handshake and can stall until the native connect timeout fires.Suggested fix
- protected startConnection() { - createPooledConnectionHandle( + protected async startConnection() { + this.connection = await createPooledConnectionHandle( createMySQLConnection, this.connectionInfo, this.handleConnected.bind(this), this.handleClose.bind(this), true, ); }🤖 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 130 - 137, The current startConnection calls createPooledConnectionHandle but drops its returned native handle so BaseSQLAdapter.close cannot abort an in-flight MySQL dial; modify startConnection (and related logic) to capture and preserve the handle returned by createPooledConnectionHandle (call to createMySQLConnection) immediately (e.g., assign to this.connection or this.pendingConnectionHandle) before handleConnected runs, and ensure handleConnected/handleClose update/replace that stored handle when the connection fully succeeds or closes so close() can call connection.connection?.close() to cancel pending connects.
🤖 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 779-782: The on_connect_error method is incorrectly collapsing all
socket/connect failures into AnyPostgresError::ConnectionRefused; update it to
preserve the original connect failure semantics by using
AnyPostgresError::ConnectionFailed (or explicitly propagate the real socket
error) inside on_connect_error when calling handle_socket_failure -> this.fail,
unless you also plumb the real OS/socket error through and map only true
ECONNREFUSED cases to ConnectionRefused; specifically modify
PostgresSQLConnection::on_connect_error (and where the connect error is dropped)
to either pass the actual error through to fail or change the enum variant used
to ConnectionFailed to avoid misclassifying DNS/timeouts/unix-socket-missing
errors.
---
Outside diff comments:
In `@src/js/internal/sql/mysql.ts`:
- Around line 130-137: The current startConnection calls
createPooledConnectionHandle but drops its returned native handle so
BaseSQLAdapter.close cannot abort an in-flight MySQL dial; modify
startConnection (and related logic) to capture and preserve the handle returned
by createPooledConnectionHandle (call to createMySQLConnection) immediately
(e.g., assign to this.connection or this.pendingConnectionHandle) before
handleConnected runs, and ensure handleConnected/handleClose update/replace that
stored handle when the connection fully succeeds or closes so close() can call
connection.connection?.close() to cancel pending connects.
🪄 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: b490ee98-cb95-4fca-8dba-d084111a2784
📒 Files selected for processing (5)
src/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/shared.tssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/postgres/PostgresSQLConnection.rs
🛑 Comments failed to post (1)
src/sql_jsc/postgres/PostgresSQLConnection.rs (1)
779-782:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't collapse every connect-time failure into
ConnectionRefused.Line 781 now reports a specific refusal code, but Line 1352 drops the actual socket error before this method runs. That means DNS failures, timeouts, unreachable hosts, and missing Unix sockets will all be surfaced as
ConnectionRefused, which is a behavior change and the wrong JS-visible error contract for many real failures. KeepConnectionFailedhere unless you also plumb the real connect error through and map only true refusal cases.Suggested minimal fix
- this.fail(b"Failed to connect", AnyPostgresError::ConnectionRefused); + this.fail(b"Failed to connect", AnyPostgresError::ConnectionFailed);As per coding guidelines, "Platform-specific code: never assume OS/ABI facts are portable - validate errno meanings..."
🤖 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 779 - 782, The on_connect_error method is incorrectly collapsing all socket/connect failures into AnyPostgresError::ConnectionRefused; update it to preserve the original connect failure semantics by using AnyPostgresError::ConnectionFailed (or explicitly propagate the real socket error) inside on_connect_error when calling handle_socket_failure -> this.fail, unless you also plumb the real OS/socket error through and map only true ECONNREFUSED cases to ConnectionRefused; specifically modify PostgresSQLConnection::on_connect_error (and where the connect error is dropped) to either pass the actual error through to fail or change the enum variant used to ConnectionFailed to avoid misclassifying DNS/timeouts/unix-socket-missing errors.Source: Coding guidelines
|
Checked both findings from the latest review against main (8df5916). Both describe code that is identical on main; neither is a change made by this PR.
mysql |
…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.
2 participants
What this does
Consolidates copy-pasted logic across the SQL drivers: the JS adapters' shared pool/connection/query plumbing moves into
src/js/internal/sql/shared.ts; the Rust side gainssrc/sql/shared/(StackReader, QueryStatus, StatementStatus) andsrc/sql_jsc/shared/(connection/query ctor args, SQLDataCell) replacing per-driver copies. Net −3.7k lines.Split from #31912 (whole-repo simplification pass; closing that PR in favor of module-scoped splits). This PR only moves and removes code — zero intended behavior change. Verified there by a per-file behavioral-equivalence audit and full CI (green on build 61383); verified here by a standalone full-workspace compile check.
Verification (adoption)
Since this is a behavior-preserving refactor, there is no input that fails before and passes after; the verification is equivalence coverage:
NoticeResponseand degenerate empty-notice cases to the mock-server framing tests intest/js/sql/postgres-multi-statement-fields.test.ts. These drive the one protocol path this PR rewires (NoticeResponseis now a type alias ofErrorResponsedecoded viadecode_notice_internal) and had no prior coverage anywhere intest/js/sql. They pass on this branch and on the released bun.test/js/sqlon a debug ASAN build: 507 pass; the only failures are 2 sqlite fuzz tests that exceed their 5s timeout identically on a main debug build.Merge with main (#32028 connect-failure retry)
Main landed #32028 while this PR was open, adding the same ~100 mirrored lines of connect-retry logic to both
postgres.tsandmysql.ts, inside the exact region this PR deduplicates. The merge commit 47c520c resolves the conflict by hosting that machinery once inBasePooledConnectioninshared.ts(connect-cycle budget fields, backoff scheduling inhandleClose,#finishClose,cancelRetry, plus the two adapter hunks: the queue-drain completion check inrelease()and retry cancellation in#close()). The driver-specific piece, which error code marks a retryable connect failure, becomes an abstractisConnectFailureErrorhook implemented per driver, mirroring the existingisNonRetryableErrorpattern.Verified on the merged branch: all 15 tests in
test/js/sql/sql-connect-error-reporting.test.ts(the #32028 suite, mock TCP servers covering retry-while-waiting, retry cancellation during graceful close, onclose-once, andconnectionTimeout: 0) pass; fulltest/js/sqlis 518 pass with the same 2 pre-existing sqlite debug-build timeouts as before the merge.