sqlite: keep sessions alive across SQLite callbacks

Project: nodejs/node | Language: C++/JavaScript | Concepts: memory-management, garbage-collection, error-handling | Difficulty: senior
Link: https://github.com/nodejs/node/pull/65465

The problem

When SQLite calls back into JavaScript from inside certain hooks (like the pre-update hook), it may still be iterating over a list of Session objects. These Session objects are held weakly (using MakeWeak()), so if JavaScript has dropped all references to a session, it can be garbage collected at any time—including during a callback triggered by SQLite. If this happens, SQLite could crash (segfault) because it is still using memory that has just been freed.

Why it's non-trivial

A naive solution might be to just make the session list hold strong references, but this would create a reference cycle: Session already holds a strong reference to its DatabaseSync, and making DatabaseSync hold strong references to its sessions would cause memory leaks. The fix needs to keep sessions alive only for the duration of callbacks, without introducing leaks or cycles, and must handle all the places where SQLite can call into JavaScript.

The approach

1. Add a method to pin all sessions for the duration of a callback:

+std::vector<BaseObjectPtr<Session>> DatabaseSync::PinSessions() const {
+  std::vector<BaseObjectPtr<Session>> pinned;
+  pinned.reserve(sessions_.size());
+  for (Session* session : sessions_) {
+    pinned.emplace_back(session);
+  }
+  return pinned;
+}

This method returns a vector of strong references (BaseObjectPtr<Session>) to all attached sessions. By holding these during a callback, we prevent them from being garbage collected.

2. Update the callback guard to pin sessions:

-class CallbackDepthGuard {
- public:
-  explicit CallbackDepthGuard(DatabaseSync* db) : db_(db) {
-    db_->IncrementCallbackDepth();
-  }
-  ~CallbackDepthGuard() { db_->DecrementCallbackDepth(); }
-
- private:
-  DatabaseSync* db_;
-};
+// Guards a window in which SQLite hands control back to JavaScript. Construct
+// it before allocating anything on the V8 heap, since the pinned sessions
+// below are what keep a garbage collection during that window safe.
+class CallbackDepthGuard {
+ public:
+  explicit CallbackDepthGuard(DatabaseSync* db)
+      : db_(db), pinned_sessions_(db->PinSessions()) {
+    db_->IncrementCallbackDepth();
+  }
+  ~CallbackDepthGuard() { db_->DecrementCallbackDepth(); }
+
+ private:
+  DatabaseSync* db_;
+  std::vector<BaseObjectPtr<Session>> pinned_sessions_;
+};

The guard now stores strong references to all sessions for the duration of the callback, ensuring they cannot be collected until the callback is finished.

3. Move the guard to cover all allocations that could trigger GC:

-  CallbackDepthGuard guard(db);
+  // Entered before building the payload below, because allocating it can
+  // trigger a garbage collection that SQLite is not prepared for.
+  CallbackDepthGuard guard(db);

The guard is now constructed before any V8 heap allocations that could trigger garbage collection, preventing sessions from being collected during those allocations.

4. Add regression tests to ensure sessions survive GC during callbacks:

+test('session - survives GC during an authorizer callback', (t) => {
+  ...
+  database.createSession(); // Never referenced again, so it is collectable.
+  ...
+  database.setAuthorizer((actionCode, param1) => {
+    if (actionCode === constants.SQLITE_PRAGMA && param1 === 'table_xinfo') {
+      ran = true;
+      globalThis.gc();
+      globalThis.gc();
+    }
+    return constants.SQLITE_OK;
+  });
+  ...
+  database.exec('INSERT INTO data VALUES (1)');
+  t.assert.ok(ran, 'the authorizer callback never ran');
+});
+
+test("session - survives GC during a 'sqlite.db.query' subscriber", (t) => {
+  ...
+  database.createSession(); // Never referenced again, so it is collectable.
+  ...
+  const handler = ({ sql }) => {
+    if (sql.includes('table_xinfo')) {
+      ran = true;
+      globalThis.gc();
+      globalThis.gc();
+    }
+  };
+  dc.subscribe('sqlite.db.query', handler);
+  ...
+  database.exec('INSERT INTO data VALUES (1)');
+  t.assert.ok(ran, 'the subscriber never ran');
+});

These tests create a session, drop all references, and then force garbage collection during a callback. If the session is collected too early, SQLite will crash; if not, the test passes.

From the review thread

No notable review discussion on this PR.

Takeaways

Guard against GC in native-to-JS callbacks: When native code (like SQLite) calls into JavaScript, ensure that any native objects still in use are strongly referenced for the duration of the callback to prevent premature garbage collection.
Avoid reference cycles when managing object lifetimes: Use temporary strong references (like scoped guards) instead of making persistent references strong, to prevent memory leaks and cycles.
Test for GC edge cases explicitly: Write regression tests that force garbage collection at critical moments to catch lifecycle bugs that may not appear under normal execution.