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

sqlite: tidy up handle ownership and lifetimes#66912
araujogui wants to merge 4 commits into
nodejs:mainfrom
araujogui:sqlite-ownership-cleanup

Conversation

@araujogui

@araujogui araujogui commented Oct 22, 2026

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.

@araujogui
sqlite: track statements in the Statement constructor
b069411 
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>
@araujogui
sqlite: use DeleteFnPtr for all SQLite handles
52e05f9 
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>
@araujogui
sqlite: settle BackupJob through a single exit point
9ad233d 
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>
@araujogui
sqlite: cache the SQLTagStore constructor template
b8409d9 
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

nodejs-github-bot commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

Copilot AI reviewed Oct 22, 2026

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 •
edited
Loading

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 66 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.

@araujogui
araujogui requested a review from trivikr October 23, 2026 18:10
@trivikr trivikr added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. author ready PRs with CI started, the required approvals, and no outstanding review comments. labels Oct 23, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 23, 2026
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

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

@araujogui araujogui added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 24, 2026
@github-actions github-actions Bot added request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 24, 2026
@github-actions

github-actions Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Failed to start CI

   ℹ  Existing CI run found: https://ci.nodejs.org/job/node-test-pull-request/78616/
   ✖  Refusing to start a potentially duplicate CI job. CI has already succeeded for this commit.
Full Auto Start CI output 
�[36m⠋�[39m Getting reviews from nodejs/node/pull/66912
�[36m⠋�[39m Getting commits from nodejs/node/pull/66912
�[36m⠙�[39m Validating Jenkins credentials
�[36m⠙�[39m Validating Jenkins credentials
✔  Jenkins credentials valid
�[36m⠹�[39m Getting comments from nodejs/node/pull/66912
�[36m⠸�[39m Querying data for job/node-test-pull-request/78616/
�[36m⠸�[39m Querying data for job/node-test-pull-request/78616/
�[36m⠸�[39m Querying API for job/node-test-pull-request/78616/
✔  Build data downloaded
   ℹ  Existing CI run found: https://ci.nodejs.org/job/node-test-pull-request/78616/
   ✖  Refusing to start a potentially duplicate CI job. CI has already succeeded for this commit.

View workflow run

@trivikr trivikr added commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. commit-queue PRs queued for automated landing through the Commit Queue. and removed request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. labels Oct 24, 2026
@nodejs-github-bot nodejs-github-bot added the lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. label Oct 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@trivikr trivikr trivikr approved these changes

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@araujogui @nodejs-github-bot @trivikr