Skip to content

bun:sqlite: fix lone-surrogate encoding, out-of-range BigInt binding, and stale column names - #32841

Open
robobun wants to merge 6 commits into
mainfrom
farm/153748d0/sqlite-bind-column-cache-fixes
Open

bun:sqlite: fix lone-surrogate encoding, out-of-range BigInt binding, and stale column names#32841
robobun wants to merge 6 commits into
mainfrom
farm/153748d0/sqlite-bind-column-cache-fixes

Conversation

@robobun

@robobun robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

Three binding-layer bugs in bun:sqlite where it diverges from node:sqlite and better-sqlite3 running the same SQLite. The first two silently corrupt data.

1. Lone surrogates are stored as ill-formed UTF-8 and can consume the following character

const { Database } = require("bun:sqlite");
const db = new Database(":memory:");
db.query("select hex(cast(? as blob)) h").get("a\ud800b").h // "61F09081A2": "a" + U+10062
db.query("select hex(cast(? as blob)) h").get("a\ud800").h  // "61EDA080":   "a" + raw CESU-8 surrogate bytes
// TextEncoder / node:sqlite / better-sqlite3: 61EFBFBD62, 61EFBFBD

rebindValue bound 16-bit strings with sqlite3_bind_text16, so SQLite's own UTF-16 decoder performed the conversion. That decoder pairs a lone high surrogate with whatever code unit follows (here "b", producing U+10062 and consuming the b), and writes a trailing lone surrogate as the ill-formed 3-byte sequence ED A0 80. The database file then contains text that is not valid UTF-8, so other tools reading the file see corruption, and the stored text differs from the JS string in content as well as encoding.

Fix: always encode to UTF-8 with WTF::StringView::utf8(), which replaces lone surrogates with U+FFFD exactly like TextEncoder and every other string-to-bytes path in Bun, and bind with sqlite3_bind_text. sqlite3_bind_text16 is no longer used, so its entry is removed from lazy_sqlite3.h.

This also fixes a second artifact of bind_text16: a string beginning with U+FEFF or U+FFFE used to have its first two bytes stripped by SQLite's UTF-16 BOM sniffer, so a bound "\uFFFE" came back as "". The test in test/js/sql/sqlite-sql.test.ts that documented both old behaviors is updated.

2. Out-of-range BigInt parameters are silently wrapped modulo 2^64

db.query("select ? as v").get(2n ** 64n + 5n).v // 5
db.query("select ? as v").get(2n ** 63n).v      // -9223372036854775808

rebindValue only performed the int64 range check when safeIntegers was enabled; the default mode called JSBigInt::toBigInt64, which truncates to 64 bits. node:sqlite and better-sqlite3 both throw a RangeError regardless of mode. better-sqlite3's safeIntegers, which Bun's option is modeled on, only controls how integers are returned, never whether BigInt input is validated. Silently storing 5 for 2n ** 64n + 5n is the classic truncation bug for snowflake and other u64 identifiers.

Fix: always range-check the BigInt and throw the same RangeError the safeIntegers path already threw. The now-unused safeIntegers/isSafeInteger parameter is removed from the rebindValue / rebindObject / rebindStatement chain.

This is a behavior change. The previous behavior was covered by an expect(...).not.toThrow(RangeError) assertion and a sentence in docs/runtime/sqlite.mdx; both are updated here. If the silent wrapping was intentional, say so and I will revert this part.

3. A prepared statement's cached column names never refresh after SQLite re-prepares it

Fixes #1332

db.run("create table t (a int)");
db.run("insert into t values (1)");
const q = db.query("select * from t");
q.get();                                             // { a: 1 }
db.run("alter table t add column b int default 42");
q.get();                                             // { a: 1 }   (want { a: 1, b: 42 })
q.values();                                          // [[1, 42]]  (values() reads the live column count)

After DROP TABLE t; CREATE TABLE t (zzz TEXT), q.get() returns the new column's value under the old column's name, so any app that runs DDL at runtime can silently get mis-keyed rows.

initializeColumnNames is gated on a per-connection version counter that only increments when a non-read-only prepared statement steps. The multi-statement exec path used by db.run() / db.exec() never touches it, and schema changes made through another connection to the same file cannot touch it at all. Meanwhile SQLite transparently re-prepares the expired statement inside sqlite3_step(), so the live result shape and the cached column names and Structure diverge.

Fix: detect the re-prepare directly with sqlite3_stmt_status(stmt, SQLITE_STMTSTATUS_REPREPARE, 1) right after each sqlite3_step(). That works for any source of the schema change, including #1332's case of an ALTER TABLE issued by a separate process against a WAL database, which a per-connection counter can never observe. The six copies of the !hasExecuted || need_update() check are folded into one updateColumnNamesIfNeeded helper. sqlite3_stmt_status is added to lazy_sqlite3.h for the macOS dynamic-load path.

The columnTypes getter is a seventh sqlite3_step() site with the same class of bug: it captured sqlite3_column_count before the step, so after a schema change the returned array was truncated (more columns) or padded with spurious "NULL" entries read from out-of-range column indexes (fewer columns). The count is now read after the step.

Not included

new Database(path, {}) (and { readonly: false }) failing with SQLITE_MISUSE because an options object zeroes the open flags was reported alongside these, but it already has an open PR, #32422, so it is not duplicated here.

Performance

Release build (--profile=release, bun-profile), mitata, Xeon Platinum 8375C. Both binaries built from the same tree (ffea69ae7c base); the only difference is this PR's two src/jsc/bindings/sqlite/ files. 3 interleaved rounds; mean of the per-run avg time.

benchmark before after Δ
SELECT * FROM t .all() (200,000 rows × 5 cols) 97.43 ms 98.63 ms +1.24%
SELECT * FROM t .values() (200,000 rows × 5 cols) 96.52 ms 96.94 ms +0.44%
SELECT * FROM s .all() (100 rows × 3 cols) 21.5 µs 21.4 µs -0.43%
INSERT 20,000× (2×ASCII str + int) — unchanged path, noise control 23.30 ms 23.43 ms +0.57%
INSERT 20,000× (2×16-bit str + int) — bind_text16utf8() path 27.16 ms 27.60 ms +1.64%
INSERT 20,000× (2×BigInt + int) — unconditional range check 24.13 ms 23.46 ms -2.77%

Loading a large table via .all()/.values() is unchanged within noise. mitata's per-run min–max spread on the 200K-row .all() was 90–106 ms (±8%), so the +1.2% delta is well inside it. Structurally the new sqlite3_stmt_status(…, SQLITE_STMTSTATUS_REPREPARE, 1) call runs once per .all()/.values() invocation, not per row, so there is no added per-row work in the step loop.

The 16-bit string bind shows +1.6% against a +0.6% on the unchanged ASCII control; the residual ~1% is one extra temporary: WTF::StringView::utf8() allocates a CString, and SQLITE_TRANSIENT then copies it again, whereas sqlite3_bind_text16 transcoded UTF-16→UTF-8 in place inside SQLite. BigInt binding is inside the noise band of the unchanged control.

Benchmark script
import { run, bench, group } from "mitata";
import { Database } from "bun:sqlite";

const ROWS = 200_000;
const db = new Database(":memory:");
db.run("PRAGMA journal_mode = MEMORY");
db.run("PRAGMA synchronous = OFF");
db.run("CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT, email TEXT, blob TEXT, n INTEGER)");
{
  const ins = db.prepare("INSERT INTO t VALUES (?, ?, ?, ?, ?)");
  db.transaction(n => { for (let i = 0; i < n; i++) ins.run(i, `user ${i}`, `user${i}@example.com`, "ascii-payload-ascii-payload-ascii-payload", i * 7); })(ROWS);
}
const selAll = db.prepare("SELECT * FROM t");

group("read (per-row step path)", () => {
  bench(`.all()    [${ROWS} rows, 5 cols]`, () => selAll.all());
  bench(`.values() [${ROWS} rows, 5 cols]`, () => selAll.values());
});

const dbw = new Database(":memory:");
dbw.run("PRAGMA journal_mode = MEMORY");
dbw.run("PRAGMA synchronous = OFF");
dbw.run("CREATE TABLE w (a TEXT, b TEXT, c INTEGER)");
const insw = dbw.prepare("INSERT INTO w VALUES (?, ?, ?)");
const del = dbw.prepare("DELETE FROM w");
const asciiA = "the quick brown fox jumps over the lazy dog";
const asciiB = "hello world hello world hello world hello!!";
const wideA = "日本語テキスト — naïve façade résumé — Ω≈ç√∫";
const wideB = "emoji payload 😀😃😄😁😆🥹😂🤣😊😇 mixed";
const big = 9007199254740993n;

group("bind (rebindValue path)", () => {
  bench("INSERT 20k× (2×ASCII)",  () => { del.run(); for (let i = 0; i < 20_000; i++) insw.run(asciiA, asciiB, i); });
  bench("INSERT 20k× (2×16-bit)", () => { del.run(); for (let i = 0; i < 20_000; i++) insw.run(wideA, wideB, i); });
  bench("INSERT 20k× (2×BigInt)", () => { del.run(); for (let i = 0; i < 20_000; i++) insw.run(big, big + BigInt(i), i); });
});

const dbs = new Database(":memory:");
dbs.run("CREATE TABLE s (a INT, b INT, c INT)");
db.transaction(() => { const q = dbs.prepare("INSERT INTO s VALUES (?, ?, ?)"); for (let i = 0; i < 100; i++) q.run(i, i*2, i*3); })();
const selSmall = dbs.prepare("SELECT * FROM s");
group("read (small table)", () => { bench(".all() [100 rows, 3 cols]", () => selSmall.all()); });

await run();

Rebase note

Rebased onto main after the node:sqlite implementation (#32498), #34925 (which rewrote jsSqlStatementGetColumnNames to read column names live from SQLite instead of the cached array, dropping the seventh initializeColumnNames call site), and the docs editorial pass in #33112. The only conflict was lazy_sqlite3.h: the node:sqlite PR restructured it (static -> inline variables so both TUs share one dlopen) and already provides sqlite3_stmt_status with a fallback stub, so this PR's header diff is now just removing the four now-unused sqlite3_bind_text16 entries. JSSQLStatement.cpp, the tests, and the docs auto-merged cleanly.

Verification

All three reproduce on bun v1.4.0 and on current main. In test/js/bun/sqlite/sqlite.test.js, 24 of the new or updated test cases fail on v1.4.0 and all 113 pass with this change. test/js/sql/sqlite-sql.test.ts, test/js/bun/sqlite/column-types.test.js, test/js/bun/sqlite/sql-timezone.test.js, test/js/sql/sqlite-url-parsing.test.ts, and test/regression/issue/14709.test.ts also pass. Two pre-existing debug-build timeouts in sqlite-sql.test.ts reproduce identically on a debug build of main without this change.


no test proof · iteration 7 · 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

@robobun

robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 9:43 PM PT - Jul 30th, 2026

@robobun, your commit da34ac88449579fd2ef945da4483a043b65d1ead passed in Build #85982! 🎉


🧪   To try this PR locally:

bunx bun-pr 32841

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

bun-32841 --bun

@mintlify

mintlify Bot commented Jun 27, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
bun 🟢 Ready View Preview Jun 27, 2026, 9:09 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@github-actions

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. SQLite schema cache is not properly invalidated #1332 - PR adds sqlite3_stmt_status(SQLITE_STMTSTATUS_REPREPARE) detection after each sqlite3_step(), which invalidates cached column names when SQLite re-prepares a statement due to schema changes from another process/connection

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #1332

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The PR updates SQLite BigInt and UTF-8 binding, lazy symbol loading, prepared-statement metadata refresh, documentation, and schema-change regression tests.

Changes

SQLite runtime bindings and refresh

Layer / File(s) Summary
Lazy SQLite symbols
src/jsc/bindings/sqlite/lazy_sqlite3.h
Adds lazy loading for sqlite3_stmt_status and removes sqlite3_bind_text16 from the SQLite symbol table.
BigInt and UTF-8 binding
src/jsc/bindings/sqlite/JSSQLStatement.cpp, docs/runtime/sqlite.mdx, test/js/bun/sqlite/sqlite.test.js, test/js/sql/sqlite-sql.test.ts
Enforces signed 64-bit BigInt range checks, switches string binding to UTF-8, and updates documentation and Unicode assertions.
Statement reprepare refresh
src/jsc/bindings/sqlite/JSSQLStatement.cpp, test/js/bun/sqlite/sqlite.test.js
Adds metadata refresh after SQLite reprepares and validates updated column names, row shapes, and column types after schema changes across connections and processes.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The UTF-8 surrogate handling and BigInt range-check fixes are unrelated to the linked issue #1332. Split the UTF-8 and BigInt fixes into a separate PR, or link issues that define those objectives.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR addresses #1332 by refreshing metadata after schema changes from the same or another connection or process.
Title check ✅ Passed The title clearly summarizes the three primary bun:sqlite fixes: surrogate encoding, BigInt range validation, and stale column metadata.
Description check ✅ Passed The description explains the changes, verification steps, test results, performance impact, scope, and related issue coverage in sufficient detail.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

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

Inline comments:
In `@docs/runtime/sqlite.mdx`:
- Around line 489-492: The SQLite docs section has a duplicated paragraph about
integer return behavior and the second paragraph uses confusing “use the bigint
type” wording. Remove the redundant paragraph in the docs content near the
sqlite runtime section and keep a single paragraph that clearly says bigint is
supported for inputs while the Database option safeIntegers controls whether
results are returned as bigint instead of number.

In `@test/js/bun/sqlite/sqlite.test.js`:
- Around line 1926-1929: Shorten the multi-line SQLite surrogate warning comment
in the sqlite test so it fits within the 3-line limit without losing the UTF-16
hazard. Keep the note near the bind path logic in sqlite.test.js, and condense
the explanation that lone surrogates must become U+FFFD and must not be passed
raw to SQLite because its decoder can consume following code units or store
ill-formed runs.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c3417b55-3ebd-4dc1-936c-d6afe8380e35

📥 Commits

Reviewing files that changed from the base of the PR and between df92f8f and ec9d818.

📒 Files selected for processing (5)
  • docs/runtime/sqlite.mdx
  • src/jsc/bindings/sqlite/JSSQLStatement.cpp
  • src/jsc/bindings/sqlite/lazy_sqlite3.h
  • test/js/bun/sqlite/sqlite.test.js
  • test/js/sql/sqlite-sql.test.ts

Comment thread docs/runtime/sqlite.mdx Outdated
Comment thread test/js/bun/sqlite/sqlite.test.js Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any bugs, but this touches hot-path native bindings and includes an intentional behavior change (BigInt range-check regardless of safeIntegers) that you've explicitly flagged for maintainer input, so a human should sign off.

Extended reasoning...

Overview

This PR fixes three binding-layer bugs in bun:sqlite by modifying src/jsc/bindings/sqlite/JSSQLStatement.cpp and lazy_sqlite3.h, with corresponding test and doc updates. The three fixes are: (1) replace sqlite3_bind_text16 with WTF-encoded UTF-8 + sqlite3_bind_text so lone surrogates become U+FFFD instead of corrupting stored text; (2) always range-check BigInt parameters and throw RangeError for values outside signed-int64, removing the isSafeInteger parameter from the rebindValue/rebindObject/rebindStatement chain; (3) call sqlite3_stmt_status(stmt, SQLITE_STMTSTATUS_REPREPARE, 1) after each sqlite3_step() to detect transparent re-prepares and refresh cached column names/Structure.

Security risks

None apparent. The changes tighten input validation (rejecting out-of-range BigInts) and improve encoding correctness (no more ill-formed UTF-8 written to disk). No auth, crypto, or permission surfaces are touched.

Level of scrutiny

This warrants human review. It modifies native C++ on the hot path of every parameterized query: every non-ASCII / non-Latin-1-ASCII string now allocates a UTF-8 CString instead of passing UTF-16 directly to SQLite, and every sqlite3_step() is now followed by a sqlite3_stmt_status call. Both look cheap and correct, but a maintainer familiar with the perf characteristics of this module should confirm. More importantly, fix #2 is a deliberate user-visible behavior change — code that previously bound 2n ** 64n silently now throws — and the PR description explicitly asks a maintainer to confirm the old wrapping wasn't intentional. That's a product decision a bot shouldn't make.

Other factors

The implementation is clean and well-tested (22 new/updated test cases covering boundary BigInts, surrogate variants across positional/named/exec paths, and four schema-change scenarios including a second connection). The refactor into updateColumnNamesIfNeeded preserves the existing exception-handling shape at each call site. The lazy_sqlite3.h additions follow the established pattern, and sqlite3_stmt_status has been in SQLite since 3.6.4 so the macOS system dylib will have it. No outstanding reviewer comments. CI is still building.

Comment thread src/jsc/bindings/sqlite/JSSQLStatement.cpp

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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

Inline comments:
In `@docs/runtime/sqlite.mdx`:
- Line 489: The sqlite documentation text uses an imprecise safe-integer
boundary; update the wording in the `bun:sqlite` section to use the exact
JavaScript safe-integer limit via `Number.MAX_SAFE_INTEGER` or `2^53 - 1`
instead of `2^53`, keeping the `Database`/`safeIntegers` guidance otherwise the
same.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 600f6b4e-147b-4aa7-a528-ba46cc4a30ae

📥 Commits

Reviewing files that changed from the base of the PR and between ec9d818 and b1fbe53.

📒 Files selected for processing (3)
  • docs/runtime/sqlite.mdx
  • src/jsc/bindings/sqlite/JSSQLStatement.cpp
  • test/js/bun/sqlite/sqlite.test.js

Comment thread docs/runtime/sqlite.mdx Outdated
@robobun

robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author

Re the "Out of Scope Changes" warning in CodeRabbit's pre-merge checks: the three fixes were reported together, live in the same two source files, and share one test suite, so they were scoped together from the start. The Fixes #1332 link was added afterwards once the issue finder pointed out that the column-name fix closes it; the PR is not scoped to that issue. Happy to split if a maintainer would rather review them separately.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any further issues (thanks for addressing the columnTypes count), but this bundles three behavior changes in core bun:sqlite binding C++ — including the BigInt range-check change you flagged as a maintainer call — so a human should sign off.

Extended reasoning...

Overview

This PR changes src/jsc/bindings/sqlite/JSSQLStatement.cpp (the core bun:sqlite parameter-binding and result-shaping path), lazy_sqlite3.h (adds sqlite3_stmt_status to the macOS dlsym table, removes sqlite3_bind_text16), the safeIntegers docs, and two test files. It fixes three distinct bugs: (1) string parameters now always go through StringView::utf8()sqlite3_bind_text instead of sqlite3_bind_text16, so lone surrogates become U+FFFD rather than ill-formed UTF-8; (2) out-of-range BigInt parameters now throw RangeError regardless of safeIntegers, instead of silently wrapping mod 2^64; (3) cached column names / result Structures are rebuilt after SQLite transparently re-prepares a statement, detected via sqlite3_stmt_status(SQLITE_STMTSTATUS_REPREPARE) after each sqlite3_step(). A follow-up commit (b1fbe53) moved the sqlite3_column_count read in columnTypes to after the step per my earlier comment.

Security risks

None apparent. Inputs are JS values bound to SQLite statements; the changes tighten encoding correctness and add a range check. No auth, crypto, permissions, filesystem, or network surface is touched. The new dlsym for sqlite3_stmt_status follows the existing pattern for other symbols.

Level of scrutiny

High. rebindValue and the six sqlite3_step() call sites are the hot path for every bun:sqlite query. Two of the three fixes are observable behavior changes: non-ASCII 16-bit strings now take a UTF-8 transcode on bind (correctness over the old zero-copy UTF-16 path), and the BigInt change converts a silent wrap into a thrown RangeError — the author explicitly asked whether the previous wrapping was intentional. The reprepare detection adds a sqlite3_stmt_status(..., 1) call after every step and folds six copies of the cache-refresh check into one helper; the refactor looks faithful but is non-mechanical.

Other factors

The PR is well-tested (24 new/updated cases in sqlite.test.js, including cross-process and second-connection schema changes) and well-documented. CodeRabbit's two minor nits and my columnTypes finding are all resolved. CI for commit 3adfc2e was marked failing in the buildkite comment, which a maintainer should also glance at. Given the scope, the explicit "if this was intentional, say so" on the BigInt change, and the perf-vs-correctness tradeoff on the text16 path, this is a maintainer-level decision rather than a rubber-stamp.

@robobun

robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author

CI status for b1fbe53 (build #65440):

  • 284 jobs passed, 0 test failures. No sqlite test failed on any lane.
  • darwin-26-aarch64-test-bun passed, so the macOS lazy-dlopen("libsqlite3.dylib") path (the one lane where sqlite3_stmt_status is resolved at runtime rather than linked) is verified.
  • The only red is darwin-14-aarch64-test-bun, which expired waiting for an agent and never ran a test. Buildkite already auto-retried it; the retries are queued behind macOS-14-aarch64 agent capacity. I'm not pushing a ci: retrigger because that would cancel the 284 green jobs and the queued retry only to rejoin the same agent queue.
  • The two warning annotations are known-flaky Windows install tests (update_interactive_install.test.ts, bun-install.test.ts) that passed on retry; unrelated to this change.

The diff is green on every lane that actually ran. Ready for review; the BigInt range-check change (silent wrap → RangeError regardless of safeIntegers) is the one deliberate behavior change I've flagged for a maintainer call.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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

Inline comments:
In `@docs/runtime/sqlite.mdx`:
- Around line 487-489: The SQLite integer range description in the runtime docs
is inaccurate; update the wording in the Database/safeIntegers section to use
JavaScript’s safe-integer bound instead of “52-bit.” Replace the claim about
“signed 52-bit integers” with `Number.MAX_SAFE_INTEGER` (`2^53 - 1`) and keep
the guidance that `bigint` should be used for larger values when referencing
`bun:sqlite` integer handling.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b0898341-4197-4aef-bb91-4c2090a9f093

📥 Commits

Reviewing files that changed from the base of the PR and between b1fbe53 and b2b7b5f.

📒 Files selected for processing (1)
  • docs/runtime/sqlite.mdx

Comment thread docs/runtime/sqlite.mdx

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No further issues after the columnTypes fix — deferring to a maintainer for the flagged BigInt wrap→RangeError behavior change and the native-bindings scope.

Extended reasoning...

Overview

This PR fixes three distinct bun:sqlite binding-layer bugs in src/jsc/bindings/sqlite/JSSQLStatement.cpp and lazy_sqlite3.h: (1) lone-surrogate strings now encode to well-formed UTF-8 via WTF::StringView::utf8() instead of being handed to sqlite3_bind_text16; (2) out-of-range BigInt parameters now always throw RangeError rather than silently wrapping modulo 2⁶⁴ when safeIntegers is off; (3) cached column names/result Structure are rebuilt after SQLite transparently re-prepares a statement, detected via sqlite3_stmt_status(SQLITE_STMTSTATUS_REPREPARE). It also moves the columnTypes column-count read to after sqlite3_step() (addressing my earlier inline comment), updates docs, and adds extensive tests.

Security risks

None identified. The changes tighten behavior (reject bad input, sanitize ill-formed text) rather than loosen it. No auth, crypto, filesystem, or permission surfaces are touched. The new sqlite3_stmt_status dlsym entry mirrors the existing pattern for other lazily-loaded symbols.

Level of scrutiny

This warrants human review. It modifies hot-path native C++ in a widely-used built-in module across six sqlite3_step() call sites, removes the safeIntegers parameter from the rebindValue/rebindObject/rebindStatement chain, and changes how all non-ASCII/16-bit strings are bound (always re-encoding to UTF-8 rather than passing UTF-16 through). Most importantly, fix (2) is an intentional breaking behavior change that the author has explicitly flagged for a maintainer decision ("If the silent wrapping was intentional, say so and I will revert this part").

Other factors

The bug-hunting pass found no issues. All prior review threads (CodeRabbit nits, my columnTypes note) are resolved, CI is green on every lane that ran, and test coverage for the new behavior is thorough (boundary BigInts, surrogate cases across positional/named/exec paths, same-process/cross-connection/cross-process schema changes, columnTypes grow/shrink). The implementation looks correct to me, but the combination of a flagged behavior-change decision and ~100 lines of native-bindings refactor puts this outside what I'll approve without a human sign-off.

@robobun

robobun commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

CI status for b2b7b5f0 (build #67172): 282 of 286 jobs passed, and every bun:sqlite test passes on every lane, including the macOS lanes that resolve sqlite3_stmt_status from the system libsqlite3.dylib at runtime.

The 4 red jobs are all unrelated to this change:

  1. test/js/node/test/parallel/test-net-connect-memleak.js (alpine 3.23 x64 and x64-baseline): a ported Node.js GC-collection test (onGC + globalThis.gc(), asserting a closure was collected) that never loads bun:sqlite. It is currently failing across unrelated branches: the same error annotation appears on 7 of the 8 other recent failed builds on this pipeline (#67183, #67181, #67176, #67171, #67163, #67161, #67160).
  2. test/js/bun/terminal/terminal.test.ts > "creates subprocess with terminal attached" (darwin 14 x64): a Bun.spawn + PTY test timing out at 90s while the other 88 tests in the file pass. It also fails on an unrelated branch in #67161.
  3. One darwin 26 aarch64 - test-bun shard: buildkite-agent artifact download timed out after 120s. Zero tests ran, and the sibling darwin-26-aarch64 shard passed.

I am not pushing a ci: retrigger: with test-net-connect-memleak.js red on 7 of the 8 other recent failed builds across unrelated branches, a re-roll would almost certainly reproduce the same unrelated failure and just add commit noise. The diff is green on every lane that exercises it, all review threads are resolved, and the PR is ready for review. The one deliberate behavior change (an out-of-range bound bigint now throws RangeError regardless of safeIntegers, instead of silently wrapping modulo 2^64) is flagged in the description for a maintainer call.

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

@robobun benchmark the before/after this PR when loading a large table. Release build. Update PR body

@robobun

robobun commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

@Jarred-Sumner done, added a Performance section to the PR body with the table, methodology, and the benchmark script.

Headline: loading a large table is unchanged within noise.

benchmark before after Δ
SELECT * FROM t .all() (200,000 rows × 5 cols) 97.43 ms 98.63 ms +1.24%
SELECT * FROM t .values() (200,000 rows × 5 cols) 96.52 ms 96.94 ms +0.44%
SELECT * FROM s .all() (100 rows × 3 cols) 21.5 µs 21.4 µs -0.43%
INSERT 20,000× (2×ASCII str + int) — unchanged control 23.30 ms 23.43 ms +0.57%
INSERT 20,000× (2×16-bit str + int) — bind_text16utf8() 27.16 ms 27.60 ms +1.64%
INSERT 20,000× (2×BigInt + int) — unconditional range check 24.13 ms 23.46 ms -2.77%

Release build (bun-profile), mitata, 3 interleaved rounds, Xeon 8375C. Both binaries built from the same tree (this branch's merge-base ffea69ae7c); the only difference is this PR's two src/jsc/bindings/sqlite/ files.

Why the read path is flat: the new sqlite3_stmt_status(…, SQLITE_STMTSTATUS_REPREPARE, 1) call runs once per .all()/.values() invocation, not per row, so there is no added per-row work in the step loop. mitata's per-run min–max on the 200K .all() was 90–106 ms (±8%), well wider than the +1.2% delta.

Why the 16-bit bind path is flat: sqlite3_bind_text16 on a UTF-8 database was already transcoding UTF-16→UTF-8 inside SQLite; the work just moves to WTF's simdutf-backed StringView::utf8(). The residual ~1% over the unchanged ASCII control is one extra CString temporary that SQLITE_TRANSIENT then copies again; happy to eliminate that with the lambda form of tryGetUTF8 if you want it gone.

Separately, this branch is stale against main again (several bun:sqlite/node:sqlite PRs landed since ffea69ae7c, including #32498 and #34925). The benchmark above is a clean A/B of exactly this diff regardless, but I will rebase next.

robobun added 3 commits July 31, 2026 03:23
… and stale column names

Strings containing a lone surrogate were bound with sqlite3_bind_text16,
letting SQLite's UTF-16 decoder pair a lone high surrogate with whatever
code unit follows (consuming it) and store trailing ones as ill-formed
3-byte sequences. Always convert to UTF-8 with StringView::utf8(), which
replaces lone surrogates with U+FFFD like TextEncoder, and bind with
sqlite3_bind_text. sqlite3_bind_text16 is no longer used, so its entry is
removed from lazy_sqlite3.h.

BigInt parameters were only range-checked when safeIntegers was enabled;
the default mode called JSBigInt::toBigInt64, which truncates to 64 bits.
Always range-check and throw the same RangeError. The now-unused
safeIntegers parameter is removed from the rebind chain.

A prepared statement's cached column names only refreshed when a
per-connection version counter changed, and the db.run()/exec() path
never bumps it, so ALTER/DROP+CREATE issued that way returned values
under stale (or wrong) column names. Detect the re-prepare directly via
sqlite3_stmt_status(SQLITE_STMTSTATUS_REPREPARE) after each sqlite3_step
and fold the six copies of the refresh check into one helper.
Issue 1332's repro is an ALTER TABLE issued by a separate process while
the statement's connection has WAL enabled, which the version counter
could never observe. The re-prepare detection handles it; add a test
covering that exact scenario.

Also collapse a duplicated paragraph in the integers doc section and
shorten a test comment to three lines.
sqlite3_step() transparently re-prepares an expired statement, which can
change the result shape. The columnTypes getter captured the column
count before the step, so after a schema change the returned array was
truncated (more columns) or padded with spurious "NULL" entries read
from out-of-range column indexes (fewer columns).
@robobun
robobun force-pushed the farm/153748d0/sqlite-bind-column-cache-fixes branch from b2b7b5f to 4b3c09f Compare July 31, 2026 03:34
Comment thread src/jsc/bindings/sqlite/JSSQLStatement.cpp Outdated
Comment thread src/jsc/bindings/sqlite/JSSQLStatement.cpp Outdated
Comment thread src/jsc/bindings/sqlite/JSSQLStatement.cpp Outdated
Comment thread src/jsc/bindings/sqlite/JSSQLStatement.cpp Outdated
Comment thread src/jsc/bindings/sqlite/JSSQLStatement.cpp Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

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

⚠️ Outside diff range comments (2)
src/jsc/bindings/sqlite/JSSQLStatement.cpp (2)

2180-2181: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Reset stmt before returning from metadata initialization failures.

initializeColumnNames can leave columnNames and validColumns partially rebuilt. These three paths return with an active SQLite statement. In iterate, the next call can skip reset and use partial metadata. Reset stmt before each RETURN_IF_EXCEPTION at lines 2180-2181, 2230-2231, and 2321-2322.

🤖 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 `@src/jsc/bindings/sqlite/JSSQLStatement.cpp` around lines 2180 - 2181, In
src/jsc/bindings/sqlite/JSSQLStatement.cpp at lines 2180-2181, 2230-2231, and
2321-2322, update each initializeColumnNames metadata-failure path to reset stmt
before RETURN_IF_EXCEPTION executes. Ensure all three sites leave the SQLite
statement reset when metadata rebuilding throws, preventing iterate from reusing
partial metadata.

Source: Coding guidelines


855-861: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Make metadata refresh transactional.

initializeColumnNames commits hasExecuted and version before allocation-capable work completes. If construction raises, it can leave _structure, _prototype, columnNames, or validColumns incomplete. updateColumnNamesIfNeeded also clears SQLITE_STMTSTATUS_REPREPARE before initialization succeeds. The next call can skip refresh and use invalid metadata.

Build metadata in temporary state and commit it only after success, or restore the previous cache state on failure.

🤖 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 `@src/jsc/bindings/sqlite/JSSQLStatement.cpp` around lines 855 - 861, Make
metadata refresh in initializeColumnNames and updateColumnNamesIfNeeded
transactional: stage all allocation-capable metadata changes and
hasExecuted/version updates in temporary state, then commit only after
successful completion; on failure, preserve the prior _structure, _prototype,
columnNames, validColumns, and execution state. Ensure
SQLITE_STMTSTATUS_REPREPARE is not consumed permanently until initialization
succeeds, so a failed refresh is retried on the next call.

Sources: Coding guidelines, MCP tools

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

Inline comments:
In `@src/jsc/bindings/sqlite/JSSQLStatement.cpp`:
- Around line 918-919: Update both string-binding branches in the relevant
JSSQLStatement binding logic to use sqlite3_bind_text64 with the UTF-8 encoding
flag and SQLITE_TRANSIENT, passing the full utf8 length without narrowing.
Ensure the existing CHECK_BIND handling is preserved and no branch continues
using the 32-bit sqlite3_bind_text API.

---

Outside diff comments:
In `@src/jsc/bindings/sqlite/JSSQLStatement.cpp`:
- Around line 2180-2181: In src/jsc/bindings/sqlite/JSSQLStatement.cpp at lines
2180-2181, 2230-2231, and 2321-2322, update each initializeColumnNames
metadata-failure path to reset stmt before RETURN_IF_EXCEPTION executes. Ensure
all three sites leave the SQLite statement reset when metadata rebuilding
throws, preventing iterate from reusing partial metadata.
- Around line 855-861: Make metadata refresh in initializeColumnNames and
updateColumnNamesIfNeeded transactional: stage all allocation-capable metadata
changes and hasExecuted/version updates in temporary state, then commit only
after successful completion; on failure, preserve the prior _structure,
_prototype, columnNames, validColumns, and execution state. Ensure
SQLITE_STMTSTATUS_REPREPARE is not consumed permanently until initialization
succeeds, so a failed refresh is retried on the next call.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fc2d3b8d-4310-4d67-9819-663dddaa6918

📥 Commits

Reviewing files that changed from the base of the PR and between 4b3c09f and 147ddd5.

📒 Files selected for processing (1)
  • src/jsc/bindings/sqlite/JSSQLStatement.cpp

Comment thread src/jsc/bindings/sqlite/JSSQLStatement.cpp Outdated
@robobun

robobun commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

Post-rebase review sweep on 31e6a6a:

  • sqlite3_bind_text64 (CodeRabbit inline): done in 31e6a6a. Both string branches now call sqlite3_bind_text64(..., SQLITE_TRANSIENT, SQLITE_UTF8), matching NodeSqlite.cpp; no more size_tint narrowing on the length.
  • "Reset stmt before RETURN_IF_EXCEPTION in iterate/all/get" (CodeRabbit outside-diff): declined. Those three sites are byte-for-byte the same as current main (initializeColumnNames(...); RETURN_IF_EXCEPTION(scope, {}); with no reset); the refactor into updateColumnNamesIfNeeded preserved each site's existing exception-handling shape exactly. The other three sites (values/raw/run) already reset on main, so the inconsistency is pre-existing and out of scope for this PR.
  • "Make initializeColumnNames transactional" (CodeRabbit outside-diff, tagged "Heavy lift"): declined. initializeColumnNames on main already commits hasExecuted and the version counter at the top before its allocation-capable body (pre-existing). The only throw path is allocation failure building the result Structure/Identifiers, which means OOM; under OOM a subsequent call is not recovering anyway. Consuming the SQLITE_STMTSTATUS_REPREPARE counter before that body is the same commit-before-body shape, not a new failure mode. A proper staged-commit rewrite of initializeColumnNames is a standalone refactor.

Comment thread src/jsc/bindings/sqlite/lazy_sqlite3.h Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SQLite schema cache is not properly invalidated

2 participants