margelo / margelo/react-native-nitro-sqlite

Prepared statement reuse rethrows the previous execution error

Open
#311 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
C
Stars
565
Forks
53
Avg merge
1d 21h
Merged PRs (30d)
18

Description

Summary

The prepared-statement implementation in #294 reports a statement execution error twice: once from sqlite3_step(), then again on the next call when sqlite3_reset() returns the previous evaluation's error code.

Consequently, after a constraint failure, the next invocation can be rejected without executing its new parameters. A third invocation can then succeed because the prior sqlite3_reset() did reset the virtual machine despite returning the old error.

This was found while reviewing #294 at 4fe1d1bc4310570bb9c0a6652ee2b817c50948e0 against current main at ad8b835ba0f44a207649ecc2953820d39e4e8639 (v9.7.0). The public prepared-statement API exists only in the draft PR at the time of filing.

Code evidence

  • Statement execution throws immediately when sqlite3_step() returns an error: operations.cpp#L190-L203.
  • The next SQLitePreparedStatement::execute() calls sqlite3_reset() and treats any non-SQLITE_OK result as a new failure: operations.cpp#L304-L330.

SQLite documents that the return code from sqlite3_reset(S) indicates whether the previous evaluation completed successfully. If the most recent sqlite3_step(S) failed, reset returns that error code even though it resets the statement back to its initial state: https://www.sqlite.org/c3ref/reset.html

Reproduction against #294

const db = open({ name: 'prepared-reset.sqlite' })
db.execute('CREATE TABLE users (id INTEGER PRIMARY KEY)')

const insert = db.prepare('INSERT INTO users(id) VALUES (?)')

insert.execute([1])

// Correctly reports the UNIQUE/PRIMARY KEY constraint failure.
expect(() => insert.execute([1])).toThrow(/constraint/i)

// Expected: succeeds and inserts id=2.
// Current PR behavior: throws the previous constraint error without executing.
expect(insert.execute([2]).rowsAffected).toBe(1)

expect(
  db.execute<{ id: number }>('SELECT id FROM users ORDER BY id').rows._array,
).toEqual([{ id: 1 }, { id: 2 }])

insert.finalize()

Calling insert.execute([2]) a second time after the unexpected failure can succeed, demonstrating that the failed reset call already reset the VM and that the extra rejection belonged to the previous evaluation.

The same behavior applies to executeAsync() because both paths call the same native SQLitePreparedStatement::execute() implementation.

Impact

  • A long-lived prepared insert/update can appear poisoned for one additional invocation after any constraint, busy, schema, or other step error.
  • Applications may retry a write that never ran, producing confusing retry accounting and unnecessary user-visible failures.
  • ORMs and ingestion pipelines cannot safely reuse a prepared statement after handling a normal row-level constraint failure.

Proposed direction

Own reset/clear cleanup in the same execution attempt that stepped the statement. On both success and failure, leave the statement ready for the next call before returning or throwing.

The original step/execution error must remain the primary error for that invocation. If reset/cleanup reveals a distinct finalization error, preserve both without deferring the previous evaluation's status to the next call.

This lifecycle should be implemented together with the per-connection execution contract discussed in #303 and #304, so reset, rebinding, stepping, finalization, and close cannot race. Parameter binding and cleanup should continue to share the validation tracked in #309.

Acceptance criteria

  • A constraint failure is surfaced exactly once.
  • The very next call with valid parameters executes successfully.
  • Sync and async prepared execution have identical recovery behavior.
  • The statement is reset and bindings are cleared after successful execution and after execution failure.
  • The original execution error remains primary if cleanup also fails; a cleanup failure is not silently moved to the next invocation.
  • Harness tests cover successful reuse after SQLITE_CONSTRAINT for both execute() and executeAsync().
  • Tests verify the valid retry actually changed the database, rather than only checking that it did not throw.

Related

  • #294
  • #303
  • #304
  • #309

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with SQLitePreparedStatement::execute() in packages/react-native-nitro-sqlite/cpp/operations.cpp, especially the sqlite3_reset() handling at operations.cpp#L304-L330, then review the shared sync and async paths. Run the provided prepared-insert reproduction and add harness coverage for constraint failure followed by a valid execute() and executeAsync() retry. Done means each retry executes once, updates the database, and preserves the original execution error if cleanup also fails.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, react-native, sqlite
Domain
database, mobile
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.