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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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

[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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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

[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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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

[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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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

[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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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

[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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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

[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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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

[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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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');constdb1=newDatabaseSync(':memory:');constdb2=newDatabaseSync(':memory:');db2.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1);');for(leti=0;i<200000;i++)db1.prepare('-- '+i);// stmt == NULLconstlive=[];for(leti=0;i<50000;i++)live.push(db2.prepare('SELECT a FROM t'));db1.close();// walks db1->statements_, now full of stale pointersletbroken=0;for(constsoflive){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:

constdb=newDatabaseSync(':memory:');db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (111);');conststore=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 connectionstore.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