sql: await array result before RELEASE SAVEPOINT

[robobun]

What

tx.savepoint(async sp => [q1, q2, ...]) was sending RELEASE SAVEPOINT before awaiting the returned array of queries. The queries then executed outside the savepoint, so:

• A failing query left its siblings' writes durable in the outer transaction (savepoint atomicity lost).
• The catch block's ROLLBACK TO SAVEPOINT sN failed with no such savepoint: s0 (SQLite) / 3B001 (Postgres), masking the caller's real error.

Repro

import { SQL } from "bun";
const sql = new SQL("sqlite://:memory:");
await sql`CREATE TABLE accounts (id INTEGER PRIMARY KEY, balance REAL)`;
await sql`INSERT INTO accounts VALUES (1, 1000), (2, 500)`;

let err;
await sql.begin(async tx => {
  try {
    await tx.savepoint(async sp => [
      sp`INSERT INTO accounts VALUES (3, 100)`,
      sp`INSERT INTO accounts VALUES (1, 999)`,  // PK violation
    ]);
  } catch (e) { err = e; }
});
console.log(String(err));
console.log(await sql`SELECT id FROM accounts ORDER BY id`);

Before:

SQLiteError: no such savepoint: s0
[ { id: 1 }, { id: 2 }, { id: 3 } ]

After:

SQLiteError: UNIQUE constraint failed: accounts.id
[ { id: 1 }, { id: 2 } ]

Cause

run_internal_savepoint in src/js/bun/sql.ts had:

let result = await savepoint_callback(transaction_sql);
await run_internal_transaction_sql(`RELEASE SAVEPOINT ${name}`);  // released first
if ($isArray(result)) result = await Promise.all(result);         // queries run here

The top-level begin runner in the same function already has the correct order (await the array, then COMMIT). This applies the same ordering to the savepoint path.

Fix

Swap the two blocks so Promise.all(result) is awaited before RELEASE SAVEPOINT. Adapter-generic (the transaction runner is shared across SQLite/Postgres/MySQL).

Verification

• bun bd test test/js/sql/sqlite-sql.test.ts -t "savepoint returning an array": 2 pass
• Fail-before: with src/ stashed, the rollback test fails with no such savepoint: s0
• Full sqlite-sql.test.ts: 232 pass, 1 pre-existing unrelated timeout (properly finalizes prepared statements, also times out on main)

[robobun]

^{Updated 8:05 AM PT - Jul 8th, 2026}

❌ @robobun, your commit bf0246d has 2 failures in Build #70499 (All Failures):

• test/regression/issue/26030.test.ts - code 1 on 🐧 3.23 x64
• test/js/bun/http/proxy-stress-concurrent.test.ts - code 1 on 🍎 14 x64

---

🧪   To try this PR locally:

bunx bun-pr 33750

That installs a local version of the PR into your bun-33750 executable, so you can run:

bun-33750 --bun

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 9 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ef6a1446-faeb-40f5-b01f-4d442d1f0cc5

📥 Commits

Reviewing files that changed from the base of the PR and between 6f6b8e8 and bf0246d.

📒 Files selected for processing (1)

• test/js/sql/sqlite-sql.test.ts

Walkthrough

This PR reorders savepoint release so array results are resolved before RELEASE_SAVEPOINT_COMMAND, and adds SQLite tests for savepoint array result visibility, rollback behavior, and debug-mode stress-test iteration changes.

Changes

Savepoint Result Resolution Fix

Layer / File(s)	Summary
Reorder release after result resolution src/js/bun/sql.ts	run_internal_savepoint now awaits array results via Promise.all before executing RELEASE_SAVEPOINT_COMMAND.
Savepoint array result tests test/js/sql/sqlite-sql.test.ts	Adds savepoint tests for array result visibility and atomic rollback, and conditionally lowers prepared-statement stress-test iterations in debug mode.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: awaiting array results before releasing the savepoint.
Description check	✅ Passed	The description includes the problem, repro, fix, and verification despite using different headings.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

This PR may be a duplicate of:

1. sql(sqlite): detect SQLite's automatic ROLLBACK inside sql.begin() #33749 - Also moves Promise.all(result) before RELEASE SAVEPOINT in run_internal_savepoint, as part of a larger fix for SQLite automatic rollback detection

🤖 Generated with Claude Code

[robobun]

Re the duplicate flag for #33749: the src/js/bun/sql.ts change here overlaps with one block in that PR (both move the $isArray(result) await before RELEASE SAVEPOINT), but the two PRs address different bugs with different test coverage:

• sql(sqlite): detect SQLite's automatic ROLLBACK inside sql.begin() #33749 fixes SQLite's automatic-ROLLBACK case (ON CONFLICT ROLLBACK / RAISE(ROLLBACK)), adds an adapter hook, and touches 4 files. Its reorder of the array await is incidental to checking transaction_still_open() afterward.
• This PR is a 6-line fix for the adapter-generic savepoint array-return ordering bug (no such savepoint: s0 on SQLite, 3B001 on Postgres). Its tests cover the specific case sql(sqlite): detect SQLite's automatic ROLLBACK inside sql.begin() #33749's tests don't: a savepoint callback returning an array where one query fails with an ordinary error, verifying the sibling write is rolled back and the real error surfaces.

