Conversation
|
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 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 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
In the existing 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. |
|
@ospfranco Could you please cancel the stalled CI run and rerun only the cancelled The I attempted cancellation through GitHub CLI, but GitHub returned HTTP 403 ( |
|
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. |
A deferred foreign-key violation can let every statement in a transaction succeed and then make SQLite reject
COMMIT. Currentlytransaction()resolves anyway because the automaticcommit()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:
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_executedrops errors returned bylibsql_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:
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.