bun:sqlite: finalize outstanding prepared statements on close via sqlite3_next_stmt

[robobun]

Repro

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_finalizes 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 (#36572)
  error: database is locked
(fail) close(true) finalizes query() statements past the cache limit that are still referenced (#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 #36572

Closes #35604
Closes #33307
Closes #11418

---

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

[coderabbitai]

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

Changes

SQLite close lifecycle

Layer / File(s)	Summary
Close finalization and invalidation packages/bun-types/sqlite.d.ts, docs/runtime/sqlite.mdx, src/jsc/bindings/sqlite/JSSQLStatement.cpp	Database.close() now finalizes tracked statements, retires the connection, and prevents duplicate finalization. Documentation describes the updated behavior.
Statement binding and execution safety src/jsc/bindings/sqlite/JSSQLStatement.cpp	Binding, rebinding, iteration, and one-off execution detect statements finalized during database closure.
Closure behavior validation test/js/bun/sqlite/sqlite.test.js	Tests cover cached and uncached statements, forced closure, file release, disposal, garbage-collected statements, concurrency, and raw-row callback closure.

Possibly related PRs

• oven-sh/bun#36712: This PR also changes SQLite statement APIs and lifecycle handling in JSSQLStatement.cpp.

Suggested reviewers: alii, jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address issue #36572 by finalizing uncached and tracked statements so close() no longer fails after the query-cache limit.
Out of Scope Changes check	✅ Passed	The implementation, documentation, type updates, and tests directly support the linked issue and stated cleanup objectives.
Title check	✅ Passed	The title clearly summarizes the main change: finalizing outstanding SQLite prepared statements during close.
Description check	✅ Passed	The description explains the issue, cause, fix, behavior change, and verification results, covering the template requirements.

---

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

[robobun]

^{Updated 7:05 AM PT - Aug 2nd, 2026}

❌ @robobun, your commit 6645477 has some failures in Build #87832 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 36573

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

bun-36573 --bun

[github-actions]

Found 1 issue this PR may fix:

1. bun:sqlite memory growth with dynamic query text can OOM 1GB container (in-memory DB) #28911 - PR's WeakRef-based tracking and FinalizationRegistry for overflow prepared statements directly addresses the unbounded memory growth from calling db.query() with many distinct SQL strings exceeding MAX_QUERY_CACHE_SIZE

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

Fixes #28911

🤖 Generated with Claude Code

[robobun]

This does not fix #28911. The overflow statements tracked here were already collectable by GC before this change (the WeakRef tracking keeps that behavior, it only lets close() finalize the survivors). The memory growth in #28911 comes mainly from the first MAX_QUERY_CACHE_SIZE huge statements staying pinned in the cache until close(), which this PR does not change; #28912 addresses that.

[github-actions]

This PR may be a duplicate of:

1. bun:sqlite: close() finalizes outstanding prepared statements (drizzle close(true) "database is locked") #35604 - Also fixes "database is locked" on close() by tracking and finalizing all outstanding prepared statements
2. bun:sqlite: close() finalizes outstanding statements instead of leaving them live #33307 - Earlier version of the same fix, tracking statements and finalizing them on close()

🤖 Generated with Claude Code

[robobun]

Not a duplicate, though they are related. #35604 and #33307 track every statement prepared on the connection natively (for #11418), which changes the close(true) contract for user db.prepare() statements: #35604 makes close(true) finalize them instead of throwing, #33307 keeps the throw for close(true). This PR is scoped to the statements query() creates internally once the cache is full, which close() is already documented to clean up, and leaves the db.prepare() behavior exactly as it is today. It applies independently of whichever of those two lands.

[claude]

I didn't find any bugs, but this PR makes a couple of design choices that a maintainer should weigh in on:

• close(true) now triggers a full synchronous GC on the SQLITE_BUSY path before retrying. That changes the documented contract from "throws while any prepared statement exists" to "throws while any reachable prepared statement exists", and adds full-heap-GC latency to the error path.
• The JS-side WeakRef + FinalizationRegistry tracking overlaps with the native per-database statement tracking being added in #35604 / #33307; worth deciding whether this should land as-is or fold into that.

What was reviewed:

• Double-finalize on already-finalized overflow statements — native finalize() null-checks stmt, so it's idempotent.
• FinalizationRegistry callback capturing this — the Database↔registry cycle is self-contained and collectable.
• clearQueryCache() now finalizing uncached statements — consistent with how it already treats cached ones.
• The existing close(true) should throw / should dispose AND throw tests keep the prepared statement reachable past the close call, so they still throw after the GC-and-retry.

Extended reasoning...

Overview

The PR fixes #36572: db.query() statements created after MAX_QUERY_CACHE_SIZE fills up were never tracked, so close(true) failed with SQLITE_BUSY. It touches three files: src/js/bun/sqlite.ts adds a Set<WeakRef<Statement>> pruned by a FinalizationRegistry for overflow statements and finalizes them in clearQueryCache(); src/jsc/bindings/sqlite/JSSQLStatement.cpp adds a full synchronous GC + retry on the strict close path when sqlite3_close() returns SQLITE_BUSY; and four new tests in test/js/bun/sqlite/sqlite.test.js.

Security risks

None identified. This is lifecycle bookkeeping for internally-created SQLite statements; no untrusted input parsing, no auth/crypto/permission surface.

Level of scrutiny

Medium-high. The JS-side change is straightforward, but the C++ change alters the observable semantics of a documented API: close(true) now runs vm.heap.collectNow(Sync, Full) when the first close attempt reports busy, which means (a) a user holding a db.prepare() result that is no longer reachable will now get a successful close instead of the documented error, and (b) the error path now pays for a full-heap GC. The pattern of calling collectNow from a host function is used elsewhere (Bun.gc(true), BunDebugger.cpp), so it's mechanically safe, but relying on GC reachability to preserve an API contract is the kind of choice a maintainer should sign off on.

Other factors

• The PR itself notes that #35604 and #33307 are adding native per-database statement tracking for #11418. That work would let close() finalize statements deterministically without WeakRef/FinalizationRegistry machinery or a GC-and-retry, so a maintainer may prefer to align this fix with that approach rather than land parallel JS-side bookkeeping.
• I verified the native jsSQLStatementFunctionFinalize is idempotent (null-checks castedThis->stmt), so the "already finalized" test path is safe. I also confirmed sqlite3_close() leaves the connection valid on SQLITE_BUSY, so retrying after GC is sound.
• clearQueryCache() now finalizes overflow statements too. This is a public-API behavior change but is consistent with how it already treats cached statements, so it seems fine.
• Tests look solid — they cover the drop-reference, keep-reference, GC-before-close, and pre-finalized variants, and the existing close(true) should throw tests are preserved.

Deferring to human review for the GC-and-retry design decision and coordination with the in-flight native tracking work.

[robobun]

On the two design points:

• The GC runs only on the strict close error path, where close(true) today always throws. The success path and non-strict close() pay nothing. And the contract shift is narrower than "reachable vs exists": a statement that is unreachable and unswept is one GC away from being finalized anyway, so whether close(true) throws for it was already timing-dependent (the same program throws or succeeds depending on when the sweeper last ran). The retry removes that nondeterminism rather than loosening the contract for statements the caller still holds, which keep throwing as before.
• Agreed this overlaps with the native tracking in bun:sqlite: close() finalizes outstanding prepared statements (drizzle close(true) "database is locked") #35604/bun:sqlite: close() finalizes outstanding statements instead of leaving them live #33307. If either of those lands first, the JS-side weak tracking here reduces to redundant bookkeeping and can be dropped; until then this fixes the query() case without taking a position on the db.prepare() semantics those PRs are deciding.

[Jarred-Sumner]

no just use the sqlite3 method for iterating through the satemetns and ifnalizing all of them

[robobun]

Done. close() now walks the connection with sqlite3_next_stmt() and finalizes everything before sqlite3_close(), in both the strict and non-strict paths. Dropped the WeakRef tracking and the GC retry.

Since that frees sqlite3_stmts whose JS wrappers may still be alive, the wrappers check a finalizedStatementsOnClose flag on VersionSqlite3 and never touch the dangling pointer again: statement calls after close throw "Database has closed", the destructor and finalize() skip the double sqlite3_finalize, and the bind paths re-validate after running user code (a bound-parameter getter closing the database mid-bind used to leave the statement zombie-valid, now it is freed). The exit-time path and GC release() still use sqlite3_close_v2 and are unchanged.

Behavior change that follows from this: close(true) no longer throws "database is locked" over live db.prepare() statements, it finalizes them (docs and types updated, the two tests asserting the old behavior updated). Full sqlite suite, node:sqlite, and the 14709 regression tests pass.

[coderabbitai]

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 (1)

packages/bun-types/sqlite.d.ts (1)

267-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Clarify "throws an error" claim in both close() docs.

Both docs claim that using a statement after the database closes always throws an error. This is not accurate for every method. toString() and columnNames stay readable after close() by design; they return cached/default data instead of throwing (see jsSQLStatementToStringFunction in src/jsc/bindings/sqlite/JSSQLStatement.cpp).

• packages/bun-types/sqlite.d.ts#L267-L291: Soften "Running queries or using statements after the database has been closed throws an error" to note that most operations throw, but that toString() and columnNames remain intentionally safe.
• docs/runtime/sqlite.mdx#L113-L134: Apply the same clarification to "Using a statement after its database was closed throws an error."

Based on learnings, columnNames and toString() are intentionally safe to read after db.close(), pinned by existing tests, unlike other statement methods that do throw.

🤖 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 `@packages/bun-types/sqlite.d.ts` around lines 267 - 291, Clarify the
post-close behavior in the close() documentation at
packages/bun-types/sqlite.d.ts:267-291 and the corresponding statement-usage
documentation at docs/runtime/sqlite.mdx:113-134: state that most queries and
statement operations throw after the database closes, while columnNames and
toString() remain intentionally safe to read and return cached or default data.

🤖 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 1873-1892: Update the strict close failure path in the connection
close logic around finalizedStatementsOnClose and versionDB->db so a SQLITE_BUSY
result leaves the connection consistently unavailable; prevent prepare() from
creating statements while versionDB->db remains non-null, or track a separate
closing/invalid state checked by prepare(). Preserve finalizedStatementsOnClose
once statement wrappers may hold dangling sqlite3_stmt pointers, and ensure a
later successful close clears the connection state.

---

Outside diff comments:
In `@packages/bun-types/sqlite.d.ts`:
- Around line 267-291: Clarify the post-close behavior in the close()
documentation at packages/bun-types/sqlite.d.ts:267-291 and the corresponding
statement-usage documentation at docs/runtime/sqlite.mdx:113-134: state that
most queries and statement operations throw after the database closes, while
columnNames and toString() remain intentionally safe to read and return cached
or default data.

🪄 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: 5d9e6fe8-0ea3-44b9-8267-ca6fdecf7fb2

📥 Commits

Reviewing files that changed from the base of the PR and between f68e504 and 1360aa4.

📒 Files selected for processing (4)

• docs/runtime/sqlite.mdx
• packages/bun-types/sqlite.d.ts
• src/jsc/bindings/sqlite/JSSQLStatement.cpp
• test/js/bun/sqlite/sqlite.test.js

[coderabbitai]

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 (1)

docs/runtime/sqlite.mdx (1)

115-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Narrow the post-close statement guarantee.

toString() returns an empty string after close-time finalization, and finalize() remains safe to call. The sentence “Using a statement after its database was closed throws an error” is therefore too broad. Document that SQLite-dependent operations throw, while these methods retain their existing behavior.

Suggested wording

-Using a statement after its database was closed throws an error.
+Operations that require the SQLite statement throw after its database is closed. `toString()` returns an empty string and `finalize()` remains safe to call.

🤖 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 115 - 123, Update the post-close
statement behavior text in the database closing documentation to state that
SQLite-dependent statement operations throw after the database is closed, while
toString() returns an empty string and finalize() remains safe to call. Keep the
surrounding close and finalization guidance 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 228-232: Update the connection close path associated with
finalizedStatementsOnClose and JSSQLStatement::stmt so every statement wrapper
is cleared or invalidated before sqlite3_finalize() runs. Ensure analyzeHeap()
and all other statement operations cannot forward the dangling pointer, while
preserving the existing behavior where sqlite3_close_v2() leaves outstanding
statements valid.

---

Outside diff comments:
In `@docs/runtime/sqlite.mdx`:
- Around line 115-123: Update the post-close statement behavior text in the
database closing documentation to state that SQLite-dependent statement
operations throw after the database is closed, while toString() returns an empty
string and finalize() remains safe to call. Keep the surrounding close and
finalization guidance 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: 0321e3dc-0861-4ba4-9bfd-988345d1ab69

📥 Commits

Reviewing files that changed from the base of the PR and between 1360aa4 and 888a3f5.

📒 Files selected for processing (2)

• docs/runtime/sqlite.mdx
• src/jsc/bindings/sqlite/JSSQLStatement.cpp

[claude]

All prior review feedback (including the last CHECK_PREPARED_JIT nit, addressed in 8363183) is now resolved and the bug hunter found nothing new this pass. Deferring to a human because this changes the documented close(true) contract — it now eagerly finalizes every outstanding prepared statement instead of throwing — and threads a new dangling-pointer invalidation flag through native UAF-sensitive paths; a maintainer should sign off on the API-behavior change.

What was reviewed:

• Every sqlite3_stmt* use site checks finalizedStatementsOnClose (CHECK_PREPARED, destructor, finalize, toString, rebind object/array, all/values/raw row loops, execute's auto-destructor).
• Strict-close failure path retires the handle via sqlite3_close_v2 and nulls versionDB->db before throwing, so the state machine stays consistent.
• release()/termination paths still use sqlite3_close_v2 without setting the flag, so GC-driven cleanup keeps outstanding statements valid — matches the pre-PR semantics.
• Docs, .d.ts, and the two rewritten "database is locked" tests all agree with the new contract; #13082 test now asserts the rejection.

Extended reasoning...

Overview

The PR fixes #36572: db.close() could not finalize query() statements created after MAX_QUERY_CACHE_SIZE filled up, so sqlite3_close() returned SQLITE_BUSY (strict close threw; non-strict left the file handle open until GC). The fix walks sqlite3_next_stmt() and finalizes every outstanding sqlite3_stmt on the connection before sqlite3_close(), then marks VersionSqlite3::finalizedStatementsOnClose so surviving JS wrappers never touch their now-dangling stmt pointers. Touches JSSQLStatement.cpp (~90 lines net), sqlite.test.js (7 new tests + 3 rewritten), sqlite.mdx, and sqlite.d.ts.

Security risks

No new external-input parsing or trust boundaries. The primary risk class is native memory safety (UAF on freed sqlite3_stmt*), which the PR addresses by guarding every dereference site with the new flag. Prior review passes found and fixed one real UAF in the raw() push-exception arm (bad70a1) and an inconsistent-state edge on the strict-close throw path (7295708). I re-checked all remaining stmt-touching paths against the flag and found no gaps. analyzeHeap() passing the stale pointer to setWrappedObjectForCell() was confirmed harmless (opaque address only, never dereferenced).

Level of scrutiny

High. This is native C++ in the "memory safety — the most-blocked category" per REVIEW.md, with re-entrancy hazards (user JS during rebind getters and Array.prototype setters can synchronously close the database mid-operation). It has received three rounds of automated review with fixes at each round. The code now looks correct to me, but the combination of (a) a user-visible contract change to close(true) — matching better-sqlite3 but reversing Bun's documented behavior — and (b) UAF-adjacent native surface warrants a maintainer's eyes rather than bot-only approval.

Other factors

• All 6 findings from my prior passes are addressed; the only unresolved inline thread (CHECK_PREPARED_JIT) was fixed in the latest commit.
• Test coverage is thorough: cache-overflow variants (referenced/unreferenced/GC'd/pre-finalized), file-handle release, using disposal, and a subprocess test for the close-during-raw()-push re-entrancy case.
• No human maintainer has weighed in on this PR; the design pivot from WeakRef-tracking to sqlite3_next_stmt came from the automated review cycle. The PR description notes it supersedes #35604/#33307 — a maintainer should confirm this is the direction they want for those as well.

[claude]

No new findings this pass — the fixes in bad70a1, c88249b, and 8363183 address everything raised earlier. I'm deferring rather than approving because this is a user-visible contract change to close(true) (it now force-finalizes outstanding statements instead of throwing database is locked), and the implementation deliberately leaves JS wrappers holding freed sqlite3_stmt* pointers guarded by a shared flag. Both the API decision and the memory-safety surface warrant a maintainer sign-off.

What was reviewed:

• sqlite3_next_stmt finalize loop and the finalizedStatementsOnClose guard at every stmt touch site (CHECK_PREPARED, rebind object/array paths, all()/values()/raw() row loops, toString, finalize, destructor).
• The strict-throw path now retires the handle via sqlite3_close_v2 and nulls versionDB->db before throwing, so the state machine stays consistent.
• raw()'s push-exception arm no longer resets a possibly-freed stmt; matches all()/values().
• Checked VersionSqlite3::release() and the termination path stay on sqlite3_close_v2 with the flag unset, so GC-driven cleanup is unchanged.

Extended reasoning...

Overview

Changes bun:sqlite's Database.close() to walk sqlite3_next_stmt() and finalize every outstanding prepared statement before sqlite3_close(), so close cannot fail with SQLITE_BUSY and the file handle is released immediately. Because the underlying sqlite3_stmts are freed while JS Statement wrappers may still be alive, a new VersionSqlite3::finalizedStatementsOnClose flag gates every subsequent stmt dereference: CHECK_PREPARED, both rebind paths, the mid-row-collection guards in all()/values()/raw(), toString(), finalize(), and the destructor. Docs and types are updated to describe the new contract; ~8 new tests plus updates to two existing tests that asserted the old database is locked throw.

Security risks

None user-facing. The risk is internal memory safety: statement wrappers now hold dangling sqlite3_stmt* values after close(), and correctness depends on every code path that could reach sqlite3_*(stmt) checking finalizedStatementsOnClose first. Earlier review rounds caught two real gaps here (the raw() push-exception sqlite3_reset and the strict-throw path leaving versionDB->db non-null); both are fixed. I re-traced the remaining stmt uses in the file against the diff and did not find an unguarded site, and analyzeHeap's use of the stale pointer as opaque snapshot metadata is safe (JSC never dereferences it).

Level of scrutiny

High. This is native C++ in the JSC bindings with an intentional dangling-pointer design guarded by a boolean, and it changes the documented behavior of close(true) — previously "throws if statements are outstanding", now "finalizes them and succeeds" (matching better-sqlite3). That is an API design decision a maintainer should ratify, independent of correctness.

Other factors

The PR has been through three bot-review iterations; every prior finding (one 🔴 UAF, several 🟡 nits) is addressed and resolved. Test coverage is thorough: the original #36572 repro, still-referenced overflow statements, GC-then-close, pre-finalized overflow statements, file-handle release on disk, close-during-bind via getter, close-during-raw()-push via Array.prototype setter, and the #13082 test now asserts the new rejection. The node:sqlite implementation uses a separate registry (NodeSqlite.cpp) and is unaffected. Given the combination of memory-safety subtlety and a user-visible contract change, I'm not comfortable auto-approving.

[Jarred-Sumner]

The semantics are right — explicit close() finalizing outstanding statements is what both better-sqlite3 and node:sqlite do, and the old "database is locked" from bun's own query() cache overflow was clearly a bug. But the mechanism should match theirs too: track the wrappers and null their handles, rather than sqlite3_next_stmt() + a shared flag that leaves every JSSQLStatement holding a dangling sqlite3_stmt*.

Prior art, for reference:

• better-sqlite3 Database::CloseHandles(): for (Statement* stmt : stmts) stmt->CloseHandles(); (sets alive = false, sqlite3_finalize(handle)), then sqlite3_close().
• node:sqlite DatabaseSync::Close(): FinalizeStatements() → for (auto stmt : statements_) stmt->Finalize(); (sqlite3_finalize, statement_ = nullptr), then sqlite3_close_v2(). ~StatementSync calls db_->UntrackStatement(this).

Requested change:

1. Give VersionSqlite3 an intrusive list (or WTF::Vector/set) of JSSQLStatement*. Link in the constructor where ++version_db->reference_count already happens; unlink in ~JSSQLStatement before release(). VersionSqlite3 already strictly outlives every wrapper via reference_count, and destructors run on the mutator thread, so there is no GC-ordering or locking concern — same invariant node/better-sqlite3 rely on.
2. In jsSQLStatementCloseStatementFunction, walk that list: sqlite3_finalize(s->stmt); s->stmt = nullptr;, clear it, then sqlite3_close(). Optionally still do a sqlite3_next_stmt() sweep afterwards as a backstop for internally-prepared statements not owned by a wrapper (e.g. the transient one in jsSQLStatementExecuteFunction), but wrappers should never be left pointing at freed memory.
3. Delete finalizedStatementsOnClose and every check of it. Once stmt is nulled, the existing CHECK_PREPARED / castedThis->stmt != stmt / statementStillAlive re-validation paths already cover close-during-callback, ~JSSQLStatement and finalize() naturally skip the double-finalize, and toString() needs no special case. The error message becomes "Statement has finalized" rather than "Database has closed" — that's fine and matches node.

That should shrink the diff to roughly: the list, the close loop, and the tests — no new state that ten call sites have to remember to consult.

Minor: the #13082 test assertion and docs/types wording will need to follow whichever error string you land on.

[robobun]

Reworked as requested in abeabc8: VersionSqlite3 now keeps a list of live JSSQLStatement wrappers (linked where reference_count is incremented, unlinked in the destructor), and close() finalizes each one and nulls its handle before sqlite3_close(). Kept the sqlite3_next_stmt() sweep as a backstop for the transient statement in db.run() when a bound getter closes the database mid-bind (that statement has no wrapper, and without the sweep its auto-destructor would double-finalize).

finalizedStatementsOnClose and all ten of its checks are gone; the existing "Statement has finalized" paths cover post-close use, and that is now the error string (matching node), with the #13082 assertion, docs, and types following. Net diff vs the previous revision is -16 lines.

Full sqlite suite (115), node:sqlite (115), and 14709 regression tests pass; the #36572 repros still fail on the released build and pass with this.

[coderabbitai]

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 `@test/js/bun/sqlite/sqlite.test.js`:
- Line 1745: Update the concurrent statement test around the finalized-statement
assertion to accept an operation parameter, then schedule separate concurrent
tasks for stmt.all(), stmt.get(), and stmt.run() with a valid run operation.
Ensure each task independently verifies the “Statement has finalized” error so
all statement methods execute despite one failure.

🪄 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: a1039d8d-4c2e-4a5c-bdd4-5b92939b293e

📥 Commits

Reviewing files that changed from the base of the PR and between 8363183 and abeabc8.

📒 Files selected for processing (2)

• src/jsc/bindings/sqlite/JSSQLStatement.cpp
• test/js/bun/sqlite/sqlite.test.js

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/jsc/bindings/sqlite/JSSQLStatement.cpp:2925-2926 — nit: two stray blank lines were added between the this->stmt != stmt block and if (val.isNumber()), leaving three consecutive blank lines — leftover whitespace from where abeabc8 deleted the finalizedStatementsOnClose check that used to sit here. No runtime effect; just drop the extra newlines.

  Extended reasoning...

  What

  The diff hunk at @@ -2883,6 +2922,8 @@ JSC::JSValue JSSQLStatement::rebind(...) adds two lines with no content:

       if (this->stmt != stmt) [[unlikely]] {
           throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s));
           return {};
       }
  
  +
  +
       if (val.isNumber()) {

  Combined with the pre-existing single blank line, this leaves three consecutive empty lines (2924–2926) between the closing brace of the liveness check and if (val.isNumber()).

  Why it's here

  An earlier iteration of this PR had a version_db->finalizedStatementsOnClose check sitting at exactly this spot in rebind(), separated from the neighboring blocks by blank lines. Commit abeabc8 ("track statement wrappers per connection and null their handles on close") removed that flag and all of its checks — including this one — but the surrounding blank lines that framed it were left behind, so the diff now inserts two content-free + lines where the deleted block used to be.

  Step-by-step proof

  1. Pre-PR, line 2883 onward had a single blank line between the } closing the this->stmt != stmt guard and if (val.isNumber()).
  2. The diff at this hunk shows exactly +\n+\n with no other content — two added empty lines.
  3. Reading the current file confirms lines 2924, 2925, and 2926 are all empty; line 2923 is } and line 2927 is if (val.isNumber()) {.
  4. No other function in this file uses a triple blank line as an internal separator, so this doesn't match the file's local conventions.

  Impact

  None at runtime — pure whitespace. Per REVIEW.md ("Match the exact file's local conventions" and "Delete dead code in the same PR that makes it dead"), the noise should go, but it does not justify blocking the merge.

  Fix

  Delete the two added blank lines so the gap between the } and if (val.isNumber()) is a single blank line, as it was before the PR.

[claude]

I didn't find any correctness issues, and every prior review nit has been addressed. Deferring to a human because this changes the public close(true) contract (finalizes outstanding statements instead of throwing database is locked) and reworks native statement-handle lifetime — both worth a maintainer sign-off.

What was reviewed:

• Statement-wrapper tracking in VersionSqlite3::statements (HashSet): link in create(), unlink in the destructor before release(); VersionSqlite3 is never freed, so version_db cannot dangle.
• close() iterates the set without mutation (nulls handles, then clear()), then sqlite3_next_stmt() sweep, then sqlite3_close() with close_v2 fallback — no double-finalize path found.
• Re-validation after JS callbacks in rebindObject/rebindStatement/jsSQLStatementExecuteFunction/raw() — the versionDB->db != db guard covers close-during-bind, and the transient sql.stmt = nullptr prevents the auto-destructor double-finalize.
• Docs note that GC still uses the release()/close_v2 path (not the new sweep) — checked and that's accurate; ruled out as a doc bug.

Extended reasoning...

Overview

This PR fixes #36572: Database.close(true) failed with SQLITE_BUSY when more than MAX_QUERY_CACHE_SIZE distinct db.query() strings had been prepared, and non-strict close() left the file handle open until GC (undeletable on Windows). The fix adds per-connection tracking of live JSSQLStatement wrappers in a WTF::HashSet on VersionSqlite3, and rewrites jsSQLStatementCloseStatementFunction to finalize every tracked wrapper (nulling its stmt pointer), sweep any untracked statements via sqlite3_next_stmt(), then call sqlite3_close() (falling back to close_v2 on failure so the handle is always retired). It also tightens re-entrancy guards in the bind and raw() paths so a getter/setter that calls db.close() mid-operation cannot leave code touching a freed sqlite3_stmt*. Docs, .d.ts, and tests are updated to reflect the new contract.

Files: src/jsc/bindings/sqlite/JSSQLStatement.cpp (~60 lines net), test/js/bun/sqlite/sqlite.test.js (~130 lines added, 2 tests rewritten), docs/runtime/sqlite.mdx, packages/bun-types/sqlite.d.ts.

Security risks

None identified. No untrusted input parsing, auth, or crypto. The changes are lifetime/re-entrancy hardening around an existing native binding; if anything they close use-after-free windows that existed before (e.g. raw() calling sqlite3_reset on a handle user code just finalized). No new attack surface.

Level of scrutiny

High. This is C++ pointer-lifetime code in JavaScriptCore bindings — REVIEW.md's most-blocked category. It also changes a documented public API contract: close(true) and using db = ... blocks that previously threw database is locked over live prepared statements now silently finalize them and succeed. That matches better-sqlite3 and node:sqlite, and the PR updates docs/types/tests consistently, but it's a user-visible behavior change that a maintainer should explicitly approve rather than have land via bot.

I traced the new lifecycle: create() links the wrapper next to the existing ++reference_count; the destructor unlinks (remove(this), safe on an already-cleared set) before sqlite3_finalize and before release(); release() never frees the VersionSqlite3 object itself (only close_v2s the db), so version_db cannot dangle in the destructor. close() range-fors the set without mutating it during iteration, nulls each wrapper's stmt, then clear()s — subsequent per-statement finalize()/destructor calls see stmt == nullptr and no-op. The sqlite3_next_stmt backstop plus sql.stmt = nullptr in jsSQLStatementExecuteFunction correctly handles the transient db.run() statement when a bound getter closes the db mid-bind. I did not find a double-finalize or UAF path.

Other factors

• All prior inline review feedback (mine and CodeRabbit's) has been addressed and every thread is resolved; the latest commit (6645477) swapped Vector→HashSet per my last nit.
• Test coverage is thorough: the #36572 repro at the exact cache-size boundary, referenced/unreferenced/GC'd/pre-finalized variants, the Windows file-deletion case, close-during-raw(), close-during-db.run() bind, and the #13082 test now asserts each of all/get/run rejects with the new error string.
• The GC path (release()) still uses sqlite3_close_v2 without the new sweep, which is fine — statements outlive the db handle and finalize themselves; the docs' "close() is called automatically when the database is garbage collected" is a slight simplification but not newly wrong (this was raised and ruled out).
• Two existing tests were rewritten to assert the new contract (.not.toThrow() instead of .toThrow("database is locked")), which is expected but underscores that this is an intentional behavior change.