sqlite: run generator return() on cursor refilter

SQLite re-invokes xFilter on a cursor it already used, as it does
for the inner table of a correlated subquery or join, abandoning the
previous iterator mid-loop. Call its return() method so generator
`finally` blocks still run.

Refactor the xClose cleanup intoCloseIterator() so both paths share it;
on refilter, a throwing cleanup is surfaced as the query error.

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: opencode

Author: trivikr

src/node_sqlite.cc

@@ -1097,6 +1097,50 @@ bool VirtualTableModule::CanCallIntoJS() const {
   return db_ && !db_->IsInDestructor();
 }
 
+bool VirtualTableModule::CloseIterator(NodeVTabCursor* cursor) {
+  VirtualTableModule* mod = cursor->module;
+
+  // Skipped in two cases:
+  //
+  // - While the database is being torn down from a destructor, because those
+  //   run from a garbage collection callback where JavaScript cannot be
+  //   executed. An abandoned generator does not run `finally` in JavaScript
+  //   either, so skipping matches the language.
+  // - When an error is already pending, because calling into JavaScript would
+  //   discard it and the caller would see an empty result instead of the error.
+  //   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() ||
+      mod->env_->isolate()->HasPendingException()) {
+    return false;
+  }
+
+  Environment* env = mod->env_;
+  Isolate* isolate = env->isolate();
+  HandleScope handle_scope(isolate);
+  CallbackDepthGuard callback_guard(mod->db_.get());
+
+  // Scoped above the property lookup so a throwing `return` getter is
+  // handled the same way as a throwing `return()` method.
+  TryCatch try_catch(isolate);
+  Local<Object> iterator = cursor->iterator.Get(isolate);
+  Local<Value> return_method;
+  if (iterator->Get(env->context(), FIXED_ONE_BYTE_STRING(isolate, "return"))
+          .ToLocal(&return_method) &&
+      return_method->IsFunction()) {
+    USE(return_method.As<Function>()->Call(
+        env->context(), iterator, 0, nullptr));
+  }
+
+  // Re-throw so that a throwing `finally` is not silently discarded.
+  if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
+    try_catch.ReThrow();
+    return true;
+  }
+  return false;
+}
+
 void VirtualTableModule::ReleaseHiddenValues(NodeVTabCursor* cursor) {
   for (sqlite3_value*& value : cursor->hidden_values) {
     if (value != nullptr) {
@@ -1216,44 +1260,9 @@ int VirtualTableModule::xClose(sqlite3_vtab_cursor* pCursor) {
 
   // Close the iterator so generator `finally` blocks still run when SQLite
   // stops stepping early, as it does for LIMIT or a `break` out of a for...of
-  // loop. Skipped in two cases:
-  //
-  // - While the database is being torn down from a destructor, because those
-  //   run from a garbage collection callback where JavaScript cannot be
-  //   executed. An abandoned generator does not run `finally` in JavaScript
-  //   either, so skipping matches the language.
-  // - When an error is already pending, because calling into JavaScript would
-  //   discard it and the caller would see an empty result instead of the error.
-  //   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() &&
-      !mod->env_->isolate()->HasPendingException()) {
-    Environment* env = mod->env_;
-    Isolate* isolate = env->isolate();
-    HandleScope handle_scope(isolate);
-    CallbackDepthGuard callback_guard(mod->db_.get());
-
-    // Scoped above the property lookup so a throwing `return` getter is
-    // handled the same way as a throwing `return()` method.
-    TryCatch try_catch(isolate);
-    Local<Object> iterator = cursor->iterator.Get(isolate);
-    Local<Value> return_method;
-    if (iterator->Get(env->context(), FIXED_ONE_BYTE_STRING(isolate, "return"))
-            .ToLocal(&return_method) &&
-        return_method->IsFunction()) {
-      USE(return_method.As<Function>()->Call(
-          env->context(), iterator, 0, nullptr));
-    }
-
-    // Re-throw so that a throwing `finally` is not silently discarded. SQLite
-    // discards xClose's return value, so there is no SQLite error here to
-    // suppress; calling PropagateJSError would leave the suppression flag set
-    // and swallow the next unrelated SQLite error.
-    if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
-      try_catch.ReThrow();
-    }
-  }
+  // loop. SQLite discards xClose's return value, so a throwing cleanup is
+  // re-thrown rather than paired with a SQLite error here.
+  mod->CloseIterator(cursor);
 
   ReleaseHiddenValues(cursor);
   cursor->iterator.Reset();
@@ -1277,6 +1286,15 @@ int VirtualTableModule::xFilter(sqlite3_vtab_cursor* pCursor,
   }
   CallbackDepthGuard callback_guard(mod->db_.get());
 
+  // Re-filtering a cursor occurs when SQLite re-invokes xFilter on a cursor it
+  // already used, as it does for the inner table of a correlated subquery or
+  // join. The previous iterator is abandoned mid-loop, so close it the same way
+  // xClose does; otherwise its generator `finally` blocks never run. A throwing
+  // cleanup is surfaced as the error for this query.
+  if (mod->CloseIterator(cursor)) {
+    return mod->PropagateJSError();
+  }
+
   cursor->rowid = 0;
   cursor->done = false;
   cursor->iterator.Reset();

src/node_sqlite.h

@@ -717,6 +717,11 @@ class VirtualTableModule {
   // from a garbage collection callback where JavaScript cannot be executed.
   bool CanCallIntoJS() const;
 
+  // Runs the iterator's return() method so generator `finally` blocks still run
+  // when an iterator is abandoned while suspended. Returns true if the return()
+  // threw; the exception is re-thrown for the caller to surface.
+  bool CloseIterator(NodeVTabCursor* cursor);
+
   static void ReleaseHiddenValues(NodeVTabCursor* cursor);
 
   Environment* env_;

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

@@ -558,6 +558,36 @@ suite('DatabaseSync.prototype.createModule()', () => {
       }
     });
 
+    test('closes the iterator when a filter is reapplied to the cursor', () => {
+      // A correlated subquery re-invokes xFilter on the same cursor per outer
+      // row, abandoning the previous iterator; its `finally` must still run.
+      const db = new DatabaseSync(':memory:');
+      const cleanedUp = [];
+
+      db.createModule('refilter_cleanup', {
+        columns: [
+          { name: 'input', type: 'INTEGER', hidden: true },
+        ],
+        *rows(input) {
+          try {
+            yield [input];
+          } finally {
+            cleanedUp.push(input);
+          }
+        },
+      });
+
+      db.exec('CREATE TABLE t(a); INSERT INTO t VALUES (1), (2)');
+      const rows = db.prepare(
+        'SELECT a FROM t WHERE EXISTS(SELECT 1 FROM refilter_cleanup(t.a))'
+      ).all();
+      assert.deepStrictEqual(rows, [
+        { __proto__: null, a: 1 },
+        { __proto__: null, a: 2 },
+      ]);
+      assert.deepStrictEqual(cleanedUp, [1, 2]);
+    });
+
     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