Repository navigation
Conversation
…ents The multi-statement loop in jsSQLStatementExecuteFunction checked the result of sqlite3_prepare_v3 but never the result of sqlite3_step. A statement that prepares fine but fails at step time (for example ALTER TABLE ADD COLUMN with a non-constant default on a populated table) set rc without breaking the loop, so the next iteration overwrote it and execution continued. Even a trailing newline or comment after a failing final statement reset rc to SQLITE_OK and swallowed the error. Now the loop stops at the first failing statement and the existing post-loop check throws with the correct error, matching sqlite3_exec semantics. Fixes #37415
|
Warning Review limit reached
Next review available in: 4 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 (2)
Comment |
|
Updated 9:48 PM PT - Aug 10th, 2026
✅ @robobun, your commit 5c0c75c8b65920b6b077aab5770c61af8d3a5037 passed in 🧪 To try this PR locally: bunx bun-pr 37418That installs a local version of the PR into your bun-37418 --bun |
There was a problem hiding this comment.
LGTM — tight, well-reasoned fix that stops the multi-statement exec loop at the first step-time failure so the post-loop error check can throw it.
What was reviewed
- Traced the loop:
rcfromsqlite3_stepis now checked before the nextsqlite3_prepare_v3can overwrite it; the break condition mirrors the post-loop check exactly. - Confirmed
AutoDestructingSQLiteStatement's destructor finalizes the failed stmt onbreak, and SQLite preserves the db error state through finalize socreateSQLiteErrorreads the right message. - Checked the
didExecuteAnyordering — the error check at line 1593 runs before the empty-query check, so a first-statement step failure still throws the SQLite error, not the empty-query message. - Test covers mid-string / trailing-newline / trailing-comment variants and asserts the negative contract (later statements did not run).
Extended reasoning...
Overview
Two-file change: a 4-line addition in jsSQLStatementExecuteFunction (src/jsc/bindings/sqlite/JSSQLStatement.cpp) that breaks out of the multi-statement exec loop when sqlite3_step ends with anything other than SQLITE_DONE/SQLITE_OK, and a new test in test/js/bun/sqlite/sqlite.test.js covering the three positional variants from the issue.
Security risks
None. This tightens error handling — a previously-swallowed failure now throws. No new user input parsing, no allocation, no FFI surface change.
Level of scrutiny
Low-to-medium. The change is a single conditional break inside an existing loop whose post-loop error path already handles the thrown case; the diff only ensures rc reaches that path instead of being clobbered by the next prepare. I verified:
- The
AutoDestructingSQLiteStatementlocal finalizes the stmt on scope exit at thebreak; per SQLite semantics, finalizing a stmt that failed at step returns the same error and leaves the db error state intact forsqlite3_extended_errcode/sqlite3_errmsgincreateSQLiteError. - Skipping
didExecuteAny = trueon the failing statement is harmless: thercerror check (line 1593) precedes the!didExecuteAnyempty-query check (line 1598), so a first-statement step error still throws the correct SQLite error rather than the generic empty-query message. - The break condition includes
SQLITE_OKdefensively to match the post-loop check verbatim;sqlite3_stepdoesn't actually returnSQLITE_OK, so this is just symmetry.
Other factors
The test asserts the exact SQLite error message ("Cannot add a column with non-constant default") and that the statement following the failure did not execute — a strong invariant per REVIEW.md guidance. The PR description states the test fails on current bun and the full sqlite suite passes locally. No CODEOWNERS entry for this path. No prior human review comments to address.
Fixes #37415
Fixes #41010
Bug
Database.exec()with a multi-statement string silently swallowed a statement that prepares successfully but fails atsqlite3_step(reliable reproducer:ALTER TABLE t ADD COLUMN c TEXT NOT NULL DEFAULT (datetime('now'))on a populated table, which throwsCannot add a column with non-constant default), unless the failing statement was the absolute last token of the string. Any trailing content, even a single newline or a line comment, flipped it to silent, and execution continued with the following statements. A migration runner wrapping a file in BEGIN/COMMIT could commit a half-applied migration.Cause
The multi-statement loop in
jsSQLStatementExecuteFunction(JSSQLStatement.cpp) checks the result ofsqlite3_prepare_v3(which is why prepare-time errors likeno such tablethrow from any position) but never checked the result of thesqlite3_steploop. A step-time error setrcwithout breaking, so the next iteration's prepare overwrote it. With only a trailing newline or comment left, that prepare returnsSQLITE_OKwith a null statement, resettingrcbefore the post-loop error check.Fix
Break out of the loop when
sqlite3_stepends with anything other thanSQLITE_DONE/SQLITE_OK; the existing post-loop check then throws with the correct error message. This matchessqlite3_exec()semantics: stop at the first error and report it.Verification
New test in
test/js/bun/sqlite/sqlite.test.jscovers the failing statement mid-string, with a trailing newline, and with a trailing comment, and asserts statements after the failure did not run. Fails on current bun, passes with this change. Full sqlite suite passes locally.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/sqlite/sqlite.test.js