If #33749 lands first the src/ change here becomes a no-op and only the two tests remain; happy to rebase to tests-only in that case. If this lands first, #33749 has a trivial conflict in run_internal_savepoint.

[claude]

LGTM — straightforward reorder that mirrors the existing begin path, with solid success/failure tests.

Extended reasoning...

Overview

This PR reorders two statements in run_internal_savepoint (src/js/bun/sql.ts) so that when a savepoint callback returns an array of queries, Promise.all(result) is awaited before RELEASE SAVEPOINT is sent. Previously the savepoint was released first, so the queries executed outside the savepoint — losing atomicity and causing ROLLBACK TO SAVEPOINT to fail with no such savepoint, masking the real error. Two regression tests are added to test/js/sql/sqlite-sql.test.ts covering the success path (array resolves before release) and the failure path (atomic rollback, real error surfaced, outer transaction unaffected).

Security risks

None. This is pure control-flow reordering in the SQL transaction runner with no changes to input handling, escaping, auth, or connection management.

Level of scrutiny

Low. The 3-line move is mechanically identical to the pattern already used ~50 lines below in the same function for the top-level begin path (await array → then COMMIT). The fix simply brings the savepoint path into consistency with its sibling. The transaction runner is adapter-generic, so the fix applies uniformly to SQLite/Postgres/MySQL without adapter-specific branching.

Other factors

• Tests are placed in the existing Transactions describe block and reuse its beforeEach fixture (fresh :memory: DB with seeded accounts), so they're hermetic and follow file conventions.
• The failure test asserts the specific error message (UNIQUE constraint failed) rather than just "threw", and verifies both that the sibling insert was rolled back and that a subsequent write in the outer transaction still commits — exercising the full contract.
• PR description documents fail-before verification (test fails with no such savepoint: s0 on unpatched build).
• No prior reviews or outstanding comments on the timeline.

[claude]

LGTM — straightforward reorder to await the array result before RELEASE SAVEPOINT, matching the existing begin/COMMIT path.

Extended reasoning...

Overview

This PR moves a 3-line block in run_internal_savepoint (src/js/bun/sql.ts) so that when a savepoint callback returns an array of queries, Promise.all(result) is awaited before RELEASE SAVEPOINT is sent, rather than after. It adds two SQLite tests (success + failure/rollback paths) and reduces the iteration count of an unrelated slow test under debug builds.

Security risks

None. No new inputs, no parsing, no auth/crypto, no cross-boundary data flow. Pure control-flow reordering of two existing awaited statements.

Level of scrutiny

Low. The fix is a mechanical swap that makes the savepoint path match the ordering already used a few lines below in the top-level transaction runner (await Promise.all(transaction_result) → COMMIT). The bug and fix are both self-evident: releasing the savepoint before the queued queries execute means those queries run outside the savepoint, and a subsequent failure tries to ROLLBACK TO SAVEPOINT a name that no longer exists. The reordered version keeps the queries inside the savepoint scope so the catch block's rollback is valid and the caller's real error surfaces.

Other factors

• The change is adapter-generic (shared runner for SQLite/Postgres/MySQL); no adapter-specific logic touched.
• Tests are well-formed: the failure test asserts the real error (UNIQUE constraint failed), verifies the sibling insert did not survive, and verifies the outer transaction's post-savepoint write did commit — exactly the invariants the fix restores. Author confirmed fail-before with the src change stashed.
• The isDebug ? 1000 : 10000 tweak is a benign test-runtime reduction for a pre-existing debug timeout the author noted.
• No CODEOWNERS entries match these paths.
• There is overlap with #33749; the author has already explained the relationship and either merge order works with a trivial rebase.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test/js/sql/sqlite-sql.test.ts (1)

1121-1165: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Tighten the savepoint error assertion
test/js/sql/sqlite-sql.test.ts:1154 — the array/savepoint cases hit the ordering path and use toEqual correctly, but String(caught).toMatch(/UNIQUE constraint failed/) is still too loose. Assert the thrown error shape/message more specifically so a masked no such savepoint failure can’t pass.

🤖 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 `@test/js/sql/sqlite-sql.test.ts` around lines 1121 - 1165, The savepoint
failure check is too broad, so tighten the assertion in the savepoint array
rollback test to verify the actual thrown error shape/message rather than only
matching a substring. Update the test around the `tx.savepoint`/`sp` flow to
assert the specific error object or exact message from the unique constraint
failure, ensuring a masked “no such savepoint” error cannot satisfy the
expectation.

🤖 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.

Outside diff comments:
In `@test/js/sql/sqlite-sql.test.ts`:
- Around line 1121-1165: The savepoint failure check is too broad, so tighten
the assertion in the savepoint array rollback test to verify the actual thrown
error shape/message rather than only matching a substring. Update the test
around the `tx.savepoint`/`sp` flow to assert the specific error object or exact
message from the unique constraint failure, ensuring a masked “no such
savepoint” error cannot satisfy the expectation.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: bcd4fa7c-d5c8-4de0-9f48-889e00c2be51

📥 Commits

Reviewing files that changed from the base of the PR and between f54e219 and 6f6b8e8.

📒 Files selected for processing (1)

• test/js/sql/sqlite-sql.test.ts

[claude]

LGTM — straightforward reorder that mirrors the existing begin path's array-await-before-COMMIT ordering, with solid test coverage.

Extended reasoning...

Overview

This PR fixes an ordering bug in run_internal_savepoint (src/js/bun/sql.ts): when a savepoint callback returned an array of queries, RELEASE SAVEPOINT was sent before the queries were awaited, breaking savepoint atomicity and masking the real error with "no such savepoint". The fix is a 6-line block swap so Promise.all(result) runs before RELEASE SAVEPOINT — exactly mirroring the ordering already used a few lines below in the same function's top-level begin path (await array → COMMIT). Two new tests cover the success and rollback paths, and a pre-existing slow test's iteration count is reduced under debug builds only.

Security risks

None. This is a pure control-flow reorder in the transaction runner; no new inputs are parsed, no auth/crypto/permissions surfaces are touched, and the SQL executed is unchanged (only the sequencing of already-issued commands).

Level of scrutiny

Low-to-moderate. The change is small, self-contained, and follows an established in-file pattern verbatim. The bug and fix are both clearly explained with a before/after repro, and the author verified fail-before/pass-after. The transaction runner is shared across adapters, but the reorder is adapter-generic and only tightens correctness (no path where releasing before awaiting the queries could have been intentional).

Other factors

• The bug hunting system found no issues.
• Tests are well-constructed: the rollback test asserts the real error surfaces (UNIQUE constraint), the sibling insert is rolled back, and the outer transaction's post-savepoint write still commits — covering the exact failure mode described.
• The isDebug ? 1000 : 10000 change to "properly finalizes prepared statements" only reduces iterations in debug builds (release CI unchanged); the PR notes this test already times out on main under debug, and isDebug from harness is the standard idiom for this.
• There's overlap with #33749, but the author has already addressed that in the thread and the two PRs are compatible either way.

[claude]

LGTM — minimal reorder that mirrors the already-correct begin path, with solid before/after test coverage.

Extended reasoning...

Overview

This PR reorders three lines in run_internal_savepoint (src/js/bun/sql.ts) so that when a savepoint callback returns an array of queries, they are awaited via Promise.all before RELEASE SAVEPOINT is sent — instead of after. The top-level begin runner in the same function already awaits the array before COMMIT, so this simply brings the savepoint path to parity. Two regression tests are added to test/js/sql/sqlite-sql.test.ts (success and rollback cases), plus a debug-mode iteration reduction for an unrelated stress test that was timing out on main.

Security risks

None. This is transaction control-flow ordering in the shared JS SQL layer; no auth, crypto, untrusted input parsing, or filesystem/network surface is touched.

Level of scrutiny

Low-to-medium. The source change is a 3-line block move with no new logic, and it mirrors the exact pattern already present ~60 lines below for the outer transaction. The bug and fix are both easy to reason about: releasing a savepoint before the queries it's supposed to wrap have executed is clearly wrong, and the fix restores the intended atomicity guarantee.

Other factors

• Tests are strong: the rollback test asserts the exact error code (SQLITE_CONSTRAINT_PRIMARYKEY), the exact message, and the exact resulting rows in one combined assertion — verifying both that the sibling write inside the failed savepoint is rolled back and that a subsequent write in the outer transaction still commits.
• The PR description documents fail-before verification (no such savepoint: s0 with src stashed).
• No CODEOWNERS coverage for these paths.
• The isDebug ? 1000 : 10000 iteration reduction follows the established harness pattern for debug builds and addresses a pre-existing timeout the author noted.
• The overlap with #33749 is acknowledged and the author's plan (rebase to tests-only if that lands first) is reasonable; either merge order works.

[robobun]

CI status: the diff is green on every lane that ran test/js/sql/sqlite-sql.test.ts (no mention of that file or savepoint anywhere in the build annotations). Outstanding CI red on build #70499 is unrelated to this change:

• darwin-14-x64-test-bun (shard 1, exit 3): Cannot find module '_util/numeric.ts' from .../test/harness.ts, the file exists in the repo so the box's checkout is incomplete; shard was then SIGTERM'd.
• darwin-14-x64-test-bun (shard 2, exit 2): test/js/bun/http/proxy-stress-concurrent.test.ts dropped 1-9 of 1200 https-proxy requests.
• alpine-3.23-x64-test-bun (exit 2): test/regression/issue/26030.test.ts failed because the MySQL docker container did not become healthy within 1m.
• darwin-14-aarch64-test-bun still scheduled (agent backlog; the same lane expired on build #70482).

All other failures are in the warning/flaky annotation (passed on retry). None touch src/js/bun/sql.ts or sqlite-sql.test.ts. Ready for maintainer review.