Transaction cleanup no longer masks transaction-destroying errors

A RAISE(ROLLBACK) trigger or INSERT OR ROLLBACK conflict rolls back the
entire transaction and destroys every savepoint. The cleanup paths in
atomic() and query() then raised OperationalError ("no such savepoint" /
"cannot rollback - no transaction is active"), masking the original
IntegrityError - breaking user code that catches sqlite3.IntegrityError.
Cleanup now checks conn.in_transaction first: if the error already
destroyed the transaction there is nothing left to undo, and the
original exception propagates.

Refs https://github.com/simonw/sqlite-utils/issues/769#issuecomment-4900034150

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

docs/changelog.rst

@@ -19,6 +19,7 @@ Unreleased
 - Fixed a bug where a failed write statement executed with ``db.execute()`` left the driver's implicit transaction open. Every subsequent write then joined that phantom transaction, which nothing committed, so their work was silently rolled back when the connection was closed. The implicit transaction opened by a failed statement is now rolled back before the exception is raised. A failed write inside a transaction opened with ``db.begin()`` or ``db.atomic()`` leaves that transaction open and untouched, as before.
 - Fixed a bug where transaction-control statements prefixed with an empty statement - ``db.query("; COMMIT")`` - or a UTF-8 byte order mark slipped past the check that rejects them, committing the caller's open transaction before raising a confusing ``OperationalError``. The keyword scanner used by ``db.query()`` and ``db.execute()`` now skips leading ``;`` and byte order marks, matching what the ``sqlite3`` driver tolerates before the first token, so these statements are rejected with a ``ValueError`` without being executed. The same fix means ``db.execute("; BEGIN")`` no longer auto-commits the transaction it just opened.
 - Documented a limitation of ``db.query()``: a ``PRAGMA`` statement that returns no rows raises a ``ValueError`` but still takes effect, because PRAGMA statements run outside the savepoint guard used to roll back other rejected statements. Use ``db.execute()`` for row-less PRAGMA statements.
+- Fixed exception masking when a statement destroys the enclosing transaction. An error such as a ``RAISE(ROLLBACK)`` trigger or ``INSERT OR ROLLBACK`` conflict rolls back the whole transaction, destroying every savepoint - the cleanup in ``db.atomic()`` and ``db.query()`` then failed with ``OperationalError: no such savepoint`` (or ``cannot rollback - no transaction is active``), hiding the original ``IntegrityError`` from code that tried to catch it. Cleanup now checks whether a transaction is still open first, so the original exception propagates.
 - ``sqlite-utils migrate --list`` is now read-only even when the migrations file uses the legacy ``sqlite_migrate.Migrations`` class, whose listing methods create the ``_sqlite_migrations`` table as a side effect. The listing now runs inside a transaction that is rolled back.
 - ``sqlite-utils insert ... --pk <missing column>`` and ``sqlite-utils extract <missing column>`` now show a clean ``Error:`` message instead of a raw Python traceback. The ``extract`` command also shows a clean error when pointed at a view.
 - Fixed a bug where running ``table.extract()`` more than once against the same lookup table inserted duplicate rows for values containing ``null`` - SQLite unique indexes treat ``NULL`` values as distinct, so ``INSERT OR IGNORE`` alone could not dedupe them. Each repeat extract added another copy that nothing referenced. The insert now uses an ``IS``-based ``NOT EXISTS`` guard so ``null``-containing rows match existing lookup rows.

sqlite_utils/db.py

@@ -602,8 +602,13 @@ class Database:
             try:
                 yield self
             except BaseException:
-                self.conn.execute("ROLLBACK TO SAVEPOINT {};".format(savepoint))
-                self.conn.execute("RELEASE SAVEPOINT {};".format(savepoint))
+                # An error such as a RAISE(ROLLBACK) trigger can destroy
+                # the whole transaction, savepoints included - cleaning up
+                # anyway would mask the original exception with
+                # "no such savepoint"
+                if self.conn.in_transaction:
+                    self.conn.execute("ROLLBACK TO SAVEPOINT {};".format(savepoint))
+                    self.conn.execute("RELEASE SAVEPOINT {};".format(savepoint))
                 raise
             else:
                 self.conn.execute("RELEASE SAVEPOINT {};".format(savepoint))
@@ -612,13 +617,15 @@ class Database:
             try:
                 yield self
             except BaseException:
-                self.conn.execute("ROLLBACK")
+                # rollback() is a no-op if the error already destroyed the
+                # transaction, so the original exception propagates
+                self.rollback()
                 raise
             else:
                 try:
                     self.conn.execute("COMMIT")
                 except BaseException:
-                    self.conn.execute("ROLLBACK")
+                    self.rollback()
                     raise
 
     def begin(self) -> None:
@@ -870,8 +877,11 @@ class Database:
                 return (dict(zip(keys, row)) for row in fetched)
             return (dict(zip(keys, row)) for row in cursor)
         finally:
-            if not released:
-                # An error occurred - undo anything the statement changed
+            if not released and self.conn.in_transaction:
+                # An error occurred - undo anything the statement changed.
+                # If the error itself destroyed the transaction (such as a
+                # RAISE(ROLLBACK) trigger) the savepoint is already gone
+                # and there is nothing left to undo
                 self.conn.execute('ROLLBACK TO "sqlite_utils_query"')
                 self.conn.execute('RELEASE "sqlite_utils_query"')
 

tests/test_query.py

@@ -280,3 +280,20 @@ def test_execute_returning_dicts(fresh_db):
     assert fresh_db.execute_returning_dicts("select * from test") == [
         {"id": 1, "bar": 2}
     ]
+
+
+def test_query_preserves_error_from_transaction_destroying_trigger(fresh_db):
+    # RAISE(ROLLBACK) destroys the savepoint guard - the original
+    # IntegrityError must propagate, not "no such savepoint"
+    fresh_db.execute("create table t (id integer primary key, v text)")
+    fresh_db.execute("""
+        create trigger no_bad before insert on t
+        when new.v = 'bad'
+        begin
+            select raise(rollback, 'trigger says no');
+        end
+    """)
+    with pytest.raises(sqlite3.IntegrityError, match="trigger says no"):
+        fresh_db.query("insert into t (id, v) values (1, 'bad') returning id")
+    assert not fresh_db.conn.in_transaction
+    assert fresh_db.execute("select count(*) from t").fetchone()[0] == 0