Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 8 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 (4)
Comment |
|
Updated 12:27 PM PT - Jul 8th, 2026
❌ @robobun, your commit d09a2d0 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 33582That installs a local version of the PR into your bun-33582 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Thanks, but I don't think this PR fixes #30811. That issue is about const r = await sql.unsafe("insert into dst select v from src");
// r.count === 0 (the rows are inserted, but the affected-row count is still 0)That looks like what #30814 ("report affected row count for INSERT/UPDATE/DELETE with subquery SELECT") is targeting. This PR is scoped to the separate bug where statements after the first are silently dropped when the first statement returns rows, so I'm leaving the auto-close link off. |
There was a problem hiding this comment.
Thanks for addressing the nits — no correctness issues found, but the multi-statement execution semantics (bindings only to the first statement, first row-returning statement wins) and the new C++ scan-ahead in prepare are design choices worth a human sign-off.
Extended reasoning...
Overview
This PR fixes Bun.SQL's SQLite adapter silently dropping every statement after the first when the first statement returns rows. It touches four files: adds a remainingSQL getter and a scan-ahead loop in JSSQLStatement.cpp (C++ JSC binding), rewrites the row-returning branch of SQLiteQueryHandle.run() in src/js/internal/sql/sqlite.ts into a statement-by-statement loop, adds the remainingSQL field to the CppSQLStatement TS interface, and adds six new tests plus shrinks two slow existing tests in sqlite-sql.test.ts.
I previously left three inline nits (perf of the tail-copying loop, per-test timeouts vs. shrinking workload, weak error assertion in the syntax-error test); the author addressed the latter two in a2ca5e3 and acknowledged the first as a follow-up. The bug-hunting system found no issues on the current revision.
Security risks
None identified. The change only affects how a multi-statement string that the caller already passed to sql.unsafe() / sql.file() is executed — it doesn't widen the input surface. The scan-ahead sqlite3_prepare_v3 operates within the bounds of the already-UTF-8-encoded buffer (sqlEnd - cursor), and the probe statement is finalized before the loop exits.
Level of scrutiny
Medium-high. This is a behavioral change to SQL execution, not a mechanical fix. It encodes design decisions that a maintainer should confirm:
- Bindings apply only to the first statement (matching
db.run()). A later statement with?placeholders will get[]and either error or bind NULLs — reasonable, but a choice. - The first row-returning statement provides the result set; subsequent row-returning statements execute via
.run()and their rows are discarded. Postgres's simple-query protocol returns the last result set, so this is a deliberate divergence. - New native surface:
remainingSQLis now readable on every prepared statement via the publicstmt.nativeaccessor inbun:sqlite, and everydb.prepare()call now does at least one extrasqlite3_prepare_v3probe when there's a non-empty tail.
The C++ additions look memory-safe (WTF::String owns its buffer, the probe stmt is finalized on the only path that creates it, the cursor loop is bounded by sqlEnd with a nextTail <= cursor progress guard), but native binding changes in this repo generally get human eyes.
Other factors
Test coverage is solid — six new tests covering both statement orderings, RETURNING execute-once, parameter binding, trailing comments, and syntax-error surfacing with specific error-class assertions. The two shrunk tests were pre-existing debug+ASAN timeouts on main, and the shrink follows repo test guidelines. Given the design-decision surface and native-code changes, I'm deferring rather than auto-approving.
|
The diff is ready (rebased onto 332f744).
None of these touch the sqlite adapter, |
… rows The SQLite adapter routed row-returning queries through db.prepare(), which only compiles the first statement of a multi-statement string and silently discards the rest. A string whose first statement returned rows (e.g. "select count(*) from t; delete from t") returned the rows but never ran the following statements, losing writes with no error. Expose the leftover tail from the native prepare so the adapter can walk every statement in the string, executing each exactly once. Bindings apply to the first statement, matching db.run(). The first statement that returns rows provides the result set.
'properly finalizes prepared statements' (10k awaited inserts) and 'handles exotic but valid SQL patterns' (a 50M-char identifier) take ~13s each under the debug+ASAN build, exceeding the 5s default. Both were already timing out under debug on main before this PR and pass in under 100ms on release builds. Matches the explicit-timeout convention in neighboring SQL tests (sql.test.ts, local-sql.test.ts, postgres-tls-ctx-leak.test.ts).
Drop the 60s timeouts added in 335036e and shrink the workload instead (test/CLAUDE.md): 10000 awaited inserts becomes 2000 and the 50M-char identifier becomes 5M via Buffer.alloc(..., 'a').toString(). Both now finish in ~2s under debug+ASAN without a per-test timeout. Also tighten the 'surfaces a syntax error' test to assert the caught error is a SQLiteError with code SQLITE_ERROR and a 'syntax' message, so an unrelated throw would not pass.
b15b4ca to
d09a2d0
Compare
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-08, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
What
Bun.SQL's SQLite adapter silently dropped every statement after the first when the first statement returned rows. A multi-statement string likeselect count(*) from t; delete from treturned the count and never ran theDELETE, losing writes with no error.This depends on statement order and is easy to hit with
sql.file()running a migration or seed.sqlfile that starts with aSELECTversion check. The PostgreSQL adapter runs the whole multi-statement text, so code ported tosqlite://silently stops writing.Cause
SQLiteQueryHandle.run()has two branches. Non-row queries go throughdb.run(), which executes every statement. Row-returning queries (SELECT/PRAGMA/WITH/EXPLAIN/RETURNING) went throughdb.prepare(sql), andsqlite3_prepare_v3compiles only the first statement of the string, leaving the tail unexecuted and unreported.Fix
The native
preparenow capturessqlite3_prepare_v3's leftover-tail pointer and exposes the remaining SQL on the statement (remainingSQL), scanning past trailing whitespace and comments so the remainder always starts at a real statement and is empty when nothing is left. The adapter walks every statement in the string, executing each exactly once. Bindings apply to the first statement, matchingdb.run(). The first statement that returns rows provides the result set, so the example above still returns[{ before: 3 }]while theDELETEnow runs.Verification
New tests in
test/js/sql/sqlite-sql.test.tscover: row-returning first statement followed by a write, a write followed by a row-returning statement,RETURNINGexecuting exactly once (no double-insert), parameter binding across a batch, trailing comments, and a syntax error in a later statement surfacing after earlier ones run. All fail on the current release build and pass with this change.