ReviewOS

also looking at this

stacks/bun-query-builder

fix(client): a builder's writes belong to the builder's own connection

#1065
Merged glennmichael123 wants to merge fix/builder-local-connection into main
2 files +161 -29
10 days ago

Fixes a silent data-loss bug. Also fixes the last live cause of #1001.

Writes were surviving ROLLBACK

createQueryBuilder captures the connection it was given as _sql (client.ts:2743), but ~29 of its methods reached for the module-global bunSql instead. Those are the same object for a plain builder — which is why this went unnoticed — but db.transaction() hands its callback a builder with a connection of its own, and then the global is a different pooled session.

Reproduced against live Postgres, before the fix:

transaction threw as expected: deliberate failure -> ROLLBACK
rows surviving the ROLLBACK -> [{"name":"should-be-rolled-back"}]
*** DATA LEAK: write survived ROLLBACK ***

The write autocommitted on the other session, so it outlived the rollback. The transaction failed, the caller caught the error, and the row is still in the table — no driver error anywhere. insertGetId, upsert, insertOrIgnore, updateOrInsert and create were all affected.

The read side is the same bug pointed the other way: tx.count() ran on the global, which cannot see the transaction's uncommitted rows, so it returned 0 for a row tx had just inserted. sum/avg/min/max and the advisory-lock helpers were equally adrift — and a lock taken on a session that isn't the transaction's locks nothing. With pool: { max: 1 } these don't return at all: the call waits on the single connection its own transaction is holding.

After the fix, same script:

(1) rows surviving ROLLBACK: 0 OK
(2) tx.count() sees its own uncommitted row: 1 OK
(3) committed rows persist: 1 OK

Why this is a re-fix, not a new idea

unsafe() was already corrected for exactly this, and the comment above it spells out the rule — raw SQL routed through the pool "lets it escape the transaction (and makes row locks such as FOR UPDATE ineffective)". This PR is the rest of that family. The rule now lives on the _sql declaration itself, so the next method added inherits it instead of rediscovering it.

Scope

Swapped to _sql: the advisory locks, ping, file, rawQuery, insertGetId (all four dialect paths), updateOrInsert, upsert/insertOrIgnore, and count/sum/avg/min/max.

Left on the global deliberately: reserve() and close(), which act on the pool rather than a session, and commitDistributed/rollbackDistributed, which by design run outside the transaction that prepared them.

Also drops a stray console.log('resultId', result) that was shipping in the Postgres insertGetId path.

On #1001

ping() was the user-visible face of this: it health-checked the global rather than the connection the builder actually queries, so it could report a healthy builder as down — the reporter's "Database not ready after waiting" — or report ready at boot while the first real query fails (the ordinary export const db = createQueryBuilder(...) then await getConfig() boot order).

Worth noting the reporter's original failure had a different cause that was fixed ~30 releases ago; their exact flow returns true on current main. This was the last remaining live path producing that same error on a healthy database.

Tests

The regression test is Postgres-gated and subprocess-isolated. SQLite cannot catch any of this — bunSql and the transaction share a single bun:sqlite handle, so the distinction the bug turns on doesn't exist there. Verified the test genuinely discriminates: against the unfixed client.ts it fails with Expected: 0, Received: 1 (the leaked row).

Full suite 1699 pass / 4 skip / 0 fail, typecheck 0, lint 0 errors.

🤖 Generated with Claude Code

2 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.