Repository navigation
Conversation
The pool and a transaction each called query.finally() on every query that they run. finally() goes through then() of Query, which starts an async function and makes closures and promises that nothing needs. onQuerySettled() adds one reaction with the private then(). The reaction is added at the same point, so the order of the callback and of the reactions of the caller does not change.
|
Status: ready for review. CI is green on c29573e (Buildkite build 121946). How to see the problem on main: import { SQL } from "bun";
const sql = new SQL("postgres://localhost/postgres", { max: 1 });
const query = sql`SELECT 1`;
const calls = { then: 0, finally: 0 };
for (const name of ["then", "finally"]) {
const original = query[name];
query[name] = function (...args) {
calls[name]++;
return original.apply(this, args);
};
}
await query;
console.log(calls); // main: { then: 2, finally: 1 }. This PR: { then: 1, finally: 0 }The test does the same with mock servers: |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughSQL transaction and pooled-connection cleanup now uses a shared query-settlement helper instead of attaching cleanup with ChangesSQL query settlement cleanup
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change avoids replaceable finally methods while preserving SQL cleanup behavior. No actionable merge-blocking risk was identified; merge after normal checks pass. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the semantic differences between finally(cb) and $then(cb, cb): both call sites register only from inside the query's handler, so skipping Query.then()'s #runAsyncAndCatch() start side effect changes nothing; both bound callbacks (onTransactionQueryDisconnected, onQueryFinish) ignore the value/reason argument $then now passes; and the synchronous notTaggedCallError throw from Query.finally() was unreachable at these sites since identifier helpers never reach the handler.
Extended reasoning...
The change replaces two internal query.finally() calls in the SQL client (src/js/bun/sql.ts, src/js/internal/sql/shared.ts) with a $then-based helper in src/js/internal/sql/query.ts, and adds tests. It touches no security-sensitive surface beyond tamper-resistance of built-ins. Inline findings already exist, so this note only records the refactor-preservation checks that were ruled out.
|
The three inline findings have answers in their threads. In short:
|
There was a problem hiding this comment.
LGTM — the second commit trimmed the comments to one line, and the swap from query.finally() to onQuerySettled() is behavior-preserving at both sites.
What was reviewed:
- Both call sites (
queryFromTransactionHandlerin src/js/bun/sql.ts:195,bindQueryin src/js/internal/sql/shared.ts:808) run inside the query handler, after#run/#runAsyncsetexecutedand after thenotTaggedcheck — so the oldfinally()override's#runAsyncAndCatch()and notTagged throw were already no-ops there. $then(cb, cb)registers one reaction at the same point as the oldthis.then(thenFinally, catchFinally), so callback order among reactions is unchanged; both mark the query handled, and the derived promise is poked as handled in both versions. The extra value/reason argument is ignored byonQueryFinishandonTransactionQueryDisconnected.query.$thenon aQuerysubclass follows the existing.$then(pattern in src/js (fs.promises, dgram, BundlerPlugin); the remaining.finallysites in postgres.ts/shared.ts are on plain promises, not queries.
Extended reasoning...
The change adds a 3-line onQuerySettled helper to src/js/internal/sql/query.ts, exports it from the module's export default, and rewires the two internal settle-wait sites (transaction bookkeeping in src/js/bun/sql.ts and pooled-connection release in src/js/internal/sql/shared.ts) plus adds three tests to test/js/sql/sql-pool-transaction-isolation.test.ts. It touches no security-sensitive surface (no auth, injection, or data exposure paths); the risk would be a leaked pooled connection or changed reaction ordering, and tracing both call sites shows they are only reached from inside the query handler where the old override's side effects were already inert. The bug hunt ran dry with no findings, the second commit addressed the one optional comment from the prior review, no CODEOWNERS entry covers the changed files, and the diff is small and self-contained with tests that exercise pool, reservation, transaction and pre-send-rejection paths.
|
Updated 1:12 PM PT - Sep 30th, 2026
✅ @robobun, your commit c29573e91e4fc58a1e5b5ba20e30ace9533d570e passed in 🧪 To try this PR locally: bunx bun-pr 44315That installs a local version of the PR into your bun-44315 --bun |
This branch has not been deployed
2 participants
Problem
query.finally()on each query that it runs: the pool atsrc/js/internal/sql/shared.ts:807, a transaction or a reservation atsrc/js/bun/sql.ts:195.finally()callsthen()ofQuery, which starts an async function: 17 of the 76 cells of an awaited PostgreSQL query.Fix
onQuerySettled()adds one reaction with the private$then()and one callback for both outcomes.test/js/sql/sql-pool-transaction-isolation.test.ts, 5 new tests, 3 fail on main.test/js/sqlon release builds: no new failure.Background
then()orfinally(). Reactions run as microtasks, in registration order.$thenisPromise.prototype.thenunder a private name. User code cannot replace it.Query.resolve()andQuery.reject(): 3 more cells saved, but the callback moves before or after all reactions of the caller, and both change results (Notes).Downsides
close()calls from reactions print the same (Notes).finally(), for examplesql.close()with a query in flight. A program that replacesPromise.prototype.finallystill stops those (Notes).Notes
Cells for each successful query (release builds of main and of this PR, linux x64, PostgreSQL 17.11, MariaDB 11.8.6). A sample is the change of
heapStats().objectTypeCountsover 500 queries with no collection between the two counts, after 30,000 warmup queries, withBUN_GC_TIMER_DISABLE=1. Median of 15 samples. The spread is under 0.2, exceptquery.then(cb)on PostgreSQL with this PR (59.77 to 60.95).await queryquery.then(cb)awaitin a transactionawaiton a reserved connectionawait queryquery.then(cb)awaitin a transactionawaiton a reserved connectionawait queryawaitin a transactionAn awaited PostgreSQL query makes 7 fewer functions, 6 fewer promises, 2 fewer scopes and 2 fewer async function generators. A SQLite query outside a transaction has no such callback, so it is the control.
CPU time for each query shows no difference above the noise of this machine: PostgreSQL 38.9 to 55.9 µs on main and 45.7 to 53.3 µs with this PR, MySQL 29.9 to 36.3 µs and 18.5 to 38.6 µs (user and system time of the process, 8 interleaved runs of 30,000 awaited queries each). This PR makes no claim on time.
A replaced
Promise.prototype.finally. This is a side effect, not the goal. WithPromise.prototype.finally = function () { return this; }set before the first query, a script of 11 steps (queries,begin(),reserve(), a failing query, a savepoint,close()) passes its first 3 steps on main. Thenbegin()waits for a connection that never goes back, and steps 4 to 11 hang or end inERR_POSTGRES_IDLE_TIMEOUT. With this PR all 11 steps pass. With a counting wrapper onPromise.prototype.thenthe same script makes 119 calls on main and 27 with this PR.Other waits of the client still call the public
finally()orthen():sql.close()with a query in flight (src/js/internal/sql/shared.ts:1364,:1371,:1387),close({ timeout })of a transaction or a reservation (src/js/bun/sql.ts:511,:797, which #39617 and #32149 rewrite),savepoint()(sql.ts:870) and the LISTEN connection of PostgreSQL (src/js/internal/sql/postgres.ts:770,:814,:835). With the same replacement,sql.close()with a query in flight does not resolve, on main and with this PR. This PR changes only the two calls that run for each query.What prints the same on both builds
Set.prototype.delete, which the callback calls:await query,query.then(cb),query.execute()thenthen(cb),query.execute().then(cb),query.execute()thenawait, andthen(cb)before and afterexecute()in a transaction and on a reservation.close(),close({ timeout: 30 }),close({ timeout: 0 }),end(),reserve(),begin(), new queries, new queries followed byclose(), for a pool of 1 and of 2, forquery.then(cb)andquery.execute().then(cb).close()andclose({ timeout: 30 })of a transaction and of a reservation, called in a reaction of one of their queries (8 cases).sql.begin()callback that a reaction starts, which connection a query from a reaction gets in a pool of 2, the statements on the wire for 24 calls from reactions, the order logs of 35 transaction scenarios, and the unhandled-rejection reports of 19 kinds of unread query.The first design, and why it is not in this PR. It gave the callback a promise of its own, which
Query.resolve()andQuery.reject()resolve, so that the query has only the reactions of the caller. That removes 3 more cells:then()on an object of a subclass ofPromisemakes a promise capability (two functions and an object), andthen()on a plain promise does not. But on main the place of the callback depends on when the caller added its reaction: before or after the query got its connection. A callback that is not a reaction of the query must take one fixed place, and each place changes something that a program can see:execute()can go behind a slow statement on another connection (pool of 2), andsql.begin()from a reaction runs in the async context of the query, not of its caller.close({ timeout })in a reaction of a query no longer counts that query as pending, so it rolls back where main commits, and it waits for the timeout where main returns at once.Those changes can be right, but each needs its own decision.
The 8 concerns of the self-review. Most of the review ran on the first design. Three concerns were its behaviour changes (the three above) and do not exist here. Four were about its tests: a count on the shared prototype of
Querythat fails withbun test --concurrent, a test name that said "no reaction", a test whose title named an AsyncLocalStorage property that does not hold, and a 30 s test timeout that is under the timeout of CI. Here the count is on one query object, the name says what is counted, and the other two tests are gone. The last one is a fault that main has: identifier helpers, fragments and queries that fail before they run never leave the pending set of a transaction or a reservation, soclose({ timeout })waits for them. This PR does not change it.Found on the way, not changed here.
tx.unsafe()andreserved.unsafe()(andfile()) do not check that the transaction or the reservation is still open (src/js/bun/sql.ts:390and:696). Aftertx.close(),tx.unsafe("INSERT ...")runs outside the transaction and is committed. Afterreserved.release(),reserved.unsafe()runs on a connection that another caller can have in a transaction. main and 1.4.2 do this. The tagged calls reject withERR_POSTGRES_CONNECTION_CLOSED.Suites. Debug build with ASAN:
sql-pool-transaction-isolation.test.ts(23 pass),sql-reserve-abort.test.ts,sql-close-pending-connection.test.ts,wire-frames.test.ts. Release builds of main and of this PR, all oftest/js/sqlagainst local PostgreSQL and MariaDB servers: 30 tests fail on both for the setup of that machine (TLS, password methods, MySQL 9), the 3 new tests fail only on main, and no test fails only with this PR. One 5 s timeout inpostgres-listen-notify.test.tsdid not repeat in 3 more runs of that file on each build.[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file