unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
nodejs/node
mid8/10
error-handlingtesting-strategy

sqlite: check sqlite3_step() and sqlite3_reset() results

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

The problem

The Node.js SQLite binding did not properly check the return value of sqlite3_reset() and sometimes ignored the result of sqlite3_step(). According to SQLite documentation, sqlite3_reset() can surface deferred errors (such as constraint violations) that occurred during the previous sqlite3_step(), but several code paths in src/node_sqlite.cc dropped or ignored these return values. This could cause SQLite errors to be silently ignored, leading to unexpected or incorrect application behavior.

Why it's non-trivial

A naive fix might just check the return value of sqlite3_step() and sqlite3_reset() everywhere, but this can break existing control flow, especially in the presence of exceptions or when statements are used in iterator protocols. Some functions have early returns or may already be unwinding due to exceptions, so care must be taken not to throw a new error and discard a pending exception. The fix must also preserve correct statement reusability and iterator state.

The approach

This PR introduces macros to consistently check the result of sqlite3_reset() and surface errors, and applies them across all relevant code paths. It also adds targeted tests to ensure deferred errors are actually surfaced.

1. Macros for checked reset:
These macros make it easy to check the result of sqlite3_reset() and handle errors uniformly:

+#define RESET_OR_THROW(isolate, db, stmt, ret)                                 \
+  CHECK_ERROR_OR_THROW((isolate), (db), sqlite3_reset((stmt)), SQLITE_OK, (ret))
+
+// Surface deferred SQLite errors that sqlite3_reset() returns from the prior
+// sqlite3_step(). Disables the safety-net reset guard via |needs_reset|.
+#define RESET_AND_CHECK(isolate, db, stmt, needs_reset, ret)                   \
+  do {                                                                         \
+    (needs_reset) = false;                                                     \
+    RESET_OR_THROW((isolate), (db), (stmt), (ret));                            \
+  } while (0)
  • RESET_OR_THROW checks the result of sqlite3_reset() and throws if not SQLITE_OK.
  • RESET_AND_CHECK disables the automatic reset guard and performs the checked reset, ensuring errors are surfaced and not silently ignored.

2. Guarding statement resets:
For functions that may return early or throw, an OnScopeLeave guard ensures the statement is reset, but only if needed, and only if no error is pending:

+  bool needs_reset = true;
+  auto reset = OnScopeLeave([&]() {
+    if (needs_reset) sqlite3_reset(stmt);
+  });

This ensures statements are always left in a reusable state, but doesn't overwrite existing exceptions.

3. Checking reset results after step:
After calling sqlite3_step(), the code now checks for errors both from the step and from the reset, surfacing deferred SQLite errors:

+  int step_r = sqlite3_step(stmt);
+  // SQLITE_ROW is accepted here (and discarded) so that run() can still be
+  // used on RETURNING/SELECT statements, matching prior behavior of
+  // ignoring the step result entirely.
+  if (step_r != SQLITE_DONE && step_r != SQLITE_ROW) {
+    THROW_ERR_SQLITE_ERROR(isolate, db);
+    return MaybeLocal<Object>();
+  }
+  RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal<Object>());
  • This ensures that even if sqlite3_step() appears to succeed, any deferred error surfaced by sqlite3_reset() is not missed.

4. Iterator protocol correctness:
For synchronous iterators, the logic ensures that the iterator is properly marked as done and that errors from reset are surfaced, but doesn't discard user exceptions:

@@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
   if (r != SQLITE_ROW) {
     CHECK_ERROR_OR_THROW(
         env->isolate(), iter->stmt_->db_.get(), r, SQLITE_DONE, void());
-    sqlite3_reset(iter->stmt_->statement_);
     iter->done_ = true;
+    RESET_OR_THROW(env->isolate(),
+                   iter->stmt_->db_.get(),
+                   iter->stmt_->statement_,
+                   void());
  • iter->done_ = true; is set before the checked reset, ensuring correct iterator state even if an error is thrown.

In Return(), reset errors are intentionally ignored to avoid discarding a pending user exception:

+  // Unlike Next(), the reset result is intentionally ignored here: Return()
+  // is invoked by the language during abrupt completion (e.g. a `throw`
+  // inside a `for...of` body), and throwing on a deferred SQLite error
+  // would discard the caller's already-pending exception.
   sqlite3_reset(iter->stmt_->statement_);
   iter->done_ = true;

5. Tests for deferred errors:
New tests ensure that deferred errors from sqlite3_reset() are actually thrown, even if a row was already produced:

+  test('surfaces a deferred SQLite error from reset() even though a row was already built', (t) => {
+    using db = new DatabaseSync(':memory:');
+    db.exec(`
+      PRAGMA foreign_keys = ON;
+      PRAGMA defer_foreign_keys = ON;
+      CREATE TABLE parent(id INTEGER PRIMARY KEY);
+      CREATE TABLE child(id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent(id));
+    `);
+    // The FK check is deferred until the implicit transaction commits, which
+    // happens inside reset() here because RETURNING leaves the statement's
+    // VDBE running after the row is produced.
+    const stmt = db.prepare(
+      'INSERT INTO child (parent_id) VALUES (999) RETURNING id'
+    );
+    t.assert.throws(() => {
+      stmt.get();
+    }, {
+      code: 'ERR_SQLITE_ERROR',
+      message: /FOREIGN KEY constraint failed/,
+    });
+  });
  • This test verifies that a foreign key violation, deferred until reset, is not missed.

From the review thread

CHECK_ERROR_OR_THROW does return (ret);, so when this reset reports a deferred error, iter->done_ = true on the next line is skipped — but sqlite3_reset() has already reset the statement, and reset_generation_ wasn't bumped (this is a raw reset, not ResetStatement()). So:

const it = stmt.iterate();
try { for (const row of it) break; } catch {}  // it.return() throws
it.next();  // done_ === false, generation matches -> re-steps from row 1

Setting done_ = true before the checked reset fixes it.

Separately: iterator.return() is called by the language during abrupt completion, including exception unwinding (for (const row of it) { throw err; }). Throwing here discards the user's pending exception, which is exactly what the PR description's "avoid replacing an already-pending exception" rule is meant to prevent — and this is the one place it isn't applied. Worth considering whether Return() should keep ignoring the reset result.

After a SQLITE_ROW step the VDBE is still in RUN_STATE, so this reset runs sqlite3VdbeHalt() and can genuinely fail

Takeaways

  • Always check the result of resource cleanup functions (like sqlite3_reset()), as they may surface deferred errors not caught earlier.
  • When writing error-handling logic, be careful not to discard or overwrite pending exceptions—especially in language-level protocols like iterators.
  • Automated tests should cover not just immediate errors, but also deferred or transactional errors that may be surfaced only during cleanup.
  • Use scope guards or similar patterns to ensure resources are consistently cleaned up, but allow for disabling them when a more explicit error path is needed.