Conversation
|
Updated 9:38 AM PT - Jul 7th, 2026
❌ @robobun, your commit 6f31d15 has some failures in 🧪 To try this PR locally: bunx bun-pr 33307That installs a local version of the PR into your bun-33307 --bun |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSQLite close handling now finalizes tracked prepared statements on ChangesSQLite close() statement lifecycle
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/runtime/sqlite.mdx (1)
113-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocs accurately match the new close() semantics. Minor wording nit on line 131.
✏️ Suggested wording tweak
-Using a statement after the database it came from was closed throws `Database has closed`. +Using a statement after its database has been closed throws `Database has closed`.🤖 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 `@docs/runtime/sqlite.mdx` around lines 113 - 136, The `Database.close()` docs already reflect the new semantics, but the wording around statement finalization is a bit awkward. Update the explanatory text in the `close(throwOnError: boolean = false)` section to use clearer phrasing for the behavior of prepared statements when `Database.close(false)` is called, while keeping the `Database` and `close(true)` examples unchanged.
🤖 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 191-203: `jsSQLStatementToStringFunction` and
`jsSqlStatementGetColumnNames` bypass the existing statement-lifecycle guard and
can dereference a finalized or closed statement; add the same `CHECK_PREPARED`
protection used by the rest of `JSSQLStatement.cpp` before calling
`sqlite3_expanded_sql` or `sqlite3_column_count`. Use the existing `castedThis`,
`stmt`, and `version_db` checks to reject post-close access consistently with
other methods, and make both functions return the same thrown error path as the
prepared-statement API.
In `@test/js/bun/sqlite/sqlite.test.js`:
- Around line 1391-1409: The “every statement entry point” test is missing
several `PreparedStatement` APIs, so extend the `close() with a statement still
alive` case to cover `prepared.columns`, `prepared.toString()`,
`prepared.raw()`, `prepared.columnsCount`, and `prepared.safeIntegers` in
addition to the existing methods. Use the `Database` and `prepared` setup
already in this test, and add assertions that each of these entry points now
throws after `db.close()`, so the coverage matches the test name and catches the
closed-database guard gap.
---
Outside diff comments:
In `@docs/runtime/sqlite.mdx`:
- Around line 113-136: The `Database.close()` docs already reflect the new
semantics, but the wording around statement finalization is a bit awkward.
Update the explanatory text in the `close(throwOnError: boolean = false)`
section to use clearer phrasing for the behavior of prepared statements when
`Database.close(false)` is called, while keeping the `Database` and
`close(true)` examples unchanged.
🪄 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: 0c0ac6c6-037b-44ee-8212-7d3ee24f5627
📒 Files selected for processing (3)
docs/runtime/sqlite.mdxsrc/jsc/bindings/sqlite/JSSQLStatement.cpptest/js/bun/sqlite/sqlite.test.js
|
Checked both; this PR closes neither, so I'm leaving the description as is. #29494 ( #11418 ( Making |
There was a problem hiding this comment.
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 133-136: Clarify the GC behavior in the SQLite docs note so it
does not imply the connection is fully released when the database is garbage
collected. Update the wording around the database/connection lifecycle in the
`<Note>` text to state that GC only releases the database object itself, while
outstanding statements may continue to keep the underlying connection/file alive
until they are finalized or collected; keep the distinction from `close()`
explicit.
🪄 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: 22ecd71d-bcd2-47de-9fe2-2e64d4b203cc
📒 Files selected for processing (3)
docs/runtime/sqlite.mdxsrc/jsc/bindings/sqlite/JSSQLStatement.cpptest/js/bun/sqlite/sqlite.test.js
There was a problem hiding this comment.
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 `@packages/bun-types/sqlite.d.ts`:
- Around line 274-283: The `Database.close` doc comment has free-text paragraphs
after the `@example`, which is inconsistent with `run`, `query`, and `prepare`
and can render poorly in TypeDoc/IDE tooltips. Reorder the JSDoc so the
garbage-collection and `sqlite3_close_v2` prose appears before the `@example`
block, and keep `@example` as the final tag in the `close` documentation.
🪄 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: 4b9871f4-d655-470c-b981-f7a714afd48f
📒 Files selected for processing (1)
packages/bun-types/sqlite.d.ts
CI status: green on the diff, infra failures on
|
| build | result | the one failure |
|---|---|---|
68216 (f4f4ba40) |
282 pass, 1 fail | proxy-stress-concurrent.test.ts fixture can't resolve a harness import on agent macOS-13-x64-1 (stale checkout) |
68221 (017768d7) |
285 pass, 1 fail | buildkite-agent artifact download timed out after 120s on agent darwin-aarch64-26-5-1-1, no tests ran |
69849 (6f31d158, rebased) |
285 pass, 1 fail | buildkite-agent artifact download timed out after 120s on agent darwin-aarch64-26-5-1-1, no tests ran |
The last two are the same agent timing out at the same line. test/js/bun/sqlite/sqlite.test.js itself ran 88 tests and passed on every lane that picked it up.
I am not pushing another retrigger. This needs a maintainer to merge through the flake (or to look at why darwin-aarch64-26-5-1-1 keeps dropping artifact downloads).
…ng them live sqlite3_close_v2() only marks the connection closed while prepared statements are still alive, and close() nulled VersionSqlite3::db without touching them. Only the write and prepare entry points checked for the null handle, so get()/all()/values()/iterate() kept stepping the live sqlite3_stmt* and returned rows from a closed database, and the database file stayed open until the statements were garbage collected. Track the statements prepared against each connection and finalize them in close(false), the way the query cache already does, and report "Database has closed" from every statement entry point once the connection is gone. close(true) still refuses to close while statements are outstanding, but says so instead of reporting "database is locked".
A parameter getter can close the database while db.run() is binding. The statement it prepared internally is then the last one holding the connection open, so finalizing it at the end of the loop iteration frees the sqlite3* that the function still holds in a local: the next sqlite3_prepare_v3(), or the trailing sqlite3_total_changes(), reads freed memory. The Statement entry points re-validate after binding, but db.run() passes a null JSSQLStatement to rebindStatement() so those guards never fire. Re-read the handle instead, the way Statement#run already does.
017768d to
6f31d15
Compare
There was a problem hiding this comment.
All prior review feedback has been addressed and I didn't find anything new, but this changes close()'s user-visible contract and adds raw-pointer statement tracking across a GC destructor path — worth a maintainer sign-off on the API semantics and lifetime model.
Extended reasoning...
Overview
This PR changes bun:sqlite's Database.close() to eagerly finalize every outstanding JSSQLStatement prepared against the connection, rather than relying on sqlite3_close_v2()'s deferred-close semantics. It touches four files: src/jsc/bindings/sqlite/JSSQLStatement.cpp (adds a Vector<JSSQLStatement*> on VersionSqlite3, populates it in JSSQLStatement::create, drains it in ~JSSQLStatement, adds finalizeOutstandingStatements(), extends CHECK_PREPARED to report Database has closed, and rewrites the SQLITE_BUSY message from close(true)), plus matching docs (docs/runtime/sqlite.mdx), type declarations (packages/bun-types/sqlite.d.ts), and ~140 lines of new tests in test/js/bun/sqlite/sqlite.test.js.
Security risks
None identified. The change tightens lifecycle handling rather than loosening it, and the input surface (JS-visible close() and statement methods) is unchanged. The one memory-safety hazard I raised earlier — a UAF in db.run() when a bindings getter closes the database mid-bind — was confirmed under ASAN by the author and the guard for it has since landed on main via #33072; this PR keeps that guard and adds a comment plus test coverage. The Vector<JSSQLStatement*> is non-owning, entries are added/removed on the JS thread only, and finalizeOutstandingStatements() swap-clears before iterating so a re-entrant ~JSSQLStatement running removeFirst on the (now-empty) member vector is safe.
Level of scrutiny
This warrants maintainer review. It is a behavioral change to a public API: code that previously kept using a prepared statement after db.close() (which half-worked — read paths returned rows) will now throw Database has closed. That's the correct behavior and matches better-sqlite3/node:sqlite, but it's still a user-visible contract change. It also adds raw-pointer bookkeeping across a GC-driven destructor (~JSSQLStatement → version_db->statements.removeFirst(this)), which is exactly the class of change CLAUDE.md's "Native code: memory safety" section flags as most-scrutinized. The implementation looks correct to me after several rounds, but a maintainer should own the sign-off on both the API decision and the lifetime model.
Other factors
The PR has been through substantial review iteration: two CodeRabbit threads (both resolved with reasoned pushback on toString()/columnNames staying unguarded), and four of my own inline comments — the db.run() UAF (fixed and now on main), the stale GC Note in docs (reworded twice), the O(N²) removeFirst in the destructor (benchmarked and left as-is with data showing the Vector beats a HashSet at every reachable N), and the stale .d.ts JSDoc (synced). All threads are resolved. Test coverage is thorough (every statement entry point after close, WAL sidecar release, mid-iteration close, close-during-bind for both Statement and db.run(), post-close GC safety, inspectability), and the author verified the suite fails on main and passes on the branch under ASAN. The only remaining call is whether the API-semantics change and the pointer-tracking design are what the maintainers want — that's not mine to make.
…ite3_next_stmt (oven-sh#36573) ## Repro ```js import { Database } from "bun:sqlite"; const db = new Database(":memory:"); db.run("create table t (a integer)"); // 21 distinct SQL strings, each run through db.query() for (let i = 0; i < 21; i++) db.query(`select a + ${i} as v from t`).all(); db.close(true); // error: database is locked ``` The threshold is exactly `Database.MAX_QUERY_CACHE_SIZE` (default 20). With a file-backed database and non-strict `close()`, nothing throws but the file handle stays open until GC, which on Windows makes the database file undeletable right after closing it. ## Cause `query()` only tracks the first `MAX_QUERY_CACHE_SIZE` distinct statements; later ones are prepared but stored nowhere, so `close()` could not finalize them and `sqlite3_close()` returned `SQLITE_BUSY`. ## Fix Per review, the mechanism mirrors better-sqlite3's `CloseHandles()` and node:sqlite's `FinalizeStatements()`: - `VersionSqlite3` keeps a list of live `JSSQLStatement` wrappers, linked at creation (next to the existing `reference_count` increment) and unlinked in the destructor. - `close()` walks that list, `sqlite3_finalize`s each statement and nulls the wrapper's handle, then runs a `sqlite3_next_stmt()` sweep as a backstop for statements not owned by a wrapper (e.g. the transient statement in `db.run()` when a bound-parameter getter closes the database mid-bind), then calls `sqlite3_close()`, which can no longer return `SQLITE_BUSY`. - Because a nulled handle is indistinguishable from an explicitly finalized statement, the existing "Statement has finalized" re-validation paths cover all post-close use; no new per-statement state. The bind paths re-check the connection after running user code, and `raw()`'s push-exception path no longer resets a statement that user code may have finalized. - If `sqlite3_close()` somehow still fails, the handle is retired via `sqlite3_close_v2()` so the connection never ends up half-alive. The termination path and GC `release()` still use `sqlite3_close_v2()` without touching the list, so their outstanding statements stay valid until each is finalized, as before. Behavior change, reflected in docs and types: `close(true)` no longer throws `database is locked` over outstanding prepared statements; it finalizes them, matching better-sqlite3 and node:sqlite. Using a statement after close throws `Statement has finalized`, matching node. ## Verification Without the fix: ``` $ USE_SYSTEM_BUN=1 bun test test/js/bun/sqlite/sqlite.test.js -t 36572 (fail) close(true) finalizes query() statements created after the cache filled up (oven-sh#36572) error: database is locked (fail) close(true) finalizes query() statements past the cache limit that are still referenced (oven-sh#36572) ``` With the fix: `bun bd test test/js/bun/sqlite/` 115 pass, `test/js/node/sqlite/` 115 pass, `test/regression/issue/14709.test.ts` 5 pass, bun-types 14 pass. Fixes oven-sh#36572 Closes oven-sh#35604 Closes oven-sh#33307 Closes oven-sh#11418 <!-- robobun:evidence:begin --> --- **no test proof** · iteration 4 · 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:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Repro
One statement object half-works after
close(): the read paths return rows, the write path throws.close(true)with an outstanding statement reportsdatabase is locked.Cause
close()callssqlite3_close_v2(), which only defers the close while prepared statements are outstanding, then nullsVersionSqlite3::db. Only the write and prepare entry points check for the null handle, soget()/all()/values()/iterate()keep stepping the still-livesqlite3_stmt*, and SQLite keeps the whole connection (and the database file) open until the last statement is finalized. Forclose(true),sqlite3_close()returnsSQLITE_BUSY, whose generic error string blames a locking conflict rather than the unfinalized statements.Fix
VersionSqlite3now tracks the statements prepared against it, andclose(false)finalizes them, the same wayclearQueryCache()already finalizes the cached ones.CHECK_PREPAREDreportsDatabase has closedonce the handle is gone, so every statement entry point fails instead of justrun().close(true)still refuses to close while statements are outstanding, which is its documented contract, but now says why.sqlite3_close()returnsSQLITE_BUSYonly for unfinalized statements and backups, andbun:sqliteexposes no backup API.Two things deliberately keep working after
close(), exactly as they do afterstmt.finalize()today, because neither reads the closed database:columnNames, which is cached from the statement's last executiontoString(), which backs the statement'sSymbol.toStringTag(src/js/bun/sqlite.ts:172), so making it throw would breakconsole.log(stmt)The database object being garbage collected while statements are alive is unchanged: that path is refcounted through
VersionSqlite3::release()and the statements keep working, ascan continue to use existing statements after database has been GC'dcovers.Rebase note
The
db.run()close-during-bind use-after-free this originally also fixed (a binding getter callingdb.close()frees thesqlite3*mid-call) has since landed onmainvia #33072, with the same guard. The conflict resolution keepsmain's condition and this PR's comment explaining why thesqlite3*is freed at that point. Therejects db.run(%p) when a binding getter closes the databasetests here still exercise that guard but also assert that the bystander statement throwsDatabase has closedafterwards, which is this PR'sCHECK_PREPAREDchange.Verification
bun bd test test/js/bun/sqlite/— 106 pass, 0 fail. The new and updated assertions fail onmain:git checkout origin/main -- src/ && bun bd test test/js/bun/sqlite/sqlite.test.js -t "statement still alive"plus
close(true) should throw an error if the database is in useandshould dispose AND throw an error if the database is in use, which assert the new message. The debug build runs under ASAN.test/js/sql/sqlite-sql.test.tshas two tests that time out in debug+ASAN builds (properly finalizes prepared statements,handles exotic but valid SQL patterns). Both time out identically onmain.