From b18715aca915567fbd116dbf66625cb5d66f5df5 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Sun, 4 Oct 2026 18:08:49 -0700 Subject: [PATCH 1/2] 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 --- src/node_sqlite.cc | 8 +++-- test/parallel/test-sqlite-virtual-table.js | 35 ++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index b4fbfa88746..11a8533f52a 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; } diff --git a/test/parallel/test-sqlite-virtual-table.js b/test/parallel/test-sqlite-virtual-table.js index c10b343b12b..be41ed73fa0 100644 --- a/test/parallel/test-sqlite-virtual-table.js +++ b/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 From aa5cfc66ff7c063f24ba637f0730cb72ea97fc1b Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue, 20 Oct 2026 23:57:25 -0700 Subject: [PATCH 2/2] 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 --- src/node_sqlite.cc | 4 ++ test/parallel/test-sqlite-virtual-table.js | 78 +++++++++++++++++++++- 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 11a8533f52a..c98ee4d95e8 100644 --- a/src/node_sqlite.cc +++ b/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"); diff --git a/test/parallel/test-sqlite-virtual-table.js b/test/parallel/test-sqlite-virtual-table.js index be41ed73fa0..cc520f72344 100644 --- a/test/parallel/test-sqlite-virtual-table.js +++ b/test/parallel/test-sqlite-virtual-table.js @@ -628,12 +628,17 @@ 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', () => { @@ -641,7 +646,7 @@ suite('Database.prototype.createModule()', () => { 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', () => {