sqlite: manage sqlite3_stmt lifetime with RAII

[araujogui]

sqlite3_stmt was owned by raw pointer, with sqlite3_finalize() spread across error paths. Replaces it with StatementPtr (DeleteFnPtr<sqlite3_stmt, FinalizeStatement>), moved into StatementSync.

Fixes two things:

• StatementSync::Create() failure leaked the statement and inserted a null pointer into db->statements_, which FinalizeStatements() dereferences.
• Tag store statements were never tracked in db->statements_, so db.close() left them open. Tracking them also makes the existing IsFinalized() re-prepare path in the cache lookup reachable.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 91.22807% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (673cdef) to head (31e31a1).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	90.74%	2 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62419      +/-   ##
==========================================
- Coverage   90.32%   90.31%   -0.02%     
==========================================
  Files         760      760              
  Lines      248553   248560       +7     
  Branches    46910    46909       -1     
==========================================
- Hits       224510   224489      -21     
- Misses      15462    15497      +35     
+ Partials     8581     8574       -7     

Files with missing lines	Coverage Δ
src/node_sqlite.h	83.33% <100.00%> (+0.72%)	⬆️
src/node_sqlite.cc	81.46% <90.74%> (+0.17%)	⬆️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[louwers]

Can RAII be used instead?

[araujogui]

Can RAII be used instead?

Well, we could use std::unique_ptr<sqlite3_stmt, decltype(&sqlite3_finalize)>, but I feel the current way is good enough.

[araujogui]

@nodejs/sqlite Bump, I did a safety improvement by implementing RAII for sqlite3_stmt.

[Copilot]

Pull request overview

Fixes a resource-management bug in the SQLite sync binding where a prepared sqlite3_stmt could be leaked (and a nullptr inserted into db->statements_) when StatementSync::Create fails to allocate the JS wrapper object (e.g., OOM).

Changes:

• Introduces RAII ownership for sqlite3_stmt via StatementPtr (DeleteFnPtr + FinalizeStatement).
• Updates StatementSync to own the prepared statement with StatementPtr and uses .get() at call sites.
• Ensures DatabaseSync::Prepare returns early when StatementSync::Create fails, preventing nullptr insertion into db->statements_.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/node_sqlite.h	Adds FinalizeStatement + StatementPtr alias and updates StatementSync API/member to use RAII for sqlite3_stmt.
src/node_sqlite.cc	Transfers statement ownership with StatementPtr, resets via RAII, and guards against Create() failure to avoid leaks/null insertion.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[TrevorBurnham]

The statement_ → statement_.get() conversion checks out. Normalizing it away leaves four semantic edits: the Prepare null guard, the StatementPtr member/ctor/Create signature, Finalize() → statement_.reset(), and dropping the two manual sqlite3_finalize(s) calls in SQLTagStore::PrepareStatement. All four look right. The removed finalizes are covered by the by-value StatementPtr parameter's destructor when Create bails, and reset() is idempotent, so the FinalizeStatements() / ~StatementSync double-call path stays safe.

While reviewing I hit two pre-existing bugs in the lines this PR touches. Both predate the change and neither is caused by it, so feel free to split them out — but since this PR is about sqlite3_stmt ownership they seemed worth raising here. Both reproduce on a local build of 78e32bc.

1. Prepare tracks a statement it never untracks (node_sqlite.cc:1551-1561)

sqlite3_prepare_v2 returns SQLITE_OK with *ppStmt == NULL when the input holds no statement. All of '', ' ', '\n', ';', '-- x', '/* x */' are accepted by prepare() on this branch. The new if (!stmt) return; catches Create failing but not s == nullptr, so a StatementSync with statement_ == nullptr gets inserted into db->statements_ at line 1561. When it is GC'd, ~StatementSync checks if (!IsFinalized()) — already true, since IsFinalized() is statement_ == nullptr — so UntrackStatement(this) is skipped and the freed pointer stays in the set. FinalizeStatements() then calls stmt->Finalize() on freed memory at close().

const {DatabaseSync} = require('node:sqlite');
const db1 = new DatabaseSync(':memory:');
const db2 = new DatabaseSync(':memory:');
db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');
for (let i = 0; i < 200000; i++) db1.prepare('-- ' + i);  // stmt == NULL
const live = [];
for (let i = 0; i < 50000; i++) live.push(db2.prepare('SELECT a FROM t'));
db1.close();  // walks db1->statements_, now full of stale pointers
let broken = 0;
for (const s of live) { try { s.get(); } catch (e) { broken++; } }
console.log(broken, '/', live.length, db2.isOpen);

With --max-old-space-size=80: 19889 / 50000 true. Closing db1 corrupts a different database's live statements into "statement has been finalized" while db2.isOpen is still true. Swapping the comment for real SQL ('SELECT ' + i) gives 0 / 50000, isolating it to the NULL-stmt path.

Fix is either bailing before the insert when s == nullptr, or untracking unconditionally in ~StatementSync.

2. Tag store statements are never tracked (node_sqlite.cc:3572-3583)

SQLTagStore::PrepareStatement puts its StatementSync in sql_tags_ but never does db->statements_.insert(...); line 1561 is the only insert in the file. FinalizeStatements() therefore misses them, sqlite3_close_v2 only defers the close, and the cached statement keeps running against the old connection:

const db = new DatabaseSync(':memory:');
db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');
const store = db.createTagStore();
store.get`SELECT a FROM t`;           // { a: 111 }
db.close(); db.open();
db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (222);');
db.prepare('SELECT a FROM t').get();  // { a: 222 }  new connection
store.get`SELECT a FROM t`;           // { a: 111 }  stale

Minor: node_webstorage.h:23 already has stmt_deleter / stmt_unique_ptr for the same job — might be worth one shared definition rather than a second. FinalizeStatement as a free function also reads close to DatabaseSync::FinalizeStatements() and StatementSync::Finalize().

[araujogui]

The statement_ → statement_.get() conversion checks out. Normalizing it away leaves four semantic edits: the Prepare null guard, the StatementPtr member/ctor/Create signature, Finalize() → statement_.reset(), and dropping the two manual sqlite3_finalize(s) calls in SQLTagStore::PrepareStatement. All four look right. The removed finalizes are covered by the by-value StatementPtr parameter's destructor when Create bails, and reset() is idempotent, so the FinalizeStatements() / ~StatementSync double-call path stays safe.

While reviewing I hit two pre-existing bugs in the lines this PR touches. Both predate the change and neither is caused by it, so feel free to split them out — but since this PR is about sqlite3_stmt ownership they seemed worth raising here. Both reproduce on a local build of 78e32bc.

1. Prepare tracks a statement it never untracks (node_sqlite.cc:1551-1561)

sqlite3_prepare_v2 returns SQLITE_OK with *ppStmt == NULL when the input holds no statement. All of '', ' ', '\n', ';', '-- x', '/* x */' are accepted by prepare() on this branch. The new if (!stmt) return; catches Create failing but not s == nullptr, so a StatementSync with statement_ == nullptr gets inserted into db->statements_ at line 1561. When it is GC'd, ~StatementSync checks if (!IsFinalized()) — already true, since IsFinalized() is statement_ == nullptr — so UntrackStatement(this) is skipped and the freed pointer stays in the set. FinalizeStatements() then calls stmt->Finalize() on freed memory at close().

const {DatabaseSync} = require('node:sqlite');
const db1 = new DatabaseSync(':memory:');
const db2 = new DatabaseSync(':memory:');
db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');
for (let i = 0; i < 200000; i++) db1.prepare('-- ' + i);  // stmt == NULL
const live = [];
for (let i = 0; i < 50000; i++) live.push(db2.prepare('SELECT a FROM t'));
db1.close();  // walks db1->statements_, now full of stale pointers
let broken = 0;
for (const s of live) { try { s.get(); } catch (e) { broken++; } }
console.log(broken, '/', live.length, db2.isOpen);

With --max-old-space-size=80: 19889 / 50000 true. Closing db1 corrupts a different database's live statements into "statement has been finalized" while db2.isOpen is still true. Swapping the comment for real SQL ('SELECT ' + i) gives 0 / 50000, isolating it to the NULL-stmt path.

Fix is either bailing before the insert when s == nullptr, or untracking unconditionally in ~StatementSync.

2. Tag store statements are never tracked (node_sqlite.cc:3572-3583)

SQLTagStore::PrepareStatement puts its StatementSync in sql_tags_ but never does db->statements_.insert(...); line 1561 is the only insert in the file. FinalizeStatements() therefore misses them, sqlite3_close_v2 only defers the close, and the cached statement keeps running against the old connection:

const db = new DatabaseSync(':memory:');
db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');
const store = db.createTagStore();
store.get`SELECT a FROM t`;           // { a: 111 }
db.close(); db.open();
db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (222);');
db.prepare('SELECT a FROM t').get();  // { a: 222 }  new connection
store.get`SELECT a FROM t`;           // { a: 111 }  stale

Minor: node_webstorage.h:23 already has stmt_deleter / stmt_unique_ptr for the same job — might be worth one shared definition rather than a second. FinalizeStatement as a free function also reads close to DatabaseSync::FinalizeStatements() and StatementSync::Finalize().

Great catches ty, fixed both.

[TrevorBurnham]

LGTM! Needs a maintainer to approve.

[trivikr]

prepare('') throwing ERR_INVALID_ARG_VALUE where it previously returned a finalized statement is okay without semver, since sqlite is still a Release candidate.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75716/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75785/

[nodejs-github-bot]

Landed in b0edc37