sqlite: throw on disposal of an in-use session

Project: nodejs/node | Language: JavaScript, C++ | Concepts: error-handling, api-design, testing-strategy | Difficulty: mid
Link: https://github.com/nodejs/node/pull/65449

The problem

When a SQLite Session was disposed using session[Symbol.dispose](), it would internally call Session::Close(), but any errors thrown (such as trying to close a session that was still in use) were caught and silently discarded. This meant that attempts to dispose an in-use session appeared to succeed, even though the session remained open, leading to inconsistent and misleading behavior compared to calling session.close() directly.

Why it's non-trivial

A naive fix might just let errors bubble up or only add a check in one place, but the disposal and close logic must be consistent, and error states must propagate correctly to JavaScript. The implementation must avoid double-closing, handle already-closed sessions gracefully, and ensure that disposal and explicit close behave the same in all relevant states.

The approach

1. Update documentation to clarify disposal error behavior:

-Closes the session. If the session is already closed, does nothing.
+Closes the session. If the session is already closed, then this is a no-op. An
+[`ERR_INVALID_STATE`][] error is thrown if the session is currently generating
+a changeset or patchset, under the same conditions as [`session.close()`][].

This change makes it clear in the docs that disposing a session in use will now throw an error, just like close(), and that both methods behave the same in this regard.

2. Change C++ implementation to propagate errors instead of swallowing them:

Old code:

-void Session::Dispose(const v8::FunctionCallbackInfo<v8::Value>& args) {
-  v8::TryCatch try_catch(args.GetIsolate());
-  Close(args);
-  if (try_catch.HasCaught()) {
-    CHECK(try_catch.CanContinue());
-  }
-}

Previously, Dispose would call Close and catch any exceptions, discarding them.

New code:

+void Session::Dispose(const FunctionCallbackInfo<Value>& args) {
+  Session* session;
+  ASSIGN_OR_RETURN_UNWRAP(&session, args.This());
+  Environment* env = Environment::GetCurrent(args);
+  if (session->session_ == nullptr) {
+    return;
+  }
+  THROW_AND_RETURN_ON_BAD_STATE(
+      env, session->is_generating_changeset_, "session is currently in use");
+
+  session->Delete();
+}

Now, Dispose checks if the session is already closed (no-op if so), then checks if the session is currently generating a changeset or patchset. If so, it throws ERR_INVALID_STATE, just like close(). Otherwise, it proceeds to delete the session. This ensures consistent error propagation and behavior between disposal and explicit close.

3. Expand tests to cover both close and dispose error cases:

Old test:

-test('session.close() - while generating changes throws exception', (t) => {
-  for (const method of ['changeset', 'patchset']) {
-    ...
-    t.assert.throws(() => session[method](), {
-      code: 'ERR_INVALID_STATE',
-      message: 'session is currently in use',
-    });
-    ...
-  }
-});

New test:

+test('session close and dispose - while generating changes throws exception', (t) => {
+  for (const close of ['close', Symbol.dispose]) {
+    for (const method of ['changeset', 'patchset']) {
+      ...
+      t.assert.throws(() => session[method](), {
+        code: 'ERR_INVALID_STATE',
+        message: 'session is currently in use',
+      });
+      ...
+    }
+  }
+});

This test now checks both close and Symbol.dispose to ensure that both throw ERR_INVALID_STATE when called while the session is in use. Additional tests ensure that disposing a closed session is a no-op, both before and after closing the database.

From the review thread

No notable review discussion on this PR.

Takeaways

• Error handling for resource disposal should be consistent with explicit close methods. If disposing a resource can fail due to its state, the error should be surfaced to the caller, not silently ignored.
• APIs should document and test all edge cases, especially around object lifecycle and error states. Ensuring that documentation and tests reflect the intended behavior prevents subtle bugs and confusion.
• Graceful handling of already-closed resources avoids unnecessary errors and matches user expectations. Disposal or close methods should be idempotent when possible, becoming no-ops if the resource is already closed.