sqlite: address review feedback on reset error handling

- StatementSyncIterator::Return() no longer throws on a deferred
  reset error, matching the OnScopeLeave guards used elsewhere: it
  is invoked during abrupt iterator completion (e.g. a throw inside
  a for...of body), and throwing there would discard the caller's
  already-pending exception.
- Add a short comment on the accepted SQLITE_ROW result in Run().
- Add tests covering get()/all() surfacing a deferred SQLite error
  from reset() after already building a row/array, the iterator not
  replaying results after natural exhaustion, and Return() not
  discarding a pending exception on a deferred reset error.

Signed-off-by: semimikoh <ejffjeosms@gmail.com>

Author: semimikoh

src/node_sqlite.cc

@@ -3006,6 +3006,9 @@ MaybeLocal<Object> StatementExecutionHelper::Run(Environment* env,
   });
 
   int step_r = sqlite3_step(stmt);
+  // SQLITE_ROW is accepted here (and discarded) so that run() can still be
+  // used on RETURNING/SELECT statements, matching prior behavior of
+  // ignoring the step result entirely.
   if (step_r != SQLITE_DONE && step_r != SQLITE_ROW) {
     THROW_ERR_SQLITE_ERROR(isolate, db);
     return MaybeLocal<Object>();
@@ -3894,8 +3897,11 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo<Value>& args) {
       env, iter->stmt_->IsFinalized(), "statement has been finalized");
   Isolate* isolate = env->isolate();
 
-  RESET_OR_THROW(
-      isolate, iter->stmt_->db_.get(), iter->stmt_->statement_, void());
+  // Unlike Next(), the reset result is intentionally ignored here: Return()
+  // is invoked by the language during abrupt completion (e.g. a `throw`
+  // inside a `for...of` body), and throwing on a deferred SQLite error
+  // would discard the caller's already-pending exception.
+  sqlite3_reset(iter->stmt_->statement_);
   iter->done_ = true;
 
   auto iter_template = getLazyIterTemplate(env);

test/parallel/test-sqlite-statement-sync.js

@@ -79,6 +79,28 @@ suite('StatementSync.prototype.get()', () => {
       message: /statement has been finalized/,
     });
   });
+
+  test('surfaces a deferred SQLite error from reset() even though a row was already built', (t) => {
+    using db = new DatabaseSync(':memory:');
+    db.exec(`
+      PRAGMA foreign_keys = ON;
+      PRAGMA defer_foreign_keys = ON;
+      CREATE TABLE parent(id INTEGER PRIMARY KEY);
+      CREATE TABLE child(id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent(id));
+    `);
+    // The FK check is deferred until the implicit transaction commits, which
+    // happens inside reset() here because RETURNING leaves the statement's
+    // VDBE running after the row is produced.
+    const stmt = db.prepare(
+      'INSERT INTO child (parent_id) VALUES (999) RETURNING id'
+    );
+    t.assert.throws(() => {
+      stmt.get();
+    }, {
+      code: 'ERR_SQLITE_ERROR',
+      message: /FOREIGN KEY constraint failed/,
+    });
+  });
 });
 
 suite('StatementSync.prototype.all()', () => {
@@ -144,6 +166,25 @@ suite('StatementSync.prototype.all()', () => {
       message: /statement has been finalized/,
     });
   });
+
+  test('surfaces a deferred SQLite error from reset() even though the array was already built', (t) => {
+    using db = new DatabaseSync(':memory:');
+    db.exec(`
+      PRAGMA foreign_keys = ON;
+      PRAGMA defer_foreign_keys = ON;
+      CREATE TABLE parent(id INTEGER PRIMARY KEY);
+      CREATE TABLE child(id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent(id));
+    `);
+    const stmt = db.prepare(
+      'INSERT INTO child (parent_id) VALUES (999) RETURNING id'
+    );
+    t.assert.throws(() => {
+      stmt.all();
+    }, {
+      code: 'ERR_SQLITE_ERROR',
+      message: /FOREIGN KEY constraint failed/,
+    });
+  });
 });
 
 suite('StatementSync.prototype.iterate()', () => {
@@ -322,6 +363,49 @@ suite('StatementSync.prototype.iterate()', () => {
       message: /statement has been finalized/,
     });
   });
+
+  test('does not replay results after the iterator is naturally exhausted', (t) => {
+    using db = new DatabaseSync(':memory:');
+    db.exec(`
+      CREATE TABLE test(key TEXT);
+      INSERT INTO test (key) VALUES ('key1');
+    `);
+    const it = db.prepare('SELECT * FROM test').iterate();
+    t.assert.deepStrictEqual(it.next(), {
+      __proto__: null, done: false, value: { __proto__: null, key: 'key1' },
+    });
+    t.assert.deepStrictEqual(
+      it.next(), { __proto__: null, done: true, value: null });
+    // Calling next() again on an exhausted iterator must keep reporting
+    // done, not silently reset the statement and replay from row 1.
+    t.assert.deepStrictEqual(
+      it.next(), { __proto__: null, done: true, value: null });
+  });
+
+  test('does not discard a pending exception with a deferred SQLite error on abrupt exit', (t) => {
+    using db = new DatabaseSync(':memory:');
+    db.exec(`
+      PRAGMA foreign_keys = ON;
+      PRAGMA defer_foreign_keys = ON;
+      CREATE TABLE parent(id INTEGER PRIMARY KEY);
+      CREATE TABLE child(id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent(id));
+    `);
+    // Two RETURNING rows so the statement is still mid-execution (VDBE
+    // running) when the loop body throws after the first row, forcing
+    // iterator.return() to reset a statement with a deferred FK violation
+    // still pending.
+    const stmt = db.prepare(`
+      INSERT INTO child (parent_id)
+      SELECT column1 FROM (VALUES (999), (998))
+      RETURNING id
+    `);
+    const userError = new Error('boom');
+    t.assert.throws(() => {
+      for (const _row of stmt.iterate()) {
+        throw userError;
+      }
+    }, (err) => err === userError);
+  });
 });
 
 suite('StatementSync.prototype.run()', () => {