Skip to content

fix: await transaction commit and preserve finalized handles - #455

Merged
ospfranco merged 2 commits into
OP-Engineering:mainfrom
JaredSystems:fix/transaction-commit-finalization-454
Sep 15, 2026
Merged

ospfranco merged 2 commits into
OP-Engineering:mainfrom
JaredSystems:fix/transaction-commit-finalization-454

Conversation

@JaredSystems

@JaredSystems JaredSystems commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

A deferred foreign-key violation can let every statement in a transaction succeed and then make SQLite reject COMMIT. Currently transaction() resolves anyway because the automatic commit() promise is not awaited. The failed commit becomes an unhandled rejection, rollback is skipped, and the connection can remain inside the failed transaction.

Fixes #454 for backends that propagate native COMMIT errors.

This change:

  • Awaits automatic commit so commit failures reach the existing rollback/error path and queued work waits for reactive flushing.
  • Marks the transaction finalized immediately after SQL COMMIT succeeds, before awaiting reactive flushing. A subsequent notification error must not trigger ROLLBACK against already committed data and mask the original error.
  • Keeps completed transaction handles finalized, preventing a retained handle from writing to or finalizing a later transaction.

The example test suite now covers automatic COMMIT error propagation and queued recovery on every backend using a fault injected at the synchronous execution boundary; a real deferred-FK COMMIT failure on standard SQLite, SQLCipher, and embedded SQLite; retained handles after automatic commit, explicit commit, and explicit rollback; and a callback error after explicit commit. Retained handles are checked for execute, commit, and rollback while another transaction is active.

Known libSQL limitation: the initial CI run exposed an existing native error-propagation defect. opsqlite_libsql_execute drops errors returned by libsql_next_row, including deferred-FK COMMIT errors. The JS wrapper cannot catch an error native code does not expose. The native FK test therefore excludes libSQL and Turso; the injected-COMMIT and handle-lifetime tests run on all backends. See the CI investigation and direct native reproduction. A native libSQL fix is outside this PR.

Validation:

  • Initial upstream CI: 8/10 jobs passed, including standard Android/iOS, SQLCipher, embedded SQLite, Turso, and typechecking. Both libSQL jobs failed only on the native FK regression described above.
  • Revised tests: 5/5 passed on Android/libSQL in an isolated Android 15 x86_64 emulator, RN 0.79.6/Hermes, bundled SQLite 3.45.1. Native instrumentation only logged the discarded error; it did not change error handling.
  • Revised host tests: 6/6 passed through op-test 0.2.7 using the actual TS wrapper and file-backed Node SQLite. The original wrapper failed 4/6, including the injected-COMMIT test.
  • The unchanged production patch previously passed the broader 15/15 Android qualification on a OnePlus 7T, Android 12, arm64-v8a, RN 0.79.6/Hermes, legacy architecture, vanilla OP-SQLite 18.2.1 / SQLite 3.51.3. Original: 7/15; await-only: 12/15. The initial five example regressions also passed on that device. Details are in [Bug]: transaction() resolves after failed automatic COMMIT (18.2.1, Android) #454.
  • Scoped TypeScript 5.9.2 check using RN 0.87.0 and op-test 0.2.7 declarations, plus git diff --check, passed. The revised upstream CI run awaits maintainer approval.

A reactive-flush failure after successful SQL COMMIT still rejects with the notification error while the writes remain committed. This PR does not introduce a new error API or change that outcome into success. BEGIN/ROLLBACK failure handling, connection quarantine, executeBatch, and the native transaction-lock redesign in #416 are outside this fix.

@JaredSystems

Copy link
Copy Markdown
Contributor Author

I investigated the failed CI run and pushed 924f1b2 with a test-only correction. The production transaction patch is unchanged.

The initial CI run passed 8/10 jobs. Only android-libsql and ios-libsql failed, both on the real deferred-FK COMMIT regression. All four other new transaction tests passed in both jobs. Standard SQLite, SQLCipher, embedded SQLite, Turso, and typechecking passed.

I reproduced the libSQL failure in an isolated Android 15 x86_64 emulator using RN 0.79.6/Hermes and the bundled libSQL backend (SQLite 3.45.1). It also occurs through direct executeSync, without calling transaction():

PRAGMA foreign_keys = ON;
CREATE TABLE parent(id INTEGER PRIMARY KEY);
CREATE TABLE child(parent_id INTEGER REFERENCES parent(id) DEFERRABLE INITIALLY DEFERRED);
BEGIN;
INSERT INTO child VALUES(999);
COMMIT;
SELECT COUNT(*) FROM child; -- 1, still inside the failed transaction
ROLLBACK;
SELECT COUNT(*) FROM child; -- 0

executeSync('COMMIT') returns an ordinary result. Adding diagnostic logging only to the existing native bridge showed:

OP_LIBSQL_STEP_ERROR query=COMMIT status=1 error=Error fetching next row: SQLite failure: `FOREIGN KEY constraint failed`

In the existing opsqlite_libsql_execute, the libsql_next_row error ends the loop, but the final status is not checked before returning a successful result. Awaiting the JS commit cannot recover an error that native code never exposes. This path is unchanged by this PR.

The test update keeps the real deferred-FK regression enabled for standard SQLite, SQLCipher, and embedded SQLite, and excludes libSQL alongside the existing Turso exclusion with an explicit explanation. It also adds an automatic-COMMIT fault-injection test on every backend, asserting original error identity, exactly one rollback, rolled-back data, and recovery of an already queued transaction. This tests the JS fix independently of native error reporting; the remaining SQL runs on the real backend.

Validation of the revised tests: 5/5 passed on Android/libSQL, 6/6 passed on host SQLite, and the original unpatched wrapper failed 4/6 host cases, including the new injected-COMMIT test. Scoped TypeScript and whitespace checks passed. The libSQL diagnostic build changed only logging in native code; no native fix is included here.

This PR does not resolve libSQL's native error-propagation defect. It needs a separate native fix and qualification; the direct reproduction above should make that follow-up easier to investigate.

@JaredSystems

Copy link
Copy Markdown
Contributor Author

@ospfranco Could you please cancel the stalled CI run and rerun only the cancelled ios-embedded job? The other nine jobs have passed.

The ios-embedded log shows Corepack failing to download Yarn with UND_ERR_SOCKET. scripts/test-ios.sh continues after yarn run:ios:release fails and waits indefinitely for a test-result marker. The previous embedded job completed in about 13 minutes; this one has been stuck for over an hour.

I attempted cancellation through GitHub CLI, but GitHub returned HTTP 403 (Must have admin rights to Repository), so I cannot restart it from this account. Thank you.

@ospfranco

Copy link
Copy Markdown
Contributor

took a more detailed look and the fix is both correct and not correct, the idea behind not awaiting for commit was not to await for the reactive queries flushes as that can take a long time on every commit extending the resolution of the transaction with stuff that is not related to it. I have to take another look, but indeed there is an issue. Thanks for the PR.

@JaredSystems

Copy link
Copy Markdown
Contributor Author

took a more detailed look and the fix is both correct and not correct, the idea behind not awaiting for commit was not to await for the reactive queries flushes as that can take a long time on every commit extending the resolution of the transaction with stuff that is not related to it. I have to take another look, but indeed there is an issue. Thanks for the PR.

Awesome, thanks for taking a look at it. I just started tinkering with op-sqlite, looking for an alternative for an older library i'm using. Once a fix is in for this i'd love to start contributing more and also possibly sponsoring. Thank you for your time and for working on this.

@ospfranco
ospfranco merged commit 924f1b2 into OP-Engineering:main Sep 15, 2026
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: transaction() resolves after failed automatic COMMIT (18.2.1, Android)

2 participants