sql: drain queries awaited in the same tick as close()/end() - #33740
sql: drain queries awaited in the same tick as close()/end()#33740robobun wants to merge 7 commits into
Conversation
Query.then()/catch()/finally() deferred the pool hand-off by one microtask
via `await Promise.$resolve()`, so a close()/end() in the same synchronous
block ran first, saw hasPendingQueries() == false, and rejected the
already-awaited query with ERR_*_CONNECTION_CLOSED without ever sending it.
q.execute() (which enqueues synchronously) did not have this problem.
Query.then() now hands the query to the pool synchronously, the same way
execute() does; the SQLQueryStatus.executed flag already guards re-entry
from the then()/finally() the pool itself calls during bindQuery.
This exposed a latent truthiness bug in BaseSQLAdapter.close(): the
documented `{ timeout: 0 }` force-close went through the graceful-wait
branch because `if (timeout)` is false for 0. It happened to reject
same-tick queries only because nothing was enqueued yet. With queries
now enqueued synchronously that no longer holds, so close() checks
`timeout != null` to reach the existing `timeout === 0` fast path.
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Updated 6:36 AM PT - Aug 13th, 2026
✅ @robobun, your commit 81872d3a062479abf6cf529940f207100215555c passed in 🧪 To try this PR locally: bunx bun-pr 33740That installs a local version of the PR into your bun-33740 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
The #32039 has a dedicated mock-server test for the |
…ug builds The 10000-iteration loop in "properly finalizes prepared statements" takes ~11s on debug+ASAN builds and times out under the default 5s budget (also on main). Use 1000 iterations under isDebug, matching the pattern in test/js/web/html/FormData.test.ts and elsewhere.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The body was byte-identical after dropping the microtask yield.
There was a problem hiding this comment.
Thanks for collapsing #runAsync() into a #run() delegate — that addresses my earlier nit. I'm deferring the final call to a human since removing the microtask deferral changes when the pool handler runs relative to .then() callers, which is subtle enough to warrant a second pair of eyes.
Extended reasoning...
Overview
Two source changes plus tests across three adapters:
src/js/internal/sql/query.ts:#runAsync()no longer doesawait Promise.$resolve()before invoking the pool handler; it now delegates directly to#run(), so.then()/.catch()/.finally()enqueue the query synchronously (matching.execute()).src/js/internal/sql/shared.ts:close()now checkstimeout != nullinstead of truthytimeout, so{ timeout: 0 }reaches the documented force-close branch.- New tests in
sql.test.ts,sql-mysql.test.ts,sqlite-sql.test.ts, plus an unrelated debug-build iteration shrink insqlite-sql.test.ts.
My earlier inline nit (the #run()/#runAsync() duplication) was addressed in 062e3cf and the thread is resolved.
Security risks
None. No auth, crypto, or untrusted-input parsing is touched; the timeout != null change only affects which branch of an already-validated close path is taken.
Level of scrutiny
Medium. The line count is tiny, but dropping the microtask deferral is a timing/ordering change inside a lazy-Promise subclass's .then() path that fans out to every SQL adapter's pool handler. The original await Promise.$resolve() may have been incidental, or it may have been load-bearing for some ordering assumption (e.g., letting .then() return before the handler can synchronously re-enter or call back into user code). The PR description argues re-entry is guarded by the executed status flag and the existing test suite passes, which is reassuring, but this is exactly the kind of change where a maintainer familiar with the pool's history should confirm nothing else depended on the deferral.
Other factors
- Overlaps with #32039 on the
timeout != nullfix; the author has already noted either can land first. - Tests are well-placed next to the existing
.execute()cases and the SQLite one runs without a container. - No bugs surfaced by the automated review.
There was a problem hiding this comment.
No issues found and the dedup nit is addressed, but removing the microtask deferral from Query.then() changes execution timing on the hot path for every SQL adapter — worth a human sanity-check on re-entrancy (e.g. bindQuery's query.finally() now recursing synchronously) before landing.
Extended reasoning...
Overview
Two source changes plus tests:
src/js/internal/sql/query.ts:#runAsync()now delegates directly to#run(), dropping theawait Promise.$resolve()that previously deferred the pool hand-off by one microtask. This means.then()/.catch()/.finally()on aQuerynow invoke the pool handler synchronously before returning, matching.execute().src/js/internal/sql/shared.ts:BaseSQLAdapter.close()gates ontimeout != nullinstead ofif (timeout), so the documented{ timeout: 0 }force-close reaches the fast-close branch.- Tests added across SQLite / Postgres / MySQL for the same-tick-close case, plus a debug-build iteration reduction in
sqlite-sql.test.ts.
My earlier inline nit (dedupe #runAsync/#run) was addressed in 062e3cf.
Security risks
None. No auth, crypto, or untrusted-input parsing is touched; the timeout != null change only affects control flow inside an already-validated numeric option.
Level of scrutiny
Moderate–high. The shared.ts one-liner is trivially correct. The query.ts change, however, alters observable timing on the single code path every awaited SQL query flows through, across all three adapters. Removing a deliberate-looking microtask hop from a Promise subclass's .then() is the kind of change where second-order effects (re-entrancy, ordering assumptions in the pool) are easy to miss:
bindQuery()callsquery.finally(...), which now synchronously re-enters#runAsyncAndCatch()→#run()while the outerhandler(this, handle)call is still on the stack. TheSQLQueryStatus.executedguard makes this a no-op, and the PR description calls this out — but it's exactly the sort of invariant a maintainer familiar with the pool should confirm.- Any caller that relied (intentionally or not) on the query being enqueued after the current microtask now sees it enqueued before.
Other factors
- Overlaps with open PR #32039 on the
timeout != nullchange; the author has explained the relationship in-thread. - The PR description is thorough, fail-before is verified, and tests cover the fix across every adapter — the diligence is high.
- No CODEOWNERS entry covers these files.
Given the subtlety of the timing change on a shared hot path, deferring to a human reviewer rather than auto-approving.
|
CI status: the This diff touches |
There was a problem hiding this comment.
Thanks for addressing the dedup nit. I'm deferring to a human here since removing the microtask deferral changes dispatch timing for every await sql call across all three adapters — the reasoning looks sound, but this and the overlap with #32039 are worth a maintainer's eyes.
Extended reasoning...
Overview
Two source changes plus tests:
src/js/internal/sql/query.ts:#runAsync()no longer doesawait Promise.$resolve()before invoking the pool handler; it now delegates directly to#run(), so.then()/.catch()/.finally()enqueue the query synchronously (same as.execute()).src/js/internal/sql/shared.ts:BaseSQLAdapter.close()gates ontimeout != nullinstead ofif (timeout), so{ timeout: 0 }reaches the documented force-close branch.- New tests in
sql.test.ts,sql-mysql.test.ts,sqlite-sql.test.ts, plus a debug-build iteration shrink insqlite-sql.test.ts.
My earlier inline nit (dedup #runAsync with #run) was addressed in 062e3cf and the thread is resolved.
Security risks
None. No untrusted input parsing, auth, crypto, or filesystem paths are touched. The timeout != null change only affects which internal close branch is taken.
Level of scrutiny
Moderate–high. The diff is small and the root-cause analysis is thorough, but the query.ts change alters microtask ordering for the hot path shared by every await sql\...`across Postgres/MySQL/SQLite. The handler now runs *synchronously inside*.then(), which means pool.connect()→onConnected→handle.run()can all fire beforesuper.$thenis called. Re-entry viabindQuery's query.finally()is guarded bySQLQueryStatus.executed, and the PR description explains why the shared.tschange is required to keep the existingConnection destroyed with query before` tests correct — but timing changes in shared promise machinery are exactly where subtle regressions hide, so a maintainer familiar with the pool lifecycle should confirm.
Other factors
- Overlaps with open PR #32039 (same
timeout != nullfix, with a dedicated mock-server test this PR doesn't duplicate). A human should decide sequencing. - Bug-hunting system found nothing; tests are added alongside existing coverage in the right files and the author verified fail-before with
USE_SYSTEM_BUN=1. - CI build #70449 was still in progress at last update.
…sql-close-drains-same-tick-queries
…query in flight (from #32039)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Enqueue synchronously (same as execute()) so a same-tick close() sees | ||
| // the query as pending; #run()'s executed-status guard handles re-entry. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. The two source changes are small and well-reasoned, but removing the microtask deferral in Query.#runAsync() changes when the pool handler runs relative to user code calling .then(), so a human sign-off on the ordering change would be worthwhile. There's also an open comment-cop flag on the two-line comment at query.ts:131-132 that a maintainer should adjudicate (it reads as a false positive to me — the comment explains a non-obvious choice, not a workaround).
What was reviewed
#runAsync()now delegates to#run(); re-entry viabindQuery'squery.finally()is guarded by theexecutedstatus flag set before the handler runs.close({ timeout }):timeout != nullcorrectly routes0to the force-close branch and still treatsnull/undefinedas drain-with-no-timer; the new mock-server tests cover both.#runAsyncAndCatch()'srunPromise !== thisguard still holds — the async wrapper returns a fresh promise even when#run()returnsthis.
Extended reasoning...
Overview
Two source changes in src/js/internal/sql/: (1) Query.#runAsync() drops await Promise.$resolve() and now delegates to #run(), so .then()/.catch()/.finally() hand the query to the pool synchronously (matching .execute()); (2) BaseSQLAdapter.close() gates the timeout branch on timeout != null instead of truthiness, so the documented { timeout: 0 } reaches the force-close path. Tests are added for SQLite, Postgres, and MySQL, plus mock-server coverage in sql-close-pending-connection.test.ts for both { timeout: 0 } and { timeout: null } with a query in flight.
Security risks
None. No untrusted input parsing, no auth/crypto. The changes are internal query-lifecycle ordering and an option-presence check.
Level of scrutiny
Medium-high. The timeout != null fix is a textbook falsy-zero bug and low risk. Removing the microtask deferral is a small diff but a real behavioral change: the pool handler (which can call connection.bindQuery, which calls query.finally() back on the same query) now runs synchronously inside the user's .then() call. The re-entrancy is handled by the SQLQueryStatus.executed bit set before the handler is invoked, and .execute() has always taken this synchronous path, so the code path itself is proven — but the change in when it runs relative to surrounding user code is the kind of thing that can surface subtle ordering bugs that unit tests don't catch. That warrants a human look.
Other factors
- My earlier nit (dedupe
#runAsyncwith#run) was addressed in 062e3cf; that thread is resolved. - The async wrapper still returns a fresh promise (never
this), so#runAsyncAndCatch()'srunPromise !== thischeck and its.catch(() => {})unhandled-rejection guard are unchanged in effect. - comment-cop flagged the two-line comment on
#runAsync()today; it's short and explains why anasyncmethod enqueues synchronously, which seems like exactly the non-obvious decision worth a comment. A maintainer should decide whether to trim it or dismiss the flag. - The
isDebug ? 1000 : 10000iteration reduction insqlite-sql.test.tsaddresses a pre-existing debug-ASAN timeout the PR description mentions; unrelated to the fix but reasonable.
…sql-close-drains-same-tick-queries main landed the same isDebug iterations tweak in sqlite-sql.test.ts (#37109), so that hunk is now upstream.
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Because the core change removes the microtask deferral in Query#runAsync() — shifting when every awaited query is handed to the pool from one-microtask-later to synchronously inside .then() — a human look at the timing/re-entrancy implications would still be worthwhile.
What was reviewed:
- Re-entry from
bindQuery'squery.finally()during the now-synchronous handler call — guarded by theSQLQueryStatus.executedmask in#run(). #runAsyncAndCatch()'srunPromise !== thisguard — still holds because the async wrapper returns a fresh promise even when#run()returnsthis.close({ timeout: 0 })vsclose({ timeout: null })after thetimeout != nullgate — both paths covered by the new mock-server tests; importedwire-frames.tshelpers all exist.
Extended reasoning...
Overview
Two source changes: src/js/internal/sql/query.ts collapses #runAsync() into a one-line async delegator to #run(), dropping the await Promise.$resolve() that deferred the pool hand-off by one microtask; src/js/internal/sql/shared.ts changes if (timeout) to if (timeout != null) in BaseSQLAdapter.close() so { timeout: 0 } reaches the documented force-close branch. Four test files add coverage across SQLite, Postgres, MySQL, and mock wire servers.
Security risks
None identified. No parsing of untrusted input, no auth/TLS/crypto surface, no new external I/O beyond localhost mock servers in tests.
Level of scrutiny
Moderate-to-high. The shared.ts change is a clear falsy-zero fix, but the query.ts change alters execution ordering for every Bun.SQL query awaited via .then()/.catch()/.finally() across all three adapters. The removed await Promise.$resolve() was a deliberate deferral; the PR argues convincingly that the executed status flag makes synchronous dispatch safe, and I traced the bindQuery → query.finally() → #run() re-entry to confirm it short-circuits, but this is exactly the kind of microtask-ordering change where an unforeseen caller could be relying on the old timing.
Other factors
- My earlier duplication nit was addressed (062e3cf).
- There is a fresh unresolved comment-cop inline on
query.ts:132; it looks like a false positive on a two-line explanatory comment, but the author hasn't responded to it yet. - Test coverage is thorough (fail-before verified, all three drivers, mock-server force-close and drain cases), which raises confidence — but does not make this a mechanical change.
1 participant
Problem
A graceful
sql.close()/sql.end()is documented to wait for pending queries, but a query created and awaited in the same synchronous block as theclose()call is rejected withERR_*_CONNECTION_CLOSEDand never sent to the server, even with an explicit grace period (close({ timeout: 5 })).The same program with
q.execute()beforeend(), or withend()one macrotask later, drains correctly. The contract depends on an internal microtask race the caller cannot observe.Cause
Query.then()/catch()/finally()route through#runAsync(), which didawait Promise.$resolve()before calling the pool handler. So the pool enqueue landed one microtask after.then()returned. A same-tickpool.close()runs first, sampleshasPendingQueries()(waiting queue empty,totalQueries0), takes the close-immediately branch, and setsclosed = true. The deferred enqueue then hitsconnect(): if (this.closed) return onConnected(connectionClosedError(), null).Query.execute()calls#run(), which invokes the handler synchronously, soclose()sees the query as pending. That is why lane D worked.Fix
src/js/internal/sql/query.ts: drop theawait Promise.$resolve()from#runAsync()so.then()/.catch()/.finally()hand the query to the pool synchronously, the same way.execute()does. Re-entry (frombindQuery'squery.finally()and similar) is already guarded by theSQLQueryStatus.executedflag set immediately above.src/js/internal/sql/shared.ts:BaseSQLAdapter.close()checkedif (timeout), so the documented{ timeout: 0 }force-close fell through to the graceful-wait branch; it only rejected same-tick queries because nothing was enqueued yet. With queries now enqueued synchronously that accidental cover is gone, so checktimeout != nullto reach the existingtimeout === 0fast-close path. This keeps theConnection destroyed with query beforetests passing for the documented reason.Tests
test/js/sql/sqlite-sql.test.ts:close() drains a query awaited in the same tick(SQLite, no container).test/js/sql/sql.test.ts:Connection end does not cancel a query awaited in the same tickand a{ timeout: 5 }variant, next to the existing.execute()case.test/js/sql/sql-mysql.test.ts: matching MySQL case.test/js/sql/sql-close-pending-connection.test.ts:close({ timeout: 0 })force-closes with a query in flight, andclose({ timeout: null })still waits for it, for postgres and mysql against mock servers (thetimeout: 0cases hang without theshared.tschange). These are the cases from sql: make close({ timeout: 0 }) force-close immediately with queries in flight #32039, which made the sameshared.tschange on its own and is closed in favour of this PR.Fail-before verified with
USE_SYSTEM_BUN=1and withsrc/stashed under the debug build:All existing non-container
test/js/sqlsuites pass with the fix (the one pre-existing debug-ASAN timeout inproperly finalizes prepared statementsalso reproduces onmain).Fixes #32038