Skip to content

sqlite: tidy up handle ownership and lifetimes - #66912

Open
araujogui wants to merge 4 commits into
nodejs:mainfrom
araujogui:sqlite-ownership-cleanup
Open

araujogui wants to merge 4 commits into
nodejs:mainfrom
araujogui:sqlite-ownership-cleanup

Conversation

@araujogui

Copy link
Copy Markdown
Member

Ownership cleanups in node_sqlite.cc:

  • Track statements in the Statement constructor instead of in each caller, matching Session.
  • Use DeleteFnPtr for every SQLite handle and drop the custom deleter structs and OnScopeLeave cleanups.
  • Settle BackupJob through a single Settle() so there is one delete this. This also frees the job when building the progress object fails, which leaked before.
  • Cache the SQLTagStore constructor template on the environment, so all tag stores share one prototype.

Statement::Close() untracks itself, so the constructor should track
itself too, like Session does, instead of relying on every caller.

Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Replace the custom deleter structs and OnScopeLeave cleanups with
DeleteFnPtr aliases, and pass session ownership to Session::Create.

Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Route every exit through Settle(), which finalizes, settles the
promise and deletes the job once. This also frees the job when
building the progress object fails.

Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Store the template on the environment like the other sqlite classes,
so stores no longer get a fresh template and prototype each time.

Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Copilot AI balanced review requested due to automatic review settings October 22, 2026 01:09
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Oct 22, 2026
@codecov

codecov Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.23077% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.58%. Comparing base (df2ea49) to head (b8409d9).
⚠️ Report is 45 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 93.47% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66912      +/-   ##
==========================================
+ Coverage   90.51%   90.58%   +0.06%     
==========================================
  Files         797      797              
  Lines      278252   278487     +235     
  Branches    53472    53531      +59     
==========================================
+ Hits       251868   252257     +389     
+ Misses      16761    16584     -177     
- Partials     9623     9646      +23     
Files with missing lines Coverage Δ
src/node_sqlite.h 87.71% <100.00%> (+0.44%) ⬆️
src/node_sqlite.cc 82.16% <93.47%> (+0.24%) ⬆️

... and 52 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants