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.ccDone processing src/node_sqlite.cc
$ git diff --check -- src/node_sqlite.ccA full local build was not completed because this machine has Apple clang16.0.0, while the current tree requires a newer macOS toolchain. The buildfails in V8 because std::atomic_ref is unavailable.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (e6fa5bf) to head (90fe5e2).
⚠️ Report is 38 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #63319 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.01% 
==========================================
Files 759 759 Lines 248328 248352 +24 Branches 46856 46868 +12 ==========================================
- Hits 224296 224293 -3 + Misses 15466 15459 -7 - Partials 8566 8600 +34 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	81.03% <75.00%> (-0.20%)	⬇️

... and 45 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.

[semimikoh]

Fixed — swapped the order so done_ is set before the checked reset, matching your suggestion.

On the test request: I don't think a failing-reset test is constructible for this specific branch. This code path is only reached after CHECK_ERROR_OR_THROW has already confirmed r == SQLITE_DONE, and in SQLite's own implementation (sqlite3VdbeReset(), vdbeapi.c):

/* If the VM did not run to completion or if it encountered an** error, then it might not have been halted properly. So halt** it now.*/if( p->eVdbeState==VDBE_RUN_STATE ) sqlite3VdbeHalt(p);

sqlite3VdbeHalt() — where commits and constraint checks actually happen — only runs if the VDBE is still in VDBE_RUN_STATE. Once sqlite3_step() has returned SQLITE_DONE, the halt already happened during that step call, so a subsequent sqlite3_reset() can't trigger a new commit-time failure here. This matches @TrevorBurnham's earlier note that the equivalent SQLITE_DONE check in Get() is "effectively a no-op."

The sqlite3_reset() doc's BUSY/RETURNING example specifically requires stopping after SQLITE_ROW (VDBE still RUN_STATE) — that's the Return()/Get()/All() case, which is already covered. Happy to add the reset-fails test there if it's not covered well enough, but for this particular Next() branch I believe it's unreachable by construction.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75705/

[nodejs-github-bot]

Landed in 673cdef

[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.ccDone processing src/node_sqlite.cc
$ git diff --check -- src/node_sqlite.ccA full local build was not completed because this machine has Apple clang16.0.0, while the current tree requires a newer macOS toolchain. The buildfails in V8 because std::atomic_ref is unavailable.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (e6fa5bf) to head (90fe5e2).
⚠️ Report is 38 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #63319 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.01% 
==========================================
Files 759 759 Lines 248328 248352 +24 Branches 46856 46868 +12 ==========================================
- Hits 224296 224293 -3 + Misses 15466 15459 -7 - Partials 8566 8600 +34 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	81.03% <75.00%> (-0.20%)	⬇️

... and 45 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.

[semimikoh]

Fixed — swapped the order so done_ is set before the checked reset, matching your suggestion.

On the test request: I don't think a failing-reset test is constructible for this specific branch. This code path is only reached after CHECK_ERROR_OR_THROW has already confirmed r == SQLITE_DONE, and in SQLite's own implementation (sqlite3VdbeReset(), vdbeapi.c):

/* If the VM did not run to completion or if it encountered an** error, then it might not have been halted properly. So halt** it now.*/if( p->eVdbeState==VDBE_RUN_STATE ) sqlite3VdbeHalt(p);

sqlite3VdbeHalt() — where commits and constraint checks actually happen — only runs if the VDBE is still in VDBE_RUN_STATE. Once sqlite3_step() has returned SQLITE_DONE, the halt already happened during that step call, so a subsequent sqlite3_reset() can't trigger a new commit-time failure here. This matches @TrevorBurnham's earlier note that the equivalent SQLITE_DONE check in Get() is "effectively a no-op."

The sqlite3_reset() doc's BUSY/RETURNING example specifically requires stopping after SQLITE_ROW (VDBE still RUN_STATE) — that's the Return()/Get()/All() case, which is already covered. Happy to add the reset-fails test there if it's not covered well enough, but for this particular Next() branch I believe it's unreachable by construction.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75705/

[nodejs-github-bot]

Landed in 673cdef

[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.ccDone processing src/node_sqlite.cc
$ git diff --check -- src/node_sqlite.ccA full local build was not completed because this machine has Apple clang16.0.0, while the current tree requires a newer macOS toolchain. The buildfails in V8 because std::atomic_ref is unavailable.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (e6fa5bf) to head (90fe5e2).
⚠️ Report is 38 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #63319 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.01% 
==========================================
Files 759 759 Lines 248328 248352 +24 Branches 46856 46868 +12 ==========================================
- Hits 224296 224293 -3 + Misses 15466 15459 -7 - Partials 8566 8600 +34 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	81.03% <75.00%> (-0.20%)	⬇️

... and 45 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.

[semimikoh]

Fixed — swapped the order so done_ is set before the checked reset, matching your suggestion.

On the test request: I don't think a failing-reset test is constructible for this specific branch. This code path is only reached after CHECK_ERROR_OR_THROW has already confirmed r == SQLITE_DONE, and in SQLite's own implementation (sqlite3VdbeReset(), vdbeapi.c):

/* If the VM did not run to completion or if it encountered an** error, then it might not have been halted properly. So halt** it now.*/if( p->eVdbeState==VDBE_RUN_STATE ) sqlite3VdbeHalt(p);

sqlite3VdbeHalt() — where commits and constraint checks actually happen — only runs if the VDBE is still in VDBE_RUN_STATE. Once sqlite3_step() has returned SQLITE_DONE, the halt already happened during that step call, so a subsequent sqlite3_reset() can't trigger a new commit-time failure here. This matches @TrevorBurnham's earlier note that the equivalent SQLITE_DONE check in Get() is "effectively a no-op."

The sqlite3_reset() doc's BUSY/RETURNING example specifically requires stopping after SQLITE_ROW (VDBE still RUN_STATE) — that's the Return()/Get()/All() case, which is already covered. Happy to add the reset-fails test there if it's not covered well enough, but for this particular Next() branch I believe it's unreachable by construction.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75705/

[nodejs-github-bot]

Landed in 673cdef

[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.ccDone processing src/node_sqlite.cc
$ git diff --check -- src/node_sqlite.ccA full local build was not completed because this machine has Apple clang16.0.0, while the current tree requires a newer macOS toolchain. The buildfails in V8 because std::atomic_ref is unavailable.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (e6fa5bf) to head (90fe5e2).
⚠️ Report is 38 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #63319 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.01% 
==========================================
Files 759 759 Lines 248328 248352 +24 Branches 46856 46868 +12 ==========================================
- Hits 224296 224293 -3 + Misses 15466 15459 -7 - Partials 8566 8600 +34 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	81.03% <75.00%> (-0.20%)	⬇️

... and 45 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.

[semimikoh]

Fixed — swapped the order so done_ is set before the checked reset, matching your suggestion.

On the test request: I don't think a failing-reset test is constructible for this specific branch. This code path is only reached after CHECK_ERROR_OR_THROW has already confirmed r == SQLITE_DONE, and in SQLite's own implementation (sqlite3VdbeReset(), vdbeapi.c):

/* If the VM did not run to completion or if it encountered an** error, then it might not have been halted properly. So halt** it now.*/if( p->eVdbeState==VDBE_RUN_STATE ) sqlite3VdbeHalt(p);

sqlite3VdbeHalt() — where commits and constraint checks actually happen — only runs if the VDBE is still in VDBE_RUN_STATE. Once sqlite3_step() has returned SQLITE_DONE, the halt already happened during that step call, so a subsequent sqlite3_reset() can't trigger a new commit-time failure here. This matches @TrevorBurnham's earlier note that the equivalent SQLITE_DONE check in Get() is "effectively a no-op."

The sqlite3_reset() doc's BUSY/RETURNING example specifically requires stopping after SQLITE_ROW (VDBE still RUN_STATE) — that's the Return()/Get()/All() case, which is already covered. Happy to add the reset-fails test there if it's not covered well enough, but for this particular Next() branch I believe it's unreachable by construction.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75705/

[nodejs-github-bot]

Landed in 673cdef

[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.ccDone processing src/node_sqlite.cc
$ git diff --check -- src/node_sqlite.ccA full local build was not completed because this machine has Apple clang16.0.0, while the current tree requires a newer macOS toolchain. The buildfails in V8 because std::atomic_ref is unavailable.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (e6fa5bf) to head (90fe5e2).
⚠️ Report is 38 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #63319 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.01% 
==========================================
Files 759 759 Lines 248328 248352 +24 Branches 46856 46868 +12 ==========================================
- Hits 224296 224293 -3 + Misses 15466 15459 -7 - Partials 8566 8600 +34 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	81.03% <75.00%> (-0.20%)	⬇️

... and 45 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.

[semimikoh]

Fixed — swapped the order so done_ is set before the checked reset, matching your suggestion.

On the test request: I don't think a failing-reset test is constructible for this specific branch. This code path is only reached after CHECK_ERROR_OR_THROW has already confirmed r == SQLITE_DONE, and in SQLite's own implementation (sqlite3VdbeReset(), vdbeapi.c):

/* If the VM did not run to completion or if it encountered an** error, then it might not have been halted properly. So halt** it now.*/if( p->eVdbeState==VDBE_RUN_STATE ) sqlite3VdbeHalt(p);

sqlite3VdbeHalt() — where commits and constraint checks actually happen — only runs if the VDBE is still in VDBE_RUN_STATE. Once sqlite3_step() has returned SQLITE_DONE, the halt already happened during that step call, so a subsequent sqlite3_reset() can't trigger a new commit-time failure here. This matches @TrevorBurnham's earlier note that the equivalent SQLITE_DONE check in Get() is "effectively a no-op."

The sqlite3_reset() doc's BUSY/RETURNING example specifically requires stopping after SQLITE_ROW (VDBE still RUN_STATE) — that's the Return()/Get()/All() case, which is already covered. Happy to add the reset-fails test there if it's not covered well enough, but for this particular Next() branch I believe it's unreachable by construction.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75705/

[nodejs-github-bot]

Landed in 673cdef

[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.ccDone processing src/node_sqlite.cc
$ git diff --check -- src/node_sqlite.ccA full local build was not completed because this machine has Apple clang16.0.0, while the current tree requires a newer macOS toolchain. The buildfails in V8 because std::atomic_ref is unavailable.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (e6fa5bf) to head (90fe5e2).
⚠️ Report is 38 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #63319 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.01% 
==========================================
Files 759 759 Lines 248328 248352 +24 Branches 46856 46868 +12 ==========================================
- Hits 224296 224293 -3 + Misses 15466 15459 -7 - Partials 8566 8600 +34 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	81.03% <75.00%> (-0.20%)	⬇️

... and 45 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.

[semimikoh]

Fixed — swapped the order so done_ is set before the checked reset, matching your suggestion.

On the test request: I don't think a failing-reset test is constructible for this specific branch. This code path is only reached after CHECK_ERROR_OR_THROW has already confirmed r == SQLITE_DONE, and in SQLite's own implementation (sqlite3VdbeReset(), vdbeapi.c):

/* If the VM did not run to completion or if it encountered an** error, then it might not have been halted properly. So halt** it now.*/if( p->eVdbeState==VDBE_RUN_STATE ) sqlite3VdbeHalt(p);

sqlite3VdbeHalt() — where commits and constraint checks actually happen — only runs if the VDBE is still in VDBE_RUN_STATE. Once sqlite3_step() has returned SQLITE_DONE, the halt already happened during that step call, so a subsequent sqlite3_reset() can't trigger a new commit-time failure here. This matches @TrevorBurnham's earlier note that the equivalent SQLITE_DONE check in Get() is "effectively a no-op."

The sqlite3_reset() doc's BUSY/RETURNING example specifically requires stopping after SQLITE_ROW (VDBE still RUN_STATE) — that's the Return()/Get()/All() case, which is already covered. Happy to add the reset-fails test there if it's not covered well enough, but for this particular Next() branch I believe it's unreachable by construction.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75705/

[nodejs-github-bot]

Landed in 673cdef

[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.ccDone processing src/node_sqlite.cc
$ git diff --check -- src/node_sqlite.ccA full local build was not completed because this machine has Apple clang16.0.0, while the current tree requires a newer macOS toolchain. The buildfails in V8 because std::atomic_ref is unavailable.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (e6fa5bf) to head (90fe5e2).
⚠️ Report is 38 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #63319 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.01% 
==========================================
Files 759 759 Lines 248328 248352 +24 Branches 46856 46868 +12 ==========================================
- Hits 224296 224293 -3 + Misses 15466 15459 -7 - Partials 8566 8600 +34 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	81.03% <75.00%> (-0.20%)	⬇️

... and 45 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.

[semimikoh]

Fixed — swapped the order so done_ is set before the checked reset, matching your suggestion.

On the test request: I don't think a failing-reset test is constructible for this specific branch. This code path is only reached after CHECK_ERROR_OR_THROW has already confirmed r == SQLITE_DONE, and in SQLite's own implementation (sqlite3VdbeReset(), vdbeapi.c):

/* If the VM did not run to completion or if it encountered an** error, then it might not have been halted properly. So halt** it now.*/if( p->eVdbeState==VDBE_RUN_STATE ) sqlite3VdbeHalt(p);

sqlite3VdbeHalt() — where commits and constraint checks actually happen — only runs if the VDBE is still in VDBE_RUN_STATE. Once sqlite3_step() has returned SQLITE_DONE, the halt already happened during that step call, so a subsequent sqlite3_reset() can't trigger a new commit-time failure here. This matches @TrevorBurnham's earlier note that the equivalent SQLITE_DONE check in Get() is "effectively a no-op."

The sqlite3_reset() doc's BUSY/RETURNING example specifically requires stopping after SQLITE_ROW (VDBE still RUN_STATE) — that's the Return()/Get()/All() case, which is already covered. Happy to add the reset-fails test there if it's not covered well enough, but for this particular Next() branch I believe it's unreachable by construction.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75705/

[nodejs-github-bot]

Landed in 673cdef

[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.ccDone processing src/node_sqlite.cc
$ git diff --check -- src/node_sqlite.ccA full local build was not completed because this machine has Apple clang16.0.0, while the current tree requires a newer macOS toolchain. The buildfails in V8 because std::atomic_ref is unavailable.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (e6fa5bf) to head (90fe5e2).
⚠️ Report is 38 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #63319 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.01% 
==========================================
Files 759 759 Lines 248328 248352 +24 Branches 46856 46868 +12 ==========================================
- Hits 224296 224293 -3 + Misses 15466 15459 -7 - Partials 8566 8600 +34 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	81.03% <75.00%> (-0.20%)	⬇️

... and 45 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.

[semimikoh]

Fixed — swapped the order so done_ is set before the checked reset, matching your suggestion.

On the test request: I don't think a failing-reset test is constructible for this specific branch. This code path is only reached after CHECK_ERROR_OR_THROW has already confirmed r == SQLITE_DONE, and in SQLite's own implementation (sqlite3VdbeReset(), vdbeapi.c):

/* If the VM did not run to completion or if it encountered an** error, then it might not have been halted properly. So halt** it now.*/if( p->eVdbeState==VDBE_RUN_STATE ) sqlite3VdbeHalt(p);

sqlite3VdbeHalt() — where commits and constraint checks actually happen — only runs if the VDBE is still in VDBE_RUN_STATE. Once sqlite3_step() has returned SQLITE_DONE, the halt already happened during that step call, so a subsequent sqlite3_reset() can't trigger a new commit-time failure here. This matches @TrevorBurnham's earlier note that the equivalent SQLITE_DONE check in Get() is "effectively a no-op."

The sqlite3_reset() doc's BUSY/RETURNING example specifically requires stopping after SQLITE_ROW (VDBE still RUN_STATE) — that's the Return()/Get()/All() case, which is already covered. Happy to add the reset-fails test there if it's not covered well enough, but for this particular Next() branch I believe it's unreachable by construction.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75705/

[nodejs-github-bot]

Landed in 673cdef