Commit cfb2de3db88 for nodejs
commit cfb2de3db88d069760f27cb9c3c045cf13e92350
Author: Trivikram Kamat <trivikr.dev@gmail.com>
Date: Sat Oct 24 23:39:02 2026 -0700
sqlite: skip return() on exhausted virtual table iterators
Closing a virtual table cursor invoked the iterator's return() even
after next() had already reported done: true, deviating from for...of
semantics where return() only runs on abrupt termination. An empty
custom iterable with a throwing return() made an otherwise successful
query fail.
Only close the iterator when the scan stopped early, as with LIMIT, a
break, or a cursor re-filter; a normally finished iterator already ran
its cleanup.
Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Assisted-by: claude:opus-5.5
PR-URL: https://github.com/nodejs/node/pull/66519
Fixes: https://github.com/nodejs/node/issues/66518
Reviewed-By: Guilherme Araújo <arauujogui@gmail.com>
diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc
index 705d9b4a79a..2c1bba2c9b4 100644
--- a/src/node_sqlite.cc
+++ b/src/node_sqlite.cc
@@ -1120,7 +1120,7 @@ bool VirtualTableModule::CanCallIntoJS() const {
bool VirtualTableModule::CloseIterator(NodeVTabCursor* cursor) {
VirtualTableModule* mod = cursor->module;
- // Skipped in two cases:
+ // Skipped in three cases:
//
// - While the database is being torn down from a destructor, because those
// run from a garbage collection callback where JavaScript cannot be
@@ -1131,7 +1131,11 @@ bool VirtualTableModule::CloseIterator(NodeVTabCursor* cursor) {
// A generator whose own body threw has already run its `finally` as part of
// that throw, so this only affects an iterator abandoned while suspended
// because something else failed.
- if (cursor->iterator.IsEmpty() || !mod->CanCallIntoJS() ||
+ // - When next() has already reported `done: true`, because the iteration ran
+ // to completion. `return()` is only called on abrupt termination in
+ // `for...of`; a finished iterator must not be asked to clean up again, and
+ // a custom iterable may throw from `return()` even after completion.
+ if (cursor->iterator.IsEmpty() || cursor->done || !mod->CanCallIntoJS() ||
mod->env_->isolate()->HasPendingException()) {
return false;
}
@@ -1445,7 +1449,10 @@ int VirtualTableModule::xNext(sqlite3_vtab_cursor* pCursor) {
.ToLocal(&next_method_val)) {
return mod->PropagateJSError();
}
+ // On a protocol error, drop the iterator so it is not closed later.
+ // `for...of` does not call return() when next() is invalid either.
if (!next_method_val->IsFunction()) {
+ cursor->iterator.Reset();
return ReportProtocolError(
pCursor->pVtab,
"The \"options.rows\" iterator must have a next() method");
@@ -1458,6 +1465,7 @@ int VirtualTableModule::xNext(sqlite3_vtab_cursor* pCursor) {
return mod->PropagateJSError();
}
if (!next_val->IsObject()) {
+ cursor->iterator.Reset();
return ReportProtocolError(
pCursor->pVtab,
"The \"options.rows\" iterator's next() method must return an object");
diff --git a/test/parallel/test-sqlite-virtual-table.js b/test/parallel/test-sqlite-virtual-table.js
index c10b343b12b..cc520f72344 100644
--- a/test/parallel/test-sqlite-virtual-table.js
+++ b/test/parallel/test-sqlite-virtual-table.js
@@ -622,6 +622,115 @@ suite('Database.prototype.createModule()', () => {
assert.deepStrictEqual(cleanedUp, [1, 2]);
});
+ test('does not call return() on a normally exhausted iterator', () => {
+ // Once next() reports `done: true` the iterator is finished, so no
+ // cleanup should run. A throwing return() must not fail an otherwise
+ // successful query. Matches `for...of`, which only calls return() on
+ // abrupt termination.
+ const db = new Database(':memory:');
+ let returnCalls = 0;
+ function cleanup() {
+ returnCalls++;
+ throw new Error('cleanup boom');
+ }
+ for (const [label, makeRows] of [
+ ['empty', () => {
+ return {
+ [Symbol.iterator]() { return this; },
+ next() { return { value: [], done: true }; },
+ return: cleanup,
+ };
+ }],
+ ['fully_consumed', () => {
+ let i = 0;
+ return {
+ [Symbol.iterator]() { return this; },
+ next() { return { value: [i++], done: i > 2 }; },
+ return: cleanup,
+ };
+ }],
+ ]) {
+ db.createModule(`exhausted_${label}`, {
+ columns: [{ name: 'v', type: 'INTEGER' }],
+ rows: makeRows,
+ });
+
+ const rows = db.prepare(`SELECT v FROM exhausted_${label}`).all();
+ assert.deepStrictEqual(
+ rows.map((row) => row.v),
+ label === 'empty' ? [] : [0, 1]);
+ }
+ assert.strictEqual(returnCalls, 0);
+ });
+
+ test('does not call return() on an exhausted iterator when re-filtered', () => {
+ // A scalar subquery drains the inner iterator for each outer row, so
+ // xFilter re-filters a cursor whose iterator already reported done.
+ const db = new Database(':memory:');
+ let returnCalls = 0;
+
+ db.createModule('exhausted_refilter', {
+ columns: [
+ { name: 'v', type: 'INTEGER' },
+ { name: 'input', type: 'INTEGER', hidden: true },
+ ],
+ rows() {
+ let i = 0;
+ return {
+ [Symbol.iterator]() { return this; },
+ next() { return { value: [i++], done: i > 2 }; },
+ return() {
+ returnCalls++;
+ throw new Error('cleanup boom');
+ },
+ };
+ },
+ });
+
+ db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1), (2), (3)');
+ const rows = db.prepare(
+ 'SELECT a, (SELECT count(*) FROM exhausted_refilter(t.a)) AS c FROM t'
+ ).all();
+ assert.deepStrictEqual(rows, [
+ { __proto__: null, a: 1, c: 2 },
+ { __proto__: null, a: 2, c: 2 },
+ { __proto__: null, a: 3, c: 2 },
+ ]);
+ assert.strictEqual(returnCalls, 0);
+ });
+
+ test('does not call return() after an iterator protocol error', () => {
+ // `for...of` does not close an iterator whose next() is invalid, so the
+ // protocol error is reported and return() is never called.
+ const db = new Database(':memory:');
+ let returnCalls = 0;
+ for (const [label, next, message] of [
+ ['next_not_function', 5,
+ /iterator must have a next\(\) method/],
+ ['next_non_object', () => 42,
+ /iterator's next\(\) method must return an object/],
+ ]) {
+ db.createModule(`protocol_${label}`, {
+ columns: [{ name: 'v', type: 'INTEGER' }],
+ rows() {
+ return {
+ [Symbol.iterator]() { return this; },
+ next,
+ return() {
+ returnCalls++;
+ throw new Error('cleanup boom');
+ },
+ };
+ },
+ });
+
+ assert.throws(
+ () => db.prepare(`SELECT v FROM protocol_${label}`).all(),
+ { code: 'ERR_SQLITE_ERROR', message });
+ }
+ assert.strictEqual(returnCalls, 0);
+ });
+
test('does not run cleanup when the statement is collected', () => {
// The destructor runs from a GC callback, where JavaScript cannot be
// executed. An abandoned generator does not run `finally` in JavaScript