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>

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");

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