sqlite: check sqlite3_step() and sqlite3_reset() results

[semimikoh]

Summary

Per the SQLite docs, sqlite3_reset(S) may return a deferred error code
from the prior sqlite3_step(S) call. Several statement execution paths in
src/node_sqlite.cc dropped that return value, which could silently ignore
SQLite errors.

This also checks the previously ignored sqlite3_step() result in
StatementExecutionHelper::Run().

Fixes: #63311

Approach

Successful execution paths now explicitly check sqlite3_reset().

Functions with early-return or V8-exception paths keep an OnScopeLeave
reset guard so prepared statements are left reusable. The guard intentionally
drops the reset result to avoid replacing an already-pending SQLite or V8
exception.

StatementSyncIterator::Next() and StatementSyncIterator::Return() use a
direct checked reset because their control flow is linear.

$ python3 tools/cpplint.py src/node_sqlite.cc

Done processing src/node_sqlite.cc

$ git diff --check -- src/node_sqlite.cc

A full local build was not completed because this machine has Apple clang
16.0.0, while the current tree requires a newer macOS toolchain. The build
fails in V8 because std::atomic_ref is unavailable.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 74.28571% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (6c862f4) to head (52592e6).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	74.28%	2 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63319      +/-   ##
==========================================
+ Coverage   90.29%   90.31%   +0.01%     
==========================================
  Files         759      759              
  Lines      248295   248322      +27     
  Branches    46861    46876      +15     
==========================================
+ Hits       224205   224268      +63     
+ Misses      15517    15473      -44     
- Partials     8573     8581       +8     

Files with missing lines	Coverage Δ
src/node_sqlite.cc	80.64% <74.28%> (-0.21%)	⬇️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[geeksilva97]

I tried to reproduce a situation where the current code wouldn't catch the error but I couldn't. Please, get such a case into a test so it's clear which situation we must cover.

[TrevorBurnham]

Reviewed the reset-error handling. The direction looks right; sqlite3_reset() returning a deferred error from the prior sqlite3_step() is real and worth surfacing. Two things I'd want addressed:

1. In StatementSyncIterator, RESET_OR_THROW expands to a return, so the new throwing paths skip iter->done_ = true even though the statement has already been reset. A caught error then leaves the iterator resumable, and it replays the result set from the top. Details inline.

2. No tests. get()/all() can now throw where they previously returned an already-built row/array, which is a user-visible change on a success path. Worth coverage pinning the new behavior, plus a note on whether it needs semver-major.

Things I checked that look correct: needs_reset = false is sequenced before sqlite3_reset(), so there's no double reset on the throwing path; no function can call THROW_ERR_SQLITE_ERROR twice, so the ShouldIgnoreSQLiteError() one-shot isn't consumed twice; void() threads through both macro layers; and every RESET_AND_CHECK caller keeps its OnScopeLeave safety net for the earlier failure paths.

[semimikoh]

Rebased onto main and addressed the feedback:

• Return() now ignores the reset result (like the other OnScopeLeave guards) so it can't discard a pending exception during abrupt iterator close
• Next()'s exhaustion path setting done_ came in via the rebase
• Added tests for both, plus the get()/all() deferred-error case
• Added the suggested comment in Run()

get()/all() can now throw on a previously-succeeding path — let me know if this needs notable-change.

[semimikoh]

@trivikr CI is green now (conflicts resolved, lint fixed). Ready for another look whenever you have time.

[trivikr]

RESET_OR_THROW returns early on failure, so a deferred error here skips iter->done_ = true on the line below. The statement still got reset, and since this is a raw reset the generation isn't bumped. If the caller catches that error, the next next() silently re-steps from row 1

Setting done_ first avoids it, and is safe because the reset happened either way:

Suggested change

	RESET_OR_THROW(env->isolate(),
	iter->stmt_->db_.get(),
	iter->stmt_->statement_,
	void());
	iter->done_ = true;
	iter->done_ = true;
	RESET_OR_THROW(env->isolate(),
	iter->stmt_->db_.get(),
	iter->stmt_->statement_,
	void());

Can you write a test too? The new exhaustion test only covers the case where the reset succeeds.