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

Author: trivikr

src/node_sqlite.cc

@@ -1109,7 +1109,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
@@ -1120,7 +1120,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;
   }

test/parallel/test-sqlite-virtual-table.js

@@ -622,6 +622,41 @@ 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:');
+      for (const [label, makeRows] of [
+        ['empty', () => {
+          return {
+            [Symbol.iterator]() { return this; },
+            next() { return { value: [], done: true }; },
+            return() { throw new Error('cleanup boom'); },
+          };
+        }],
+        ['fully_consumed', () => {
+          let i = 0;
+          return {
+            [Symbol.iterator]() { return this; },
+            next() { return { value: [i++], done: i > 2 }; },
+            return() { throw new Error('cleanup boom'); },
+          };
+        }],
+      ]) {
+        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]);
+      }
+    });
+
     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