sql: consolidate JS pool/connection plumbing into BasePooledConnection in shared.ts#32145
Conversation
The cherry-pick from #31994 clobbered two pool-lifecycle fixes that landed on main since its merge-base. This re-hosts both in the new shared class structure: - #32041: try/finally around the user onconnect/onclose callbacks in handleConnected and #finishClose so a throwing callback never skips pool bookkeeping; createPooledConnectionHandle catch always defers via process.nextTick (drops the per-driver deferSyncCloseError flag, both drivers now match). - #32097: startConnection is abstract Promise<void> and both subclasses await + assign the native handle at creation; #beginConnecting awaits it and closes the handle if the pool was force-closed in the microtask window before it materialized.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds shared SQL normalization and pooled-connection base classes, refactors MySQL/Postgres/SQLite adapters to extend those bases and delegate normalization/binding, and adds tests for forced-close during a mid-handshake. ChangesSQL adapter and pooling infrastructure consolidation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 12:38 AM PT - Jun 12th, 2026
✅ @autofix-ci[bot], your commit 60eb740931bcf385a155b794d4bbee0bdfec2721 passed in 🧪 To try this PR locally: bunx bun-pr 32145That installs a local version of the PR into your bun-32145 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@robobun adopt |
|
@claude review |
|
Adopted. Review swept: all bot findings are pre-existing behaviors this refactor preserves, verified against the released build and tracked in #32035 and #32155; threads resolved. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/sql/mysql.ts`:
- Around line 282-284: The isUpsertUpdate check currently only matches
exact-case suffix; update isUpsertUpdate to perform a case-insensitive suffix
check (e.g., normalize the trimmed query to a consistent case or use a
case-insensitive regex) so "on duplicate key update" in any letter case is
detected and trailing whitespace is handled; keep the method name isUpsertUpdate
and ensure it still returns a boolean.
In `@src/js/internal/sql/shared.ts`:
- Around line 472-545: The IN and UPDATE branches currently dereference array
entries without guarding for empty arrays or non-object/null entries; add checks
to validate inputs before indexing: in the SQLCommand.in branch (use symbols:
SQLCommand.in, items, itemsCount, columns, columns[0], binding_values) reject
empty arrays (itemsCount === 0) and when columnCount > 0 ensure each items[j] is
a non-null object before using value[columns[0]] — throw a SyntaxError with a
clear message if not. In the UPDATE branch (use symbols: SQLCommand.update,
$isArray(items), item, columns, item[column], adapter.throwIfUpdateEmpty) reject
empty arrays up front (if $isArray(items) && items.length === 0) and validate
that the resolved item is a non-null object before accessing its properties; if
invalid, throw a SyntaxError instead of letting a TypeError occur. Ensure these
guards run before any property access and keep existing behavior for valid
inputs.
In `@src/js/internal/sql/sqlite.ts`:
- Around line 411-414: The update empty-check in throwIfUpdateEmpty currently
ignores the computed flag and only tests query.endsWith("SET "); change it to
use the provided _hasValues parameter (rename or use hasValues) and throw the
SyntaxError when that flag is false (e.g., if (!hasValues) throw new
SyntaxError("Update needs to have at least one column")); keep or remove the
endsWith fallback as you prefer but ensure throwIfUpdateEmpty uses the hasValues
signal (function: throwIfUpdateEmpty, param: _hasValues).
🪄 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: b7a39ae1-f993-493d-911f-57b057e88e96
📒 Files selected for processing (4)
src/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/shared.tssrc/js/internal/sql/sqlite.ts
The re-integration commit hosts the #32041 try/finally and the #32097 forced-close settlement in one BasePooledConnection close handler, so pin their interaction per driver: onclose throws while the pool is force-closed mid-handshake, and close() must still resolve and the pending query must still reject.
There was a problem hiding this comment.
No new issues found on re-review — the only finding (MySQL upsert case-sensitivity) is pre-existing and tracked in #32035. That said, this is a ~1000-line restructuring of the SQL connection-pool lifecycle across all three drivers plus a re-hosting of two recent fixes, so it warrants a human pass before merge.
Extended reasoning...
Overview
This PR consolidates the duplicated pool/connection/query plumbing from postgres.ts, mysql.ts, and sqlite.ts into shared.ts as an abstract BasePooledConnection / BaseSQLAdapter hierarchy, plus a shared normalizeQuery with adapter hooks (placeholder, bindParam, getHelperCommand, isUpsertUpdate, throwIfUpdateEmpty). It also re-integrates two pool-lifecycle fixes (#32041 try/finally around user onconnect/onclose; #32097 forced-close-during-handshake) into the new structure. Net −995 lines across 4 files.
Security risks
None introduced. The injection guards (validateTransactionOptions, validateDistributedTransactionName) were moved verbatim to the base class; escapeIdentifier stays per-driver; checkUnsafeTransaction preserves the BEGIN/START TRANSACTION guard. createPooledConnectionHandle now passes an extra trailing allowPublicKeyRetrieval arg to the postgres native ctor, which ignores it — benign.
Level of scrutiny
High. This is the connection-pool state machine for Bun.sql — retry/backoff, force-close races, query distribution, graceful shutdown — restructured into an inheritance hierarchy, with two recent bug fixes re-hosted into it. The verification is thorough (lifecycle tests pass, stash test confirms the re-integration is load-bearing, full test/js/sql/ run with failures triaged as pre-existing), but the scope and criticality put it well outside what a bot should approve unilaterally.
Other factors
- My earlier inline comment (the
isUpsertUpdatecase-sensitivity mismatch) was confirmed pre-existing, already tracked in #32035, and the thread is resolved. - The
#close()switch now merges thepending/connectedcases and callscancelRetry()unconditionally; forconnectedit's a no-op (no retry timer), so behavior is preserved — but this is the kind of subtle merge a human should glance at. - The shared
detectCommandnow takes ananyAndAllMeanInflag to preserve the prior MySQL-vs-Postgres divergence on leadingANY/ALLtokens. - SQLite keeps its own
throwIfUpdateEmpty(string-basedendsWith("SET ")) to preserve its prior, slightly different empty-update detection.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/sql/sql-onconnect-onclose-throw.test.ts`:
- Line 244: The test calls sql.close with a string timeout ("0") which
contradicts the expected timeout?: number; update the call to pass a numeric
literal instead (use timeout: 0) in the sql.close invocation in
sql-onconnect-onclose-throw.test.ts so the argument matches the declared
signature of sql.close and avoids relying on implicit coercion.
🪄 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: 40e694aa-7bef-438c-8324-9c87d554b15c
📒 Files selected for processing (1)
test/js/sql/sql-onconnect-onclose-throw.test.ts
There was a problem hiding this comment.
No new issues found — the consolidation looks faithful and the re-hosted #32041/#32097 fixes are covered by the new forced-close tests — but this is a ~1000-line refactor of the pool/connection state machine across all three SQL adapters, so it's worth a human pass (cirospaciari is the suggested reviewer).
Extended reasoning...
Overview
This PR consolidates ~1000 lines of duplicated pool/connection/query-normalization plumbing from src/js/internal/sql/{postgres,mysql,sqlite}.ts into shared.ts as an abstract BasePooledConnection / BaseSQLAdapter hierarchy. Each driver now subclasses the base and overrides only driver-specific hooks (startConnection, wrapError, isNonRetryableError, isConnectFailureError, placeholder, bindParam, getHelperCommand, isUpsertUpdate, throwIfUpdateEmpty). Two recent main-line fixes — #32041 (try/finally around throwing onconnect/onclose) and #32097 (forced close() mid-handshake) — are re-hosted in the shared base, and new per-driver tests cover the point where the two fixes intersect.
Security risks
None introduced. The security-relevant validators (validateTransactionOptions, validateDistributedTransactionName, escapeIdentifier, the getArrayType regex) are moved verbatim or left in place. createPooledConnectionHandle now passes a trailing allowPublicKeyRetrieval argument to the Postgres native constructor as well; per the Zig signatures this extra positional arg is ignored, so it doesn't change TLS/auth behavior.
Level of scrutiny
High. This is the core connection-pool state machine for Bun.SQL — lifecycle, retry/backoff, reservation, graceful/forced close — shared across three production drivers. The refactor is intended to be behavior-preserving, but it (a) collapses two per-driver #close switch arms into one with a cancelRetry() check now also reachable from the connected state, (b) hoists the post-startConnection forced-close check from the per-driver #startConnection into the shared #beginConnecting, and (c) re-expresses the per-driver retry switch as an isNonRetryableError predicate. Each of these is plausible-equivalent on inspection, and the load-bearing stash test in the PR body demonstrates the re-integration commit is required, but the surface area is large enough that a domain owner should sign off.
Other factors
- All inline review threads (mine on
isUpsertUpdate, CodeRabbit's on helper guards /throwIfUpdateEmpty/timeout: "0") are resolved as pre-existing behaviors tracked in #32035, #32155, #32091. - The bug-hunting system found nothing.
- The robobun CI comment shows
build-rustfailures on 39ccfdc across several platforms; those are inscripts/build/ci.ts(untouched by this PR) and the latest push (60eb740, autofix) hasn't reported yet, so worth confirming green before merge. - The
#closeswitch consolidation now callscancelRetry()forconnectedconnections too — harmless sinceretryTimeris alwaysnullonce connected, but it's the kind of small structural shift a maintainer should eyeball.
…ave orphans (#32207) When a test that does `await using proc = Bun.spawn(...)` times out before the using-scope exits, the dispose never runs and the subprocess survives indefinitely. The sql fixture subprocesses connect to a (mock or real) database and await the pool draining; under a regression that hangs the pool (e.g. while iterating on #32145), the fixture never exits, the test times out, and the fixture is orphaned. Locally observed 16 such orphans pinning the machine at load ~100 for hours after a `bun bd test test/js/sql/` run that exercised a buggy intermediate commit. Adding `timeout` to the `Bun.spawn` options gives the child its own hard deadline independent of the test runner, so an abandoned fixture self-kills within a minute even when the parent test has already moved on. `sql.test.ts` already had `timeout` on all three of its spawns; this brings the other 8 files in line.
Rebuilt on main after the pool plumbing was consolidated into BaseSQLAdapter/BasePooledConnection in shared.ts (#32145) — the fix now lands once in the shared base class instead of twice in the postgres/mysql adapters. Previously `new Bun.SQL({ max: N })` opened all N backend connections on the first query: `connect()` instantiated every pool slot eagerly, and each pooled-connection constructor dials TCP immediately. A single `SELECT 1` on a `max: 500` pool burst 500 real connections. The fix, ported from the pre-rebase branch: - Pool starts empty; `#tryGrowPool()` appends one connection at a time, capped at `maxPoolSize` (= `options.max`). - First `connect()` opens exactly one connection. - `flushConcurrentQueries()` grows the pool when queued queries can't be served by ready or pending slots (discounting pending slots already earmarked for `reservedQueue`). - `maxDistribution()` divides by the pool ceiling rather than the current size. - `connect()`'s retry branch grows in parallel while waiting behind in-flight slots, and for non-retryable auth failures fails fast with the cached error — unless `password` is a function (dynamic credential), in which case it grows or force-retries a closed slot via the new `forceRetry()` so a rotated token can take effect even at `max: 1`. Tests (test/js/sql/sql-pool-lazy-growth.test.ts, 4 of 5 fail on unfixed main; the sync-password-throw guard passes since main now defers createConnection failures via process.nextTick): - single query opens 1 socket, not max (postgres + mysql) - static-password auth failure fast-fails without new sockets - function-password retries auth per query at max: 1 - synchronous password() throw still rejects both queries
2 participants
What this does
Final slice from #31994 (closed as too big), on top of #32128, #32135, #32141. Consolidates the JS adapters' duplicated pool/connection/query plumbing from
src/js/internal/sql/{postgres,mysql,sqlite}.tsintosrc/js/internal/sql/shared.tsas aBasePooledConnectionclass hierarchy. Net −995 lines across 4 files.Two commits:
src/js/internal/sql/fromorigin/claude/split/sqlas-is. This intentionally clobbers two pool-lifecycle fixes that landed onmainsince Deduplicate SQL driver internals across postgres/mysql/sqlite #31994's merge-base: sql: don't let a throwing onconnect/onclose callback corrupt the connection pool #32041 (throwing onconnect/onclose corrupting the pool) and sql: make forced close() resolve while a pool connection is mid-handshake #32097 (forcedclose()resolving mid-handshake).BasePooledConnection:onconnect/onclosecalls inhandleConnectedand#finishCloseso a throwing callback never skips pool bookkeeping; thecreatePooledConnectionHandlecatch always defers viaprocess.nextTick(drops the per-driverdeferSyncCloseErrorflag — both drivers now match).startConnection()isabstract Promise<void>and both subclasses await + assignthis.connectionat creation;#beginConnectingawaits it and closes the handle ifonFinishwas set (pool force-closed) in the microtask window before it materialized.Behavioral equivalence
The original
BasePooledConnectionextraction in #31994 carried a per-file behavioral-equivalence audit against its merge-base (placeholders, escaping, helper commands, error message text, pool/transaction semantics all preserved) and was green on full CI build 61383. This PR re-applies that diff and adds the two missing fixes; the load-bearing verification is below.Verification
bun bd: builds clean.sql-onconnect-onclose-throw.test.ts,sql-close-pending-connection.test.ts,sql-connect-error-reporting.test.ts): 26 pass / 0 fail.sql-onconnect-onclose-throw.test.tsgains a forced-close variant per driver (a throwingonclosewhile the pool is force-closed mid-handshake, against a fakenetserver). The two re-hosted fixes meet inBasePooledConnection's close handler, and that interaction had no coverage. Note these pass againstorigin/main'ssrc/js/internal/sqltoo (verified): both underlying fixes already live on main per-driver, so versus main this PR is equivalence-preserving by design and no test can fail on one side only. The HEAD~1 stash test below is the proof that the re-integration commit is load-bearing.mysql: forced close() resolves when called before the native handle is storedtimeout;postgres: pool calls from onclose are safe when connecting fails synchronously→reentry threw: TypeError; both drivers'throwing onclose still rejects pending queries on connect refusedtimeouts). With the re-integration: 26 pass. The diff is required for both fixes.test/js/sql/against livepostgres_plain/mysql_plain/*_tlscontainers: 1526 pass / 7 fail / 6 errors locally. Every failure reproduces onorigin/mainin the same environment — see Notes below.Notes on local-only test failures (none introduced by this PR)
sql-mysql-query-string-leak.test.tsfails locally on a debug+ASAN build for bothorigin/main(314.6 MiB) and this branch (317.2 MiB), within noise of each other; both exceed the 256 MiB ASAN threshold. AheapStats()probe of the same workload showsMySQLQuery 2→1after GC — wrappers are finalized correctly. The test passes onmain's CI (release build); the threshold is tuned for release RSS overhead, not local debug.sql.test.ts > query string memory leak test(postgres): same category.sqlite-sql.test.ts > Query Normalization Fuzzing Tests > handles exotic but valid SQL patterns: 5.5s timeout on debug builds — pre-existing per Deduplicate SQL driver internals across postgres/mysql/sqlite #31994's original verification notes.describeWithContainercold-start races:docker compose up -d --waitreturns non-zero on an already-healthy container (test/docker/index.ts:157), causing beforeEach hook timeouts insql-mysql.test.ts(TLS),sql-mysql-bind-oob,sql-mysql.helpers,sql-mysql.auth. Pre-existing harness flake; clears on warm rerun.The Rust-side changes from #32097 (
src/sql_jsc/) were already onmainbefore this PR — only the JS hooks needed re-integrating.