Skip to content

sqlite: invalidate sessions when closing database - #64783

Open
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:sqlite-methods-post-db-sync-closed
Open

sqlite: invalidate sessions when closing database#64783
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:sqlite-methods-post-db-sync-closed

Conversation

@trivikr

@trivikr trivikr commented Jul 27, 2026

Copy link
Copy Markdown
Member

Fixes: #64782

This change tracks Session wrappers instead of native handles and
deletes sessions through Session::Delete(). This marks each wrapper as
closed when its native handle is deleted. After the database is reopened,
using an old session now throws session is not open.


Assisted-by: codex:gpt-5.6-sol

DatabaseSync::DeleteSessions() deleted native sqlite3_session handles
without clearing the pointers held by their Session wrappers. Reopening
the database allowed stale session handles to be used, crashing the
process.

Track Session wrappers and delete sessions through Session::Delete() so
the wrappers are marked closed.

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: codex:gpt-5.6-sol
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@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 Jul 27, 2026
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.50000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.16%. Comparing base (54a5095) to head (c51eb62).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 62.50% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64783      +/-   ##
==========================================
+ Coverage   90.14%   90.16%   +0.02%     
==========================================
  Files         744      744              
  Lines      242518   242528      +10     
  Branches    45685    45693       +8     
==========================================
+ Hits       218611   218676      +65     
+ Misses      15396    15355      -41     
+ Partials     8511     8497      -14     
Files with missing lines Coverage Δ
src/node_sqlite.h 80.64% <ø> (ø)
src/node_sqlite.cc 80.55% <62.50%> (-0.12%) ⬇️

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

@trivikr trivikr added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 27, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 27, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

sqlite: Session methods crash after DatabaseSync is closed and reopened

3 participants