fix: a nested transaction does not commit the outer one

src/Infrastructure/Database/Database.php

@@ -79,6 +79,11 @@ final class Database implements DatabaseInterface
 {
     private ?int $lastId = null;
 
+    /**
+     * How many nested scopes have asked for a transaction. Only the outermost may commit.
+     */
+    private int $transactionDepth = 0;
+
     /**
      * DB constructor.
      *
@@ -388,21 +393,41 @@ public function beginTransaction(): bool
     {
         $conn = $this->dbStorageHandler->getConnection();
 
-        if (!$conn->inTransaction()) {
-            $result = $conn->beginTransaction();
+        // The counter is this class's own bookkeeping; `inTransaction()` is the truth. Both are
+        // consulted, so a transaction opened outside these methods is still joined rather than
+        // begun again — PDO throws on that.
+        if ($this->transactionDepth > 0 || $conn->inTransaction()) {
+            // Joining the transaction already running, not starting one. Counted so that the
+            // matching `endTransaction()` knows it is not the one that may commit.
+            //
+            // It used to return `true` here with nothing recorded, and `endTransaction()` committed
+            // whenever a transaction was active — so the *inner* scope ended the *outer* one.
+            // Measured against the server: the inner commit leaves `inTransaction()` false, the
+            // outer rollback then fails with "There is no active transaction", and both rows
+            // survive. Nesting is the normal case, not an edge: `Import::doImport()` wraps the
+            // whole import and calls `Account::create()` per row, which opens its own; and
+            // `Account::update()` calls `AccountItems::updateItems()`, which opens up to five in
+            // sequence.
+            $this->transactionDepth++;
+
+            logger('beginTransaction: joining the transaction already open');
+
+            return true;
+        }
 
-            $this->eventDispatcher->notify(new Event(
-                'database.transaction.begin',
-                $this,
-                EventMessage::build()->addExtra('result', $result)
-            ));
+        $result = $conn->beginTransaction();
 
-            return $result;
+        if ($result) {
+            $this->transactionDepth = 1;
         }
 
-        logger('beginTransaction: already in transaction');
+        $this->eventDispatcher->notify(new Event(
+            'database.transaction.begin',
+            $this,
+            EventMessage::build()->addExtra('result', $result)
+        ));
 
-        return true;
+        return $result;
     }
 
     /**
@@ -414,8 +439,18 @@ public function endTransaction(): bool
     {
         $conn = $this->dbStorageHandler->getConnection();
 
+        if ($this->transactionDepth > 1) {
+            // An inner scope finishing. Its work stays in the open transaction, to be committed or
+            // rolled back with everything else by whoever opened it.
+            $this->transactionDepth--;
+
+            return true;
+        }
+
         $result = $conn->inTransaction() && $conn->commit();
 
+        $this->transactionDepth = 0;
+
         $this->eventDispatcher->notify(new Event(
             'database.transaction.end',
             $this,
@@ -436,6 +471,11 @@ public function rollbackTransaction(): bool
 
         $result = $conn->inTransaction() && $conn->rollBack();
 
+        // All of it, from whichever depth: a rollback is not something an inner scope can do on its
+        // own half of the work. Zeroing the depth also makes the outer scope's own rollback — which
+        // runs when the exception reaches it — a no-op rather than an error.
+        $this->transactionDepth = 0;
+
         $this->eventDispatcher->notify(new Event(
             'database.transaction.rollback',
             $this,

tests/Unit/Infrastructure/Database/DatabaseTest.php

@@ -65,6 +65,95 @@ public static function bufferedDataProvider(): array
         ];
     }
 
+    /**
+     * A nested scope's `endTransaction()` does not commit the transaction it joined.
+     *
+     * `beginTransaction()` answered `true` whether it started a transaction or joined one, and
+     * `endTransaction()` committed whenever one was active — so the *inner* scope ended the *outer*
+     * one. Measured against the server before the fix: the inner commit leaves `inTransaction()`
+     * false, the outer rollback then fails with "There is no active transaction", and every row
+     * written before the inner commit survives.
+     *
+     * Nesting is the ordinary case rather than an edge. `Import::doImport()` wraps the whole import
+     * in one transaction and calls `Account::create()` per row, which opens its own; and
+     * `Account::update()` calls `AccountItems::updateItems()`, which opens up to five in sequence.
+     * So an import or a bulk edit that failed part-way kept everything already processed.
+     */
+    public function testANestedTransactionDoesNotCommitTheOuterOne()
+    {
+        $pdo = $this->createMock(PDO::class);
+
+        $this->dbStorageHandler->method('getConnection')->willReturn($pdo);
+
+        // The connection's own state, so the second `beginTransaction()` is a genuine nested call
+        // rather than one the stub has told there is nothing open. By reference: an arrow function
+        // would capture it by value and answer `false` forever.
+        $open = false;
+        $pdo->method('inTransaction')->willReturnCallback(function () use (&$open): bool {
+            return $open;
+        });
+
+        // One real begin, and one real commit — not two of either.
+        $pdo->expects($this->once())
+            ->method('beginTransaction')
+            ->willReturnCallback(function () use (&$open): bool {
+                $open = true;
+
+                return true;
+            });
+
+        $pdo->expects($this->once())
+            ->method('commit')
+            ->willReturnCallback(function () use (&$open): bool {
+                $open = false;
+
+                return true;
+            });
+
+        self::assertTrue($this->database->beginTransaction(), 'the outer scope starts one');
+        self::assertTrue($this->database->beginTransaction(), 'the inner scope joins it');
+
+        self::assertTrue($this->database->endTransaction(), 'the inner scope finishing commits nothing');
+        self::assertTrue($this->database->endTransaction(), 'the outer scope is the one that commits');
+    }
+
+    /**
+     * And a rollback from any depth undoes all of it, leaving nothing for the outer scope to roll
+     * back a second time.
+     *
+     * `transactionAware()` rolls back in its own `catch`, so when an inner failure propagates the
+     * outer scope calls `rollbackTransaction()` again — that must be a no-op rather than an error.
+     */
+    public function testARollbackFromANestedScopeUndoesAllOfIt()
+    {
+        $pdo = $this->createMock(PDO::class);
+
+        $this->dbStorageHandler->method('getConnection')->willReturn($pdo);
+
+        // True while the transaction is open, false once it has been rolled back.
+        $open = true;
+        // By reference, not `fn()` — an arrow function captures by value when it is created, so a
+        // `fn(): bool => $open` here would answer `true` forever and rollBack() would run twice.
+        $pdo->method('inTransaction')->willReturnCallback(function () use (&$open): bool {
+            return $open;
+        });
+        $pdo->method('beginTransaction')->willReturn(true);
+
+        $pdo->expects($this->once())
+            ->method('rollBack')
+            ->willReturnCallback(static function () use (&$open): bool {
+                $open = false;
+
+                return true;
+            });
+
+        $this->database->beginTransaction();
+        $this->database->beginTransaction();
+
+        self::assertTrue($this->database->rollbackTransaction(), 'the inner failure rolls all of it back');
+        self::assertFalse($this->database->rollbackTransaction(), 'and the outer scope finds nothing left');
+    }
+
     /**
      * @throws Exception
      */