Fix Transaction cleanup after rollback

CHANGELOG.md

@@ -20,6 +20,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Remove the deprecated `Savepoint::rollback()` alias; use `rollbackTo()` instead (#585)
 
+### Fixed
+
+- Track manually rolled-back transactions as finished and report destructor rollback failures through assertions (#586)
+
 ## [3.4.0] - 2026-09-21
 
 ### Added

include/SQLiteCpp/Transaction.h

@@ -76,7 +76,7 @@ class SQLITECPP_API Transaction
     Transaction& operator=(const Transaction&) = delete;
 
     /**
-     * @brief Safely rollback the transaction if it has not been committed.
+     * @brief Safely rollback the transaction if it has not already been committed or rolled back.
      */
     ~Transaction();
 
@@ -92,7 +92,7 @@ class SQLITECPP_API Transaction
 
 private:
     Database&   mDatabase;              ///< Reference to the SQLite Database Connection
-    bool        mbCommited = false;     ///< True when commit has been called
+    bool        mbFinished = false;     ///< True when the transaction has been committed or rolled back
 };
 
 }  // namespace SQLite

src/Transaction.cpp

@@ -46,46 +46,42 @@ Transaction::Transaction(Database &aDatabase) :
     mDatabase.exec("BEGIN TRANSACTION");
 }
 
-// Safely rollback the transaction if it has not been committed.
+// Safely rollback the transaction if it has not already been committed or rolled back.
 Transaction::~Transaction()
 {
-    if (false == mbCommited)
+    if (!mbFinished)
     {
-        try
-        {
-            mDatabase.exec("ROLLBACK TRANSACTION");
-        }
-        catch (...)
-        {
-            // Never throw an exception in a destructor: error if already rollbacked, but no harm is caused by this.
-        }
+        const int ret = mDatabase.tryExec("ROLLBACK TRANSACTION");
+        (void)ret;  // Avoid an unused-variable warning when assertions are disabled.
+        SQLITECPP_ASSERT(SQLITE_OK == ret, mDatabase.getErrorMsg());
     }
 }
 
 // Commit the transaction.
 void Transaction::commit()
 {
-    if (false == mbCommited)
+    if (!mbFinished)
     {
         mDatabase.exec("COMMIT TRANSACTION");
-        mbCommited = true;
+        mbFinished = true;
     }
     else
     {
-        throw SQLite::Exception("Transaction already committed.");
+        throw SQLite::Exception("Transaction already finished.");
     }
 }
 
 // Rollback the transaction
 void Transaction::rollback()
 {
-    if (false == mbCommited)
+    if (!mbFinished)
     {
         mDatabase.exec("ROLLBACK TRANSACTION");
+        mbFinished = true;
     }
     else
     {
-        throw SQLite::Exception("Transaction already committed.");
+        throw SQLite::Exception("Transaction already finished.");
     }
 }
 

tests/Transaction_test.cpp

@@ -38,10 +38,10 @@ TEST(Transaction, commitRollback)
         // Commit transaction
         transaction.commit();
 
-        // Commit again throw an exception
+        // Committing an already finished transaction throws.
         EXPECT_THROW(transaction.commit(), SQLite::Exception);
 
-        // Rollback after commit also throws an exception
+        // Rolling back an already finished transaction also throws.
         EXPECT_THROW(transaction.rollback(), SQLite::Exception);
     }
 
@@ -59,29 +59,29 @@ TEST(Transaction, commitRollback)
         EXPECT_THROW(SQLite::Transaction(db, static_cast<SQLite::TransactionBehavior>(-1)), SQLite::Exception);
     }
 
-    // Auto rollback if no commit() before the end of scope
+    // Automatic rollback if commit() is not called before the end of scope.
     {
         // Begin transaction
         SQLite::Transaction transaction(db);
 
-        // Insert a second value (that will be rollbacked)
+        // Insert a second value (that will be rolled back)
         EXPECT_EQ(1, db.exec("INSERT INTO test VALUES (NULL, 'third')"));
         EXPECT_EQ(2, db.getLastInsertRowid());
 
         // end of scope: automatic rollback
     }
 
-    // Auto rollback of a transaction on error/exception
+    // Automatic rollback when leaving scope because of an exception.
     try
     {
         // Begin transaction
         SQLite::Transaction transaction(db);
 
-        // Insert a second value (that will be rollbacked)
+        // Insert a second value (that will be rolled back)
         EXPECT_EQ(1, db.exec("INSERT INTO test VALUES (NULL, 'second')"));
         EXPECT_EQ(2, db.getLastInsertRowid());
 
-        // Execute with an error => exception with auto-rollback
+        // Trigger an exception; stack unwinding destroys the transaction and rolls it back.
         db.exec("DesiredSyntaxError to raise an exception to rollback the transaction");
 
         GTEST_FATAL_FAILURE_("we should never get there");
@@ -93,22 +93,19 @@ TEST(Transaction, commitRollback)
         // expected error, see above
     }
 
-    // Double rollback with a manual command before the end of scope
+    // Manual rollback before the end of scope
     {
-        // Begin transaction
         SQLite::Transaction transaction(db);
 
-        // Insert a second value (that will be rollbacked)
+        // Insert a second value that will be rolled back.
         EXPECT_EQ(1, db.exec("INSERT INTO test VALUES (NULL, 'third')"));
         EXPECT_EQ(2, db.getLastInsertRowid());
 
-        // Execute a manual rollback
+        // A manual rollback finishes the transaction; the destructor has nothing left to do.
         transaction.rollback();
-
-        // end of scope: the automatic rollback should not raise an error because it is harmless
     }
 
-    // Check the results (expect only one row of result, as all other one have been rollbacked)
+    // Only the explicitly committed first row should remain; all later rows were rolled back.
     SQLite::Statement query(db, "SELECT * FROM test");
     int nbRows = 0;
     while (query.executeStep())
@@ -119,3 +116,27 @@ TEST(Transaction, commitRollback)
     }
     EXPECT_EQ(1, nbRows);
 }
+
+TEST(Transaction, manualRollbackFinishesTransaction)
+{
+    SQLite::Database db(":memory:", SQLite::OPEN_READWRITE | SQLite::OPEN_CREATE);
+    db.exec("CREATE TABLE test (id INTEGER PRIMARY KEY, value TEXT)");
+
+    {
+        SQLite::Transaction transaction(db);
+        transaction.rollback();
+
+        // The Transaction object is finished after rollback(). A new transaction
+        // on the same connection must therefore be left untouched by its destructor.
+        db.exec("BEGIN TRANSACTION");
+        EXPECT_EQ(1, db.exec("INSERT INTO test VALUES (NULL, 'kept')"));
+    }
+
+    // If the finished Transaction destructor issued another ROLLBACK, this COMMIT
+    // would fail and the inserted row would be lost.
+    EXPECT_NO_THROW(db.exec("COMMIT TRANSACTION"));
+
+    SQLite::Statement query(db, "SELECT value FROM test");
+    ASSERT_TRUE(query.executeStep());
+    EXPECT_STREQ("kept", query.getColumn(0).getText());
+}