amend! sqlite: skip return() on exhausted virtual table iterators

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.

Likewise, skip return() after an iterator protocol error, where next()
is missing or returns a non-object, as for...of does not close the
iterator in that case.

Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Assisted-by: claude:opus-5.5

Author: trivikr

src/node_sqlite.cc

@@ -1449,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");
@@ -1462,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

@@ -628,20 +628,25 @@ suite('Database.prototype.createModule()', () => {
       // 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() { throw new Error('cleanup boom'); },
+            return: cleanup,
           };
         }],
         ['fully_consumed', () => {
           let i = 0;
           return {
             [Symbol.iterator]() { return this; },
             next() { return { value: [i++], done: i > 2 }; },
-            return() { throw new Error('cleanup boom'); },
+            return: cleanup,
           };
         }],
       ]) {
@@ -655,6 +660,75 @@ suite('Database.prototype.createModule()', () => {
           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', () => {