close
Skip to content

sqlite: keep sessions alive across SQLite callbacks - #65465

Open
TrevorBurnham wants to merge 2 commits into
nodejs:mainfrom
TrevorBurnham:sqlite-session-gc-pin
Open

sqlite: keep sessions alive across SQLite callbacks#65465
TrevorBurnham wants to merge 2 commits into
nodejs:mainfrom
TrevorBurnham:sqlite-session-gc-pin

Conversation

@TrevorBurnham

@TrevorBurnham TrevorBurnham commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Session calls MakeWeak() and DatabaseSync::sessions_ holds raw Session*, so nothing keeps a session alive once JavaScript drops the reference. SQLite runs PRAGMA table_xinfo from inside its pre-update hook, which reaches JavaScript, so a GC during a callback invoked from there collects the session the hook is still walking:

db.createSession(); // return value dropped
db.setAuthorizer((a, p1) => {
  if (p1 === 'table_xinfo') globalThis.gc();
  return constants.SQLITE_OK;
});
db.exec('INSERT INTO t VALUES (1)'); // segfault in xPreUpdate

CallbackDepthGuard already wraps every point where SQLite hands control back to JavaScript (authorizer, trace subscriber, user-defined functions, aggregates, changeset apply), so it now also holds a BaseObjectPtr to each attached session for the callback's duration. BaseObjectPtr clears the weak handle on the first strong reference and restores it on the last, so a session that JavaScript no longer references simply becomes collectable again once the callback returns. Connections with no sessions allocate nothing.

Making sessions_ strong instead would create a cycle with Session::database_, which is deliberately strong (a session must keep its database alive), and leak both objects.

TraceCallback built its payload before entering the guard. That allocation can trigger a GC, so the guard moves above it. I could not get that window to crash on demand, even with --gc-interval=1, so treat it as defensive rather than a demonstrated fix.

Fixes: #65460

Session objects are weak and nothing else holds a strong reference to
them, so a garbage collection can free one while SQLite is still using
it. SQLite runs "PRAGMA table_xinfo" from inside its pre-update hook,
which reaches JavaScript, so a GC during a callback invoked from there
can collect a session the hook is still walking.

Hold a strong reference to every attached session for the duration of
each callback SQLite invokes. The trace callback now enters that guard
before building its payload, since the allocation can itself trigger a
garbage collection.

Fixes: nodejs#65460
Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
@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 Aug 21, 2026
Return the pinned sessions by value instead of appending to a caller's
vector, which read as an append but reserved as if the target were empty.

Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
@TrevorBurnham
TrevorBurnham marked this pull request as ready for review August 21, 2026 20:41
@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (21f0f27) to head (7adc572).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65465      +/-   ##
==========================================
+ Coverage   90.12%   90.13%   +0.01%     
==========================================
  Files         752      751       -1     
  Lines      252315   252306       -9     
  Branches    47444    47436       -8     
==========================================
+ Hits       227395   227416      +21     
+ Misses      16217    16196      -21     
+ Partials     8703     8694       -9     
Files with missing lines Coverage Δ
src/node_sqlite.h 86.40% <100.00%> (+0.13%) ⬆️
src/node_sqlite.cc 82.09% <87.50%> (+0.25%) ⬆️

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

sqlite: GC of an unreferenced session frees memory SQLite is still using

2 participants