Repository navigation
Conversation
…nges delta Statement#run() and Database#run() computed .changes as the sqlite3_total_changes() delta across the statement, which includes rows written by triggers and foreign-key cascades. Use sqlite3_changes64() instead so .changes reflects only the rows directly modified by the statement, matching better-sqlite3, node:sqlite, and Python sqlite3. For multi-statement Database#run(), sum sqlite3_changes64() per statement, gated on total_changes movement so non-modifying statements don't re-add the previous statement's count.
|
Reproduced with the trigger/cascade script (received 3/6/4, expected 1/2/1). Fix applied in |
|
Warning Review limit reached
Next review available in: 14 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 5:56 AM PT - Jul 21st, 2026
✅ @robobun, your commit b9a73ffe1f070ae1c89071a113b49b4e91477dd1 passed in 🧪 To try this PR locally: bunx bun-pr 34916That installs a local version of the PR into your bun-34916 --bun |
There was a problem hiding this comment.
I didn't find any bugs. The fix is correct and matches node:sqlite/better-sqlite3, but it changes user-visible semantics — Statement#run() on a read-only statement now returns the previous write's changes instead of 0 (an existing test assertion was updated). Worth a human sign-off on that behavior change.
What was reviewed:
jsSQLStatementExecuteStatementFunctionRunnow readssqlite3_changes64(db)— identical toNodeSqlite.cpp:2795and safe on macOS 12 via thelazy_sqlite3_changes64fallback.- Multi-statement
Database#run()gates the per-statementchanges64sum ontotal_changesmovement so a trailingSELECT/DDL doesn't re-add the prior count — covered by the new test. - New test's sanity checks (log=14, ch=0) confirm triggers/cascades actually fired, so the assertion isn't vacuous.
Extended reasoning...
Overview
Two functions in src/jsc/bindings/sqlite/JSSQLStatement.cpp are changed: jsSQLStatementExecuteFunction (Database#run, multi-statement) and jsSQLStatementExecuteStatementFunctionRun (Statement#run). Both previously computed .changes as sqlite3_total_changes() after − before, which includes trigger and ON DELETE CASCADE rows. Now Statement#run reads sqlite3_changes64() directly (matching NodeSqlite.cpp, better-sqlite3, and Python's sqlite3), and Database#run sums sqlite3_changes64() per statement, gated on total_changes moving so read-only/DDL statements don't double-count. A new test in test/js/bun/sqlite/sqlite.test.js covers triggers, multi-row inserts, multi-statement run(), and FK cascades; one existing assertion (stmt2.run() on a SELECT returning changes: 0) was updated to changes: 1.
Security risks
None. This only changes how a per-connection change counter is read after stepping a statement. No new user-controlled data flows into SQLite, no new allocation, no new FFI surface.
Level of scrutiny
Medium. The core fix (use sqlite3_changes64 for direct changes) is the textbook approach and mirrors Bun's own node:sqlite implementation line-for-line. However, it alters a documented public return value: Statement#run().changes on a SELECT is no longer 0 but the connection's most recent direct-change count. The PR description acknowledges this and cites better-sqlite3/node:sqlite as precedent, and .run() is documented for non-returning statements — but it's still a semantic change to a Bun-native API that a maintainer should confirm is acceptable.
Other factors
sqlite3_changes64availability on old macOS system SQLite:lazy_sqlite3.h:508-512already installs a 32-bit fallback when the symbol is absent, so no null-fn-pointer crash on macOS 12.- Type width: previous code used
int − int(32-bit), new code accumulates insqlite3_int64and casts todoubleforjsNumber— strictly wider than before. - Test quality: the new test asserts positive counts (1, 2, 1, 3, 1) and verifies via
SELECT count(*)that the trigger and cascade actually ran, so it can't pass vacuously if the schema setup silently failed. - No prior human review comments on the PR.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-21 and it conflicts with main. 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
Statement#run().changesandDatabase#run().changeswere computed as thesqlite3_total_changes()delta around the statement, so every row a trigger writes and every child row anON DELETE CASCADEremoves was added to the count. The standard optimistic-concurrency checkif (res.changes !== 1) throwmiscounts on any schema with triggers or cascades.Repro
Fix
Statement#run(): readsqlite3_changes64()after stepping, same asbetter-sqlite3,node:sqlite(including Bun's own implementation inNodeSqlite.cpp), and Pythonsqlite3.Database#run()(multi-statement): sumsqlite3_changes64()per statement, gated onsqlite3_total_changes()movement so aSELECTor DDL statement doesn't re-add the previous statement's count.Behavior change
Statement#run()on a read-only statement (e.g.SELECT) now returns the direct-change count from the most recently completedINSERT/UPDATE/DELETEon the connection, rather than0. This matchesbetter-sqlite3andnode:sqlite;.run()is documented for statements that do not return rows. One existing test assertion was updated accordingly.Verification
main(received3, expected1) and passes with this change.bun bd test test/js/bun/sqlite/: 101 passbun bd test test/js/node/sqlite/: 115 pass, 4 skipno 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