From d88de2ce36c2c475f0866d169e9486bfd5ca1d68 Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Mon, 20 Apr 2026 15:31:18 -0300
Subject: [PATCH 01/13] fix(transactions): prevent nested transaction from
 destroying outer EntityManager on connection error

---
 .../Utils/DoctrineTransactionService.php      | 199 +++--
 .../DoctrineTransactionServiceTest.php        | 844 ++++++++++++++++++
 2 files changed, 975 insertions(+), 68 deletions(-)
 create mode 100644 tests/Unit/Services/DoctrineTransactionServiceTest.php

diff --git a/app/Services/Utils/DoctrineTransactionService.php b/app/Services/Utils/DoctrineTransactionService.php
index 0c30fb96c..6cdf41a1e 100644
--- a/app/Services/Utils/DoctrineTransactionService.php
+++ b/app/Services/Utils/DoctrineTransactionService.php
@@ -22,9 +22,29 @@
 use Exception;
 use libs\utils\ITransactionService;
 use ErrorException;
+
 /**
  * Class DoctrineTransactionService
  * @package App\Services\Utils
+ *
+ * Root-aware transaction wrapper.
+ *
+ * Root (outermost) transaction:
+ *  - owns the connection lifecycle (may retry on transient errors)
+ *  - sets isolation level
+ *  - flushes the EntityManager and issues the real COMMIT
+ *
+ * Nested transaction (called while a DB transaction is already active):
+ *  - uses Doctrine DBAL nesting counter (beginTransaction / commit)
+ *  - flushes the EntityManager so auto-generated IDs are available
+ *  - does NOT retry, reset the EntityManager, or close the connection
+ *  - on error, rolls back only the nested level and re-throws,
+ *    letting the root transaction decide how to handle the failure
+ *
+ * Savepoints are enabled (setNestTransactionsWithSavepoints) so that
+ * nested rollBack() issues ROLLBACK TO SAVEPOINT — undoing only the
+ * inner work without poisoning the outer transaction. This allows the
+ * "catch inner error and continue" pattern to work correctly.
  */
 final class DoctrineTransactionService implements ITransactionService
 {
@@ -61,47 +81,15 @@ public function shouldReconnect(\Exception $e):bool
             )
         );
         if($e instanceof ErrorException && str_contains($e->getMessage(), "Packets out of order")){
-            Log::debug
-            (
-                sprintf
-                (
-                    "DoctrineTransactionService::shouldReconnect %s Packets out of order true",
-                    get_class($e),
-                )
-            );
             return true;
         }
         if($e instanceof RetryableException) {
-            Log::debug
-            (
-                sprintf
-                (
-                    "DoctrineTransactionService::shouldReconnect %s true",
-                    get_class($e),
-                )
-            );
             return true;
         }
         if($e instanceof ConnectionLost) {
-            Log::debug
-            (
-                sprintf
-                (
-                    "DoctrineTransactionService::shouldReconnect %s true",
-                    get_class($e),
-                )
-            );
             return true;
         }
         if($e instanceof ConnectionException) {
-            Log::debug
-            (
-                sprintf
-                (
-                    "DoctrineTransactionService::shouldReconnect %s true",
-                    get_class($e),
-                )
-            );
             return true;
         }
         if($e instanceof \PDOException){
@@ -131,58 +119,133 @@ public function shouldReconnect(\Exception $e):bool
      * @return mixed|null
      * @throws Exception
      */
-    public function transaction(Closure $callback,  int $isolationLevel = TransactionIsolationLevel::READ_COMMITTED)
+    public function transaction(Closure $callback, int $isolationLevel = TransactionIsolationLevel::READ_COMMITTED)
+    {
+        $em   = Registry::getManager($this->manager_name);
+        $conn = $em->getConnection();
+
+        // Detect whether we are already inside a DB transaction.
+        $isNested = $conn->isTransactionActive();
+
+        if ($isNested) {
+            return $this->runNestedTransaction($callback);
+        }
+
+        return $this->runRootTransaction($callback, $isolationLevel);
+    }
+
+    /**
+     * Root (outermost) transaction: may retry on transient connection errors,
+     * sets isolation level, flushes, and issues the real COMMIT.
+     *
+     * @param Closure $callback
+     * @param int $isolationLevel
+     * @return mixed|null
+     * @throws Exception
+     */
+    private function runRootTransaction(Closure $callback, int $isolationLevel)
     {
-        $retry  = 0;
-        $done   = false;
-        $result = null;
+        $retry = 0;
+
+        while ($retry < self::MaxRetries) {
+            $em = Registry::getManager($this->manager_name);
 
-        while (!$done and $retry < self::MaxRetries) {
             try {
-                $em  = Registry::getManager($this->manager_name);
                 if (!$em->isOpen()) {
-                    Log::warning("DoctrineTransactionService::transaction: entity manager is closed!, trying to re open...");
+                    Log::warning("DoctrineTransactionService::runRootTransaction: entity manager is closed, resetting...");
                     $em = Registry::resetManager($this->manager_name);
                 }
-                $em->getConnection()->setTransactionIsolation($isolationLevel);
-                $em->getConnection()->beginTransaction(); // suspend auto-commit
-                $result = $callback($this);
-                $em->flush();
-                $em->getConnection()->commit();
-                $done = true;
-            }
-            catch (Exception $ex) {
 
+                $conn = $em->getConnection();
+                $conn->setNestTransactionsWithSavepoints(true);
+                $conn->setTransactionIsolation($isolationLevel);
+                $conn->beginTransaction();
+
+                try {
+                    $result = $callback($this);
+                    $em->flush();
+                    $conn->commit();
+                    return $result;
+                } catch (\Throwable $inner) {
+                    if ($conn->isTransactionActive()) {
+                        $conn->rollBack();
+                    }
+                    throw $inner;
+                }
+            } catch (Exception $ex) {
                 $retry++;
-                $em->getConnection()->close();
-                $em->close();
-                if($em->getConnection()->isTransactionActive())
-                    $em->getConnection()->rollBack();
-                Registry::resetManager($this->manager_name);
-
-                if($this->shouldReconnect($ex)){
-                    Log::warning
-                    (
-                        sprintf
-                        (
-                            "DoctrineTransactionService::transaction should reconnect %s retry %s",
-                            $ex->getMessage(),
-                            $retry
-                        )
-                    );
-                    if ($retry === self::MaxRetries) {
-                        Log::warning(sprintf("DoctrineTransactionService::transaction Max Retry Reached %s", $retry));
+
+                if ($this->shouldReconnect($ex)) {
+                    Log::warning(sprintf(
+                        "DoctrineTransactionService::runRootTransaction reconnectable error '%s', retry %d/%d",
+                        $ex->getMessage(),
+                        $retry,
+                        self::MaxRetries
+                    ));
+
+                    // Root only: destructive recovery is safe here because
+                    // no outer transaction holds references to this EM/connection.
+                    try {
+                        if ($em->isOpen()) {
+                            $em->clear();
+                            $em->close();
+                        }
+                    } catch (\Throwable $ignore) {
+                    }
+
+                    try {
+                        $em->getConnection()->close();
+                    } catch (\Throwable $ignore) {
+                    }
+
+                    Registry::resetManager($this->manager_name);
+
+                    if ($retry >= self::MaxRetries) {
+                        Log::warning(sprintf("DoctrineTransactionService::runRootTransaction Max Retry Reached %d", $retry));
                         Log::error($ex);
                         throw $ex;
                     }
+
                     continue;
                 }
-                Log::warning("DoctrineTransactionService::transaction rolling back TX");
+
+                Log::warning("DoctrineTransactionService::runRootTransaction rolling back TX");
                 Log::warning($ex);
                 throw $ex;
             }
         }
 
-        return $result;
+        throw new \RuntimeException("DoctrineTransactionService::runRootTransaction exceeded max retries.");
+    }
+
+    /**
+     * Nested transaction: runs inside an already-active transaction.
+     * Flushes so auto-generated IDs are available to the caller,
+     * but does NOT retry, close the connection, or reset the EntityManager.
+     * On failure, rolls back the nested level and re-throws to the root.
+     *
+     * @param Closure $callback
+     * @return mixed|null
+     * @throws \Throwable
+     */
+    private function runNestedTransaction(Closure $callback)
+    {
+        $em   = Registry::getManager($this->manager_name);
+        $conn = $em->getConnection();
+
+        $conn->beginTransaction();
+
+        try {
+            $result = $callback($this);
+            $em->flush();
+            $conn->commit();
+            return $result;
+        } catch (\Throwable $ex) {
+            if ($conn->isTransactionActive()) {
+                $conn->rollBack();
+            }
+            // No resetManager, no close(), no retry — let the root handle recovery.
+            throw $ex;
+        }
     }
-}
\ No newline at end of file
+}
diff --git a/tests/Unit/Services/DoctrineTransactionServiceTest.php b/tests/Unit/Services/DoctrineTransactionServiceTest.php
new file mode 100644
index 000000000..ecd805fd3
--- /dev/null
+++ b/tests/Unit/Services/DoctrineTransactionServiceTest.php
@@ -0,0 +1,844 @@
+<?php namespace Tests\Unit\Services;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use Closure;
+use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\Exception\RetryableException;
+use Doctrine\DBAL\TransactionIsolationLevel;
+use Doctrine\ORM\EntityManagerInterface;
+use Doctrine\Persistence\ManagerRegistry;
+use Illuminate\Support\Facades\Facade;
+use LaravelDoctrine\ORM\Facades\Registry;
+use Mockery;
+use PHPUnit\Framework\TestCase;
+use services\utils\DoctrineTransactionService;
+
+/**
+ * A concrete retryable exception for testing.
+ * ConnectionLost requires a Driver\Exception argument which is hard to construct;
+ * RetryableException is a marker interface that triggers the same retry logic.
+ */
+class TestRetryableException extends \RuntimeException implements RetryableException
+{
+}
+
+/**
+ * Unit tests for DoctrineTransactionService root-vs-nested transaction behavior.
+ *
+ * Covers:
+ * - Root transaction: retry on connection errors, flush, commit, EM reset
+ * - Nested transaction: no retry, no EM reset, no connection close, flush, exception propagation
+ * - Direct nesting: inner transaction() called inside outer transaction() (e.g. email-send pattern)
+ * - Indirect nesting: service method with its own transaction() called from within another transaction()
+ * - Connection error in nested: no destructive recovery, exception propagates to root
+ */
+class DoctrineTransactionServiceTest extends TestCase
+{
+    private $container;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        Facade::clearResolvedInstances();
+        $this->container = new \Illuminate\Container\Container();
+        $this->container->instance('app', $this->container);
+        $this->container->instance('log', new class {
+            public function __call($name, $args) { /* swallow */ }
+        });
+        Facade::setFacadeApplication($this->container);
+    }
+
+    protected function tearDown(): void
+    {
+        Facade::clearResolvedInstances();
+        Facade::setFacadeApplication(null);
+        Mockery::close();
+        parent::tearDown();
+    }
+
+    // ─────────────────────────────────────────────────────────────────────────
+    // Helpers
+    // ─────────────────────────────────────────────────────────────────────────
+
+    /**
+     * Build mocked EM + Connection and register them in the facade.
+     *
+     * @param bool $transactionActive  Initial state of isTransactionActive()
+     * @return array{EntityManagerInterface&\Mockery\MockInterface, Connection&\Mockery\MockInterface}
+     */
+    private function buildMocks(bool $transactionActive = false): array
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('isTransactionActive')->andReturn($transactionActive)->byDefault();
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('beginTransaction')->byDefault();
+        $conn->shouldReceive('commit')->byDefault();
+        $conn->shouldReceive('rollBack')->byDefault();
+        $conn->shouldReceive('close')->byDefault();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->byDefault();
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault();
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        return [$em, $conn, $registry];
+    }
+
+    // ─────────────────────────────────────────────────────────────────────────
+    // ROOT TRANSACTION TESTS
+    // ─────────────────────────────────────────────────────────────────────────
+
+    public function testRootTransactionCommitsAndFlushes(): void
+    {
+        [$em, $conn] = $this->buildMocks(transactionActive: false);
+
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')
+            ->once()
+            ->with(TransactionIsolationLevel::READ_COMMITTED);
+        $conn->shouldReceive('beginTransaction')->once();
+        $em->shouldReceive('flush')->once();
+        $conn->shouldReceive('commit')->once();
+
+        $service = new DoctrineTransactionService('default');
+        $result = $service->transaction(function () {
+            return 'success';
+        });
+
+        $this->assertSame('success', $result);
+    }
+
+    public function testRootTransactionRollsBackOnNonRetryableException(): void
+    {
+        [$em, $conn] = $this->buildMocks(transactionActive: false);
+
+        $conn->shouldReceive('beginTransaction')->once();
+        $conn->shouldReceive('isTransactionActive')->andReturn(true);
+        $conn->shouldReceive('rollBack')->once();
+        $conn->shouldReceive('commit')->never();
+        $em->shouldReceive('flush')->never();
+
+        $service = new DoctrineTransactionService('default');
+
+        $this->expectException(\RuntimeException::class);
+        $this->expectExceptionMessage('business error');
+
+        $service->transaction(function () {
+            throw new \RuntimeException('business error');
+        });
+    }
+
+    public function testRootTransactionRetriesOnConnectionLost(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('beginTransaction')->byDefault();
+        $conn->shouldReceive('commit')->byDefault();
+        $conn->shouldReceive('rollBack')->byDefault();
+        $conn->shouldReceive('close')->byDefault();
+
+        // First call: isTransactionActive returns false (root detection)
+        // then returns true inside the inner catch for rollback check
+        $txActiveSequence = [false, true, false];
+        $txActiveIndex = 0;
+        $conn->shouldReceive('isTransactionActive')->andReturnUsing(
+            function () use (&$txActiveSequence, &$txActiveIndex) {
+                $val = $txActiveSequence[$txActiveIndex] ?? false;
+                $txActiveIndex++;
+                return $val;
+            }
+        );
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->byDefault();
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->andReturn($em);
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $callCount = 0;
+        $service = new DoctrineTransactionService('default');
+        $result = $service->transaction(function () use (&$callCount) {
+            $callCount++;
+            if ($callCount === 1) {
+                throw new TestRetryableException('Connection lost');
+            }
+            return 'recovered';
+        });
+
+        $this->assertSame('recovered', $result);
+        $this->assertSame(2, $callCount, 'Callback should be retried after connection lost');
+    }
+
+    public function testRootTransactionResetsManagerOnConnectionError(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('isTransactionActive')->andReturn(false, true, false);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('beginTransaction')->byDefault();
+        $conn->shouldReceive('commit')->byDefault();
+        $conn->shouldReceive('rollBack')->byDefault();
+        $conn->shouldReceive('close')->byDefault();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->byDefault();
+        $em->shouldReceive('clear')->once(); // destructive recovery
+        $em->shouldReceive('close')->once(); // destructive recovery
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($em);
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $callCount = 0;
+        $service = new DoctrineTransactionService('default');
+        $service->transaction(function () use (&$callCount) {
+            $callCount++;
+            if ($callCount === 1) {
+                throw new TestRetryableException('gone');
+            }
+            return 'ok';
+        });
+
+        $this->assertSame(2, $callCount);
+    }
+
+    // ─────────────────────────────────────────────────────────────────────────
+    // NESTED TRANSACTION TESTS
+    // ─────────────────────────────────────────────────────────────────────────
+
+    public function testNestedTransactionFlushesAndCommits(): void
+    {
+        // Connection already has an active transaction → detected as nested
+        [$em, $conn] = $this->buildMocks(transactionActive: true);
+
+        $conn->shouldReceive('beginTransaction')->once();
+        $em->shouldReceive('flush')->once();
+        $conn->shouldReceive('commit')->once();
+        // Must NOT set isolation level in nested mode
+        $conn->shouldReceive('setTransactionIsolation')->never();
+
+        $service = new DoctrineTransactionService('default');
+        $result = $service->transaction(function () {
+            return 'nested-result';
+        });
+
+        $this->assertSame('nested-result', $result);
+    }
+
+    public function testNestedTransactionDoesNotRetryOnConnectionError(): void
+    {
+        [$em, $conn] = $this->buildMocks(transactionActive: true);
+
+        $conn->shouldReceive('isTransactionActive')->andReturn(true);
+        $conn->shouldReceive('rollBack')->once();
+
+        // Must NOT reset manager or close connection in nested
+        $registry = $this->container->make(ManagerRegistry::class);
+        $registry->shouldReceive('resetManager')->never();
+        $conn->shouldReceive('close')->never();
+        $em->shouldReceive('close')->never();
+        $em->shouldReceive('clear')->never();
+
+        $callCount = 0;
+        $service = new DoctrineTransactionService('default');
+
+        try {
+            $service->transaction(function () use (&$callCount) {
+                $callCount++;
+                throw new TestRetryableException('inner connection lost');
+            });
+            $this->fail('Expected exception was not thrown');
+        } catch (TestRetryableException $e) {
+            // expected
+        }
+
+        $this->assertSame(1, $callCount, 'Nested transaction must NOT retry');
+    }
+
+    public function testNestedTransactionRollsBackAndRethrowsOnError(): void
+    {
+        [$em, $conn] = $this->buildMocks(transactionActive: true);
+
+        $conn->shouldReceive('isTransactionActive')->andReturn(true);
+        $conn->shouldReceive('rollBack')->once();
+        $conn->shouldReceive('commit')->never();
+        $em->shouldReceive('flush')->never(); // flush not reached on error
+
+        $service = new DoctrineTransactionService('default');
+
+        $this->expectException(\LogicException::class);
+        $this->expectExceptionMessage('inner failure');
+
+        $service->transaction(function () {
+            throw new \LogicException('inner failure');
+        });
+    }
+
+    // ─────────────────────────────────────────────────────────────────────────
+    // DIRECT NESTING: inner transaction() inside outer transaction()
+    // Simulates the email-send pattern (SummitPromoCodeService, etc.)
+    // ─────────────────────────────────────────────────────────────────────────
+
+    /**
+     * Simulates the pattern at SummitPromoCodeService.php:980-998,
+     * SummitRSVPInvitationService.php:448-458, etc.
+     *
+     * Outer transaction starts (root), inner transaction is called from within
+     * the outer's closure. Inner must detect the active transaction and run
+     * as nested (no retry, no EM reset).
+     */
+    public function testDirectNestingInnerRunsAsNested(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->once(); // only root sets isolation
+        $conn->shouldReceive('close')->byDefault();
+
+        // Track transaction active state: false initially (root detection),
+        // then true once beginTransaction is called (for nested detection)
+        $txActive = false;
+        $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) {
+            return $txActive;
+        });
+        $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) {
+            $txActive = true;
+        });
+        $conn->shouldReceive('commit')->andReturnUsing(function () use (&$txActive) {
+            // Only outer commit sets txActive to false
+            // In real Doctrine this decrements a counter, simulate with simple bool
+        });
+        $conn->shouldReceive('rollBack')->byDefault();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->byDefault();
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault();
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $service = new DoctrineTransactionService('default');
+
+        $innerExecuted = false;
+        $result = $service->transaction(function ($tx) use ($service, &$innerExecuted) {
+            // Simulates the inner transaction (e.g. getByIdExclusiveLock pattern)
+            $innerResult = $service->transaction(function () use (&$innerExecuted) {
+                $innerExecuted = true;
+                return 'promo_code_entity';
+            });
+
+            // Outer continues with the result (e.g. dispatches email)
+            return "dispatched:{$innerResult}";
+        });
+
+        $this->assertTrue($innerExecuted);
+        $this->assertSame('dispatched:promo_code_entity', $result);
+    }
+
+    /**
+     * Direct nesting: inner transaction throws — exception propagates to outer,
+     * outer catches and the root handles the full rollback.
+     * No EM reset or connection close happens during inner failure.
+     */
+    public function testDirectNestingInnerErrorPropagatesWithoutDestroyingEM(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('close')->never(); // inner must NOT close connection
+
+        $txActive = false;
+        $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) {
+            return $txActive;
+        });
+        $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) {
+            $txActive = true;
+        });
+        $conn->shouldReceive('commit')->byDefault();
+        $conn->shouldReceive('rollBack')->byDefault();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->byDefault();
+        $em->shouldReceive('clear')->never(); // inner must NOT clear EM
+        $em->shouldReceive('close')->never(); // inner must NOT close EM
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->never(); // inner must NOT reset
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $service = new DoctrineTransactionService('default');
+
+        $outerCaughtException = null;
+        $result = $service->transaction(function () use ($service, &$outerCaughtException) {
+            try {
+                $service->transaction(function () {
+                    throw new \RuntimeException('lock acquisition failed');
+                });
+            } catch (\RuntimeException $e) {
+                $outerCaughtException = $e;
+                // Outer catches and continues (e.g. skips email dispatch)
+                return 'skipped';
+            }
+        });
+
+        $this->assertNotNull($outerCaughtException);
+        $this->assertSame('lock acquisition failed', $outerCaughtException->getMessage());
+        $this->assertSame('skipped', $result);
+    }
+
+    /**
+     * Direct nesting with connection error in inner: inner must NOT retry
+     * and must NOT do destructive recovery. Exception propagates to root
+     * which then handles the reconnect logic.
+     *
+     * This is the exact scenario that was broken in the old code:
+     * inner connection error would close/reset EM, destroying the outer TX.
+     */
+    public function testDirectNestingConnectionErrorInInnerDoesNotDestroyOuterTx(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+
+        $txActive = false;
+        $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) {
+            return $txActive;
+        });
+        $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) {
+            $txActive = true;
+        });
+        $conn->shouldReceive('commit')->byDefault();
+        $conn->shouldReceive('rollBack')->byDefault();
+        $conn->shouldReceive('close')->byDefault();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->byDefault();
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        // resetManager will be called by the ROOT's retry, not by the nested tx
+        $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault();
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $service = new DoctrineTransactionService('default');
+
+        $outerCallCount = 0;
+        $innerCallCount = 0;
+
+        // The outer root transaction should catch the ConnectionLost from the inner,
+        // and since it propagates through the root's inner try/catch,
+        // the root will retry with reconnection.
+        $result = $service->transaction(function () use ($service, &$outerCallCount, &$innerCallCount) {
+            $outerCallCount++;
+            if ($outerCallCount === 1) {
+                // First outer attempt: inner throws connection error
+                $service->transaction(function () use (&$innerCallCount) {
+                    $innerCallCount++;
+                    throw new TestRetryableException('packets out of order');
+                });
+            }
+            // Second outer attempt (after retry): succeeds
+            return 'recovered';
+        });
+
+        $this->assertSame('recovered', $result);
+        $this->assertSame(2, $outerCallCount, 'Root should retry after inner connection error');
+        $this->assertSame(1, $innerCallCount, 'Inner must NOT retry — only called once');
+    }
+
+    // ─────────────────────────────────────────────────────────────────────────
+    // INDIRECT NESTING: service method that opens its own transaction
+    // called from within another transaction
+    // ─────────────────────────────────────────────────────────────────────────
+
+    /**
+     * Simulates SummitOrderService.php:2287 calling CompanyService::addCompany()
+     * from within its own transaction. addCompany() opens its own transaction()
+     * which should run as nested.
+     */
+    public function testIndirectNestingServiceCallRunsAsNested(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->once(); // only root
+        $conn->shouldReceive('close')->byDefault();
+
+        $txActive = false;
+        $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) {
+            return $txActive;
+        });
+        $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) {
+            $txActive = true;
+        });
+        $conn->shouldReceive('commit')->byDefault();
+        $conn->shouldReceive('rollBack')->byDefault();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->byDefault();
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault();
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $service = new DoctrineTransactionService('default');
+
+        // Simulate CompanyService::addCompany() — has its own transaction() call
+        $addCompany = function () use ($service) {
+            return $service->transaction(function () {
+                // Simulates: CompanyFactory::build + repository->add + return company
+                return (object)['id' => 99, 'name' => 'Test Corp'];
+            });
+        };
+
+        // Simulate SummitOrderService outer transaction calling addCompany
+        $result = $service->transaction(function () use ($addCompany) {
+            $company = $addCompany();
+            // Caller uses the result (attendee->setCompany)
+            return "assigned:{$company->name}";
+        });
+
+        $this->assertSame('assigned:Test Corp', $result);
+    }
+
+    /**
+     * Simulates SummitService.php:3079 calling SpeakerService::addSpeaker()
+     * from within a transaction. The inner addSpeaker() transaction flushes
+     * (so IDs are available) but doesn't retry or reset EM.
+     */
+    public function testIndirectNestingInnerFlushGeneratesIds(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('close')->byDefault();
+
+        $txActive = false;
+        $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) {
+            return $txActive;
+        });
+        $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) {
+            $txActive = true;
+        });
+        $conn->shouldReceive('commit')->byDefault();
+        $conn->shouldReceive('rollBack')->byDefault();
+
+        // Track flush calls — both root and nested should flush
+        $flushCount = 0;
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->andReturnUsing(function () use (&$flushCount) {
+            $flushCount++;
+        });
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault();
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $service = new DoctrineTransactionService('default');
+
+        // Simulate SpeakerService::addSpeaker — its own transaction
+        $addSpeaker = function (array $data) use ($service) {
+            return $service->transaction(function () use ($data) {
+                // After flush, auto-increment ID would be available
+                return (object)['id' => 42, 'email' => $data['email']];
+            });
+        };
+
+        $result = $service->transaction(function () use ($addSpeaker) {
+            $speaker = $addSpeaker(['email' => 'speaker@example.com']);
+            // Caller uses speaker ID immediately
+            return "speaker_id:{$speaker->id}";
+        });
+
+        $this->assertSame('speaker_id:42', $result);
+        // Nested flush (addSpeaker) + root flush = 2 total
+        $this->assertSame(2, $flushCount, 'Both nested and root should flush');
+    }
+
+    /**
+     * Simulates SponsorUserSyncService.php:171 calling
+     * SummitSponsorService::addSponsorUser() which fails.
+     * The inner error propagates to the outer without destroying EM.
+     */
+    public function testIndirectNestingInnerServiceErrorDoesNotDestroyOuter(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('close')->byDefault();
+
+        $txActive = false;
+        $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) {
+            return $txActive;
+        });
+        $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) {
+            $txActive = true;
+        });
+        $conn->shouldReceive('commit')->byDefault();
+        $conn->shouldReceive('rollBack')->byDefault();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->byDefault();
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->never(); // critical: must NOT reset
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $service = new DoctrineTransactionService('default');
+
+        // Simulate addSponsorUser that throws validation error
+        $addSponsorUser = function () use ($service) {
+            return $service->transaction(function () {
+                throw new \InvalidArgumentException('Sponsor not found.');
+            });
+        };
+
+        // Outer transaction catches the inner failure and handles gracefully
+        $result = $service->transaction(function () use ($addSponsorUser) {
+            try {
+                $addSponsorUser();
+            } catch (\InvalidArgumentException $e) {
+                return "handled:{$e->getMessage()}";
+            }
+            return 'unreachable';
+        });
+
+        $this->assertSame('handled:Sponsor not found.', $result);
+    }
+
+    /**
+     * Simulates ScheduleService.php:510 calling SummitService::publishEvent()
+     * from within a transaction. publishEvent() has its own transaction that
+     * runs as nested — verifies the full happy-path flow with multiple
+     * indirect nested calls in sequence.
+     */
+    public function testIndirectNestingMultipleNestedCallsInSequence(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->once(); // only root
+        $conn->shouldReceive('close')->byDefault();
+
+        $txActive = false;
+        $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) {
+            return $txActive;
+        });
+        $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) {
+            $txActive = true;
+        });
+        $conn->shouldReceive('commit')->byDefault();
+        $conn->shouldReceive('rollBack')->byDefault();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->byDefault();
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->never();
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $service = new DoctrineTransactionService('default');
+
+        // Simulate multiple service calls (TagService::addTag, SpeakerService::addSpeaker)
+        $addTag = function (string $name) use ($service) {
+            return $service->transaction(function () use ($name) {
+                return (object)['id' => rand(1, 100), 'tag' => $name];
+            });
+        };
+
+        $addSpeaker = function (string $email) use ($service) {
+            return $service->transaction(function () use ($email) {
+                return (object)['id' => rand(1, 100), 'email' => $email];
+            });
+        };
+
+        $publishEvent = function () use ($service) {
+            return $service->transaction(function () {
+                return 'published';
+            });
+        };
+
+        // Outer transaction makes multiple service calls (simulates SummitSubmissionInvitationService)
+        $result = $service->transaction(function () use ($addTag, $addSpeaker, $publishEvent) {
+            $tag1 = $addTag('cloud');
+            $tag2 = $addTag('kubernetes');
+            $speaker = $addSpeaker('speaker@test.org');
+            $status = $publishEvent();
+            return "{$tag1->tag},{$tag2->tag},{$speaker->email},{$status}";
+        });
+
+        $this->assertSame('cloud,kubernetes,speaker@test.org,published', $result);
+    }
+
+    // ─────────────────────────────────────────────────────────────────────────
+    // EDGE CASES
+    // ─────────────────────────────────────────────────────────────────────────
+
+    /**
+     * Root transaction exhausts max retries on persistent connection errors.
+     */
+    public function testRootTransactionThrowsAfterMaxRetries(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('isTransactionActive')->andReturn(false, true);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('beginTransaction')->byDefault();
+        $conn->shouldReceive('commit')->never();
+        $conn->shouldReceive('rollBack')->byDefault();
+        $conn->shouldReceive('close')->byDefault();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->never();
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault();
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $callCount = 0;
+        $service = new DoctrineTransactionService('default');
+
+        $this->expectException(TestRetryableException::class);
+
+        $service->transaction(function () use (&$callCount) {
+            $callCount++;
+            throw new TestRetryableException('persistent failure');
+        });
+
+        // Should have been called MaxRetries times
+        $this->assertSame(DoctrineTransactionService::MaxRetries, $callCount);
+    }
+
+    /**
+     * Nested transaction that returns null — ensures null is properly propagated.
+     * Covers the SummitPromoCodeService pattern where inner TX returns null
+     * when promo code is not the expected type.
+     */
+    public function testNestedTransactionReturnsNull(): void
+    {
+        [$em, $conn] = $this->buildMocks(transactionActive: true);
+
+        $service = new DoctrineTransactionService('default');
+        $result = $service->transaction(function () {
+            return null;
+        });
+
+        $this->assertNull($result);
+    }
+
+    /**
+     * Root transaction with closed EntityManager — should reset before proceeding.
+     */
+    public function testRootTransactionResetsClosedEntityManager(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('isTransactionActive')->andReturn(false);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('beginTransaction')->byDefault();
+        $conn->shouldReceive('commit')->byDefault();
+        $conn->shouldReceive('rollBack')->byDefault();
+        $conn->shouldReceive('close')->byDefault();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(false); // EM is closed
+        $em->shouldReceive('flush')->byDefault();
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->byDefault();
+
+        $freshEm = Mockery::mock(EntityManagerInterface::class);
+        $freshEm->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $freshEm->shouldReceive('isOpen')->andReturn(true);
+        $freshEm->shouldReceive('flush')->once();
+        $freshEm->shouldReceive('clear')->byDefault();
+        $freshEm->shouldReceive('close')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($freshEm);
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $service = new DoctrineTransactionService('default');
+        $result = $service->transaction(function () {
+            return 'from-fresh-em';
+        });
+
+        $this->assertSame('from-fresh-em', $result);
+    }
+}

From 24c1077db6dc06e48735413a8e79782fa5e139da Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 01:18:29 -0300
Subject: [PATCH 02/13] fix(transactions): harden root/nested split against
 phantom writes and masked errors

Follow-up to the root/nested transaction split: closes review findings 1-3
plus xhigh code-review fixes on the accumulated diff.

- Root non-retryable failures now discard the UnitOfWork (em->clear()) so a
  failed callback's pending persists/changesets can't leak into the next
  transaction on the same EntityManager (phantom writes in catch-and-continue
  loops, e.g. CSV import per-row transactions).
- Nested transactions warn once when savepoints are disabled on the
  connection (outer transaction started outside this service), instead of
  failing later as an opaque rollback-only ConnectionException.
- Root transactions fail fast with a clear error when the callback swallowed
  a nested flush failure (EntityManager closed mid-transaction) instead of
  dying with an opaque EntityManagerClosed on the root's own flush; docblock
  narrowed to state the pattern is only safe for errors thrown before the
  nested flush.
- Rollback failures (root and nested) are now guarded so they can never mask
  the callback's original exception via Log::warning + finally.
- \Error throwables (not just \Exception) now reach the closed-EM recovery
  branch in the outer catch.

Adds 8 unit tests to the existing DoctrineTransactionServiceTest class
(22 total) covering each of the above; re-validated against a local MySQL
instance (real savepoints, real UniqueConstraintViolationException on nested
flush, real registry recovery) in addition to the mocked suite.
---
 .../Utils/DoctrineTransactionService.php      |  99 +++++++-
 .../DoctrineTransactionServiceTest.php        | 236 +++++++++++++++++-
 2 files changed, 317 insertions(+), 18 deletions(-)

diff --git a/app/Services/Utils/DoctrineTransactionService.php b/app/Services/Utils/DoctrineTransactionService.php
index 6cdf41a1e..a3394c954 100644
--- a/app/Services/Utils/DoctrineTransactionService.php
+++ b/app/Services/Utils/DoctrineTransactionService.php
@@ -33,6 +33,8 @@
  *  - owns the connection lifecycle (may retry on transient errors)
  *  - sets isolation level
  *  - flushes the EntityManager and issues the real COMMIT
+ *  - on failure, rolls back and clears the EntityManager so no
+ *    UnitOfWork state leaks into subsequent transactions
  *
  * Nested transaction (called while a DB transaction is already active):
  *  - uses Doctrine DBAL nesting counter (beginTransaction / commit)
@@ -43,8 +45,16 @@
  *
  * Savepoints are enabled (setNestTransactionsWithSavepoints) so that
  * nested rollBack() issues ROLLBACK TO SAVEPOINT — undoing only the
- * inner work without poisoning the outer transaction. This allows the
- * "catch inner error and continue" pattern to work correctly.
+ * inner SQL work without poisoning the outer transaction. The "catch
+ * inner error and continue" pattern is safe ONLY for errors thrown
+ * BEFORE the nested flush: if the nested flush itself fails, Doctrine
+ * closes the EntityManager, and the root transaction will fail fast
+ * with a clear RuntimeException before its own flush. In-memory entity
+ * state mutated by the failed inner callback is NOT reverted by the
+ * savepoint rollback; callers that catch a nested error and continue
+ * remain responsible for their own entity-level cleanup.
+ * Nested execution logs a warning when it detects savepoints are off
+ * (outer transaction started outside this service).
  */
 final class DoctrineTransactionService implements ITransactionService
 {
@@ -53,6 +63,11 @@ final class DoctrineTransactionService implements ITransactionService
      */
     private $manager_name;
 
+    /**
+     * @var bool
+     */
+    private $savepoints_warning_emitted = false;
+
     const MaxRetries = 10;
 
     /**
@@ -163,12 +178,47 @@ private function runRootTransaction(Closure $callback, int $isolationLevel)
 
                 try {
                     $result = $callback($this);
+                    if (!$em->isOpen()) {
+                        // A nested/inner flush failure closes the EM (ORM behavior);
+                        // reaching this point means the callback caught that error and
+                        // continued. Fail fast with the real cause instead of letting
+                        // the flush below die with an opaque EntityManagerClosed.
+                        // Plain \RuntimeException intentionally - shouldReconnect()
+                        // does not match it, so this can never trigger the retry loop.
+                        throw new \RuntimeException(
+                            'DoctrineTransactionService::runRootTransaction the EntityManager was closed during the callback '
+                            . '(typically a nested flush failure that was caught and execution continued); the transaction '
+                            . 'cannot be committed. Catching nested errors is only safe for errors thrown BEFORE the nested flush.'
+                        );
+                    }
                     $em->flush();
                     $conn->commit();
                     return $result;
                 } catch (\Throwable $inner) {
-                    if ($conn->isTransactionActive()) {
-                        $conn->rollBack();
+                    try {
+                        if ($conn->isTransactionActive()) {
+                            $conn->rollBack();
+                        }
+                    } catch (\Throwable $rollbackError) {
+                        // Never let a rollback failure replace the callback's exception:
+                        // a reconnectable rollback error would misclassify a business
+                        // failure as retryable and re-execute the whole callback.
+                        Log::warning(sprintf(
+                            "DoctrineTransactionService::runRootTransaction rollback failed after '%s': %s",
+                            $inner->getMessage(),
+                            $rollbackError->getMessage()
+                        ));
+                    } finally {
+                        // Root only: discard UnitOfWork state so pending persists/changesets
+                        // from the failed callback cannot leak into the next transaction on
+                        // this same EntityManager (phantom writes in catch-and-continue loops).
+                        // Guarded: an exception thrown in a finally supersedes the in-flight
+                        // one, so a secondary clear() failure must never mask $inner.
+                        // (ORM 3.3's clear() has no isOpen guard - this protects upgrades.)
+                        try {
+                            $em->clear();
+                        } catch (\Throwable $ignore) {
+                        }
                     }
                     throw $inner;
                 }
@@ -211,7 +261,22 @@ private function runRootTransaction(Closure $callback, int $isolationLevel)
 
                 Log::warning("DoctrineTransactionService::runRootTransaction rolling back TX");
                 Log::warning($ex);
+                if (!$em->isOpen()) {
+                    // A failed flush closes the EM (ORM behavior); reset so direct
+                    // Registry consumers (serializers, factories, workers) get a live
+                    // manager instead of an EntityManagerClosed on their next operation.
+                    Registry::resetManager($this->manager_name);
+                }
                 throw $ex;
+            } catch (\Throwable $throwable) {
+                // \Error throwables (TypeError, etc.) are not \Exception and skip the
+                // reconnect handling above, which typehints shouldReconnect(\Exception).
+                // They are never retried, but a callback that closed the EM before
+                // throwing one must still leave a live manager in the registry.
+                if (!$em->isOpen()) {
+                    Registry::resetManager($this->manager_name);
+                }
+                throw $throwable;
             }
         }
 
@@ -233,6 +298,16 @@ private function runNestedTransaction(Closure $callback)
         $em   = Registry::getManager($this->manager_name);
         $conn = $em->getConnection();
 
+        // Detection only: the flag cannot be enabled here (DBAL throws when a
+        // transaction is active), so surface the misconfiguration instead of
+        // letting it fail later as an opaque rollback-only error on the root commit.
+        // Warned once per service instance to avoid flooding logs when a bulk
+        // loop nests many calls under the same misconfigured outer transaction.
+        if (!$this->savepoints_warning_emitted && !$conn->getNestTransactionsWithSavepoints()) {
+            $this->savepoints_warning_emitted = true;
+            Log::warning('DoctrineTransactionService::runNestedTransaction savepoints are disabled for this connection (outer transaction not started by this service); a nested rollback will mark the outer transaction rollback-only.');
+        }
+
         $conn->beginTransaction();
 
         try {
@@ -241,8 +316,20 @@ private function runNestedTransaction(Closure $callback)
             $conn->commit();
             return $result;
         } catch (\Throwable $ex) {
-            if ($conn->isTransactionActive()) {
-                $conn->rollBack();
+            try {
+                if ($conn->isTransactionActive()) {
+                    $conn->rollBack();
+                }
+            } catch (\Throwable $rollbackError) {
+                // Never let a savepoint-rollback failure replace the callback's
+                // exception: a reconnectable rollback error would otherwise be
+                // reclassified by the root as retryable and re-run the callback,
+                // firing non-idempotent side effects again.
+                Log::warning(sprintf(
+                    "DoctrineTransactionService::runNestedTransaction rollback failed after '%s': %s",
+                    $ex->getMessage(),
+                    $rollbackError->getMessage()
+                ));
             }
             // No resetManager, no close(), no retry — let the root handle recovery.
             throw $ex;
diff --git a/tests/Unit/Services/DoctrineTransactionServiceTest.php b/tests/Unit/Services/DoctrineTransactionServiceTest.php
index ecd805fd3..d1b85d1df 100644
--- a/tests/Unit/Services/DoctrineTransactionServiceTest.php
+++ b/tests/Unit/Services/DoctrineTransactionServiceTest.php
@@ -75,13 +75,14 @@ protected function tearDown(): void
      * Build mocked EM + Connection and register them in the facade.
      *
      * @param bool $transactionActive  Initial state of isTransactionActive()
-     * @return array{EntityManagerInterface&\Mockery\MockInterface, Connection&\Mockery\MockInterface}
+     * @return array{EntityManagerInterface&\Mockery\MockInterface, Connection&\Mockery\MockInterface, ManagerRegistry&\Mockery\MockInterface}
      */
     private function buildMocks(bool $transactionActive = false): array
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('isTransactionActive')->andReturn($transactionActive)->byDefault();
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->byDefault();
         $conn->shouldReceive('commit')->byDefault();
@@ -113,6 +114,7 @@ public function testRootTransactionCommitsAndFlushes(): void
         [$em, $conn] = $this->buildMocks(transactionActive: false);
 
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')
             ->once()
             ->with(TransactionIsolationLevel::READ_COMMITTED);
@@ -133,7 +135,9 @@ public function testRootTransactionRollsBackOnNonRetryableException(): void
         [$em, $conn] = $this->buildMocks(transactionActive: false);
 
         $conn->shouldReceive('beginTransaction')->once();
-        $conn->shouldReceive('isTransactionActive')->andReturn(true);
+        // false: routing check picks the ROOT path; true: rollback check in the inner catch
+        // (a blanket `true` here would silently reroute this test to the nested path)
+        $conn->shouldReceive('isTransactionActive')->andReturn(false, true);
         $conn->shouldReceive('rollBack')->once();
         $conn->shouldReceive('commit')->never();
         $em->shouldReceive('flush')->never();
@@ -152,6 +156,7 @@ public function testRootTransactionRetriesOnConnectionLost(): void
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->byDefault();
         $conn->shouldReceive('commit')->byDefault();
@@ -202,6 +207,7 @@ public function testRootTransactionResetsManagerOnConnectionError(): void
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('isTransactionActive')->andReturn(false, true, false);
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->byDefault();
         $conn->shouldReceive('commit')->byDefault();
@@ -212,7 +218,7 @@ public function testRootTransactionResetsManagerOnConnectionError(): void
         $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
         $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
         $em->shouldReceive('flush')->byDefault();
-        $em->shouldReceive('clear')->once(); // destructive recovery
+        $em->shouldReceive('clear')->atLeast()->once(); // destructive recovery (also cleared by the root inner catch)
         $em->shouldReceive('close')->once(); // destructive recovery
 
         $registry = Mockery::mock(ManagerRegistry::class);
@@ -295,6 +301,7 @@ public function testNestedTransactionRollsBackAndRethrowsOnError(): void
         $conn->shouldReceive('rollBack')->once();
         $conn->shouldReceive('commit')->never();
         $em->shouldReceive('flush')->never(); // flush not reached on error
+        $em->shouldReceive('clear')->never(); // nested must never discard the outer's in-flight UoW state
 
         $service = new DoctrineTransactionService('default');
 
@@ -323,6 +330,7 @@ public function testDirectNestingInnerRunsAsNested(): void
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->once(); // only root sets isolation
         $conn->shouldReceive('close')->byDefault();
 
@@ -381,6 +389,7 @@ public function testDirectNestingInnerErrorPropagatesWithoutDestroyingEM(): void
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('close')->never(); // inner must NOT close connection
 
@@ -439,6 +448,7 @@ public function testDirectNestingConnectionErrorInInnerDoesNotDestroyOuterTx():
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
 
         $txActive = false;
@@ -506,6 +516,7 @@ public function testIndirectNestingServiceCallRunsAsNested(): void
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->once(); // only root
         $conn->shouldReceive('close')->byDefault();
 
@@ -561,6 +572,7 @@ public function testIndirectNestingInnerFlushGeneratesIds(): void
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('close')->byDefault();
 
@@ -621,6 +633,7 @@ public function testIndirectNestingInnerServiceErrorDoesNotDestroyOuter(): void
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('close')->byDefault();
 
@@ -679,6 +692,7 @@ public function testIndirectNestingMultipleNestedCallsInSequence(): void
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->once(); // only root
         $conn->shouldReceive('close')->byDefault();
 
@@ -750,6 +764,7 @@ public function testRootTransactionThrowsAfterMaxRetries(): void
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('isTransactionActive')->andReturn(false, true);
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->byDefault();
         $conn->shouldReceive('commit')->never();
@@ -772,15 +787,16 @@ public function testRootTransactionThrowsAfterMaxRetries(): void
         $callCount = 0;
         $service = new DoctrineTransactionService('default');
 
-        $this->expectException(TestRetryableException::class);
-
-        $service->transaction(function () use (&$callCount) {
-            $callCount++;
-            throw new TestRetryableException('persistent failure');
-        });
-
-        // Should have been called MaxRetries times
-        $this->assertSame(DoctrineTransactionService::MaxRetries, $callCount);
+        try {
+            $service->transaction(function () use (&$callCount) {
+                $callCount++;
+                throw new TestRetryableException('persistent failure');
+            });
+            $this->fail('Expected retryable exception was not thrown');
+        } catch (TestRetryableException $e) {
+            // Should have been called MaxRetries times
+            $this->assertSame(DoctrineTransactionService::MaxRetries, $callCount);
+        }
     }
 
     /**
@@ -808,6 +824,7 @@ public function testRootTransactionResetsClosedEntityManager(): void
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('isTransactionActive')->andReturn(false);
         $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->byDefault();
         $conn->shouldReceive('commit')->byDefault();
@@ -841,4 +858,199 @@ public function testRootTransactionResetsClosedEntityManager(): void
 
         $this->assertSame('from-fresh-em', $result);
     }
+
+    /**
+     * A non-retryable failure in a ROOT transaction must discard the
+     * UnitOfWork state ($em->clear()) after rolling back, so pending
+     * persists/changesets from the failed callback cannot leak into the
+     * NEXT transaction on the same EntityManager (phantom writes in
+     * catch-and-continue loops), while staying non-destructive on the
+     * connection/manager (no resetManager, no close).
+     */
+    public function testRootTransactionDiscardsPendingEntitiesAfterNonRetryableFailure(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        // false: routing check picks the ROOT path; true: rollback check in the inner catch
+        $conn->shouldReceive('isTransactionActive')->andReturn(false, true);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('beginTransaction')->once();
+        $conn->shouldReceive('commit')->never();
+        $conn->shouldReceive('rollBack')->once();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(true)->byDefault();
+        $em->shouldReceive('flush')->never();
+        $em->shouldReceive('clear')->once(); // the UoW discard under test
+        $em->shouldReceive('close')->never();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->never();
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $service = new DoctrineTransactionService('default');
+
+        $this->expectException(\RuntimeException::class);
+        $this->expectExceptionMessage('business error');
+
+        $service->transaction(function () {
+            throw new \RuntimeException('business error');
+        });
+    }
+
+    /**
+     * When the connection's savepoint flag is OFF (outer transaction started
+     * outside DoctrineTransactionService), the nested path must log a warning
+     * surfacing the rollback-only poisoning risk, while still executing the
+     * transaction normally. Detection only - it must NOT try to enable
+     * savepoints mid-transaction (throws on DBAL 3.9).
+     */
+    public function testNestedTransactionWarnsWhenSavepointsDisabled(): void
+    {
+        [$em, $conn] = $this->buildMocks(transactionActive: true);
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(false);
+
+        $spy = new class {
+            public array $calls = [];
+            public function __call($name, $args) { $this->calls[] = [$name, $args]; }
+        };
+        $this->container->instance('log', $spy);
+        Facade::clearResolvedInstances();
+
+        $service = new DoctrineTransactionService('default');
+        $result = $service->transaction(function () {
+            return 'ok';
+        });
+
+        $this->assertSame('ok', $result, 'Nested transaction must proceed normally despite the warning');
+        $warnings = array_values(array_filter($spy->calls, fn ($c) => $c[0] === 'warning'));
+        $this->assertCount(1, $warnings, 'Exactly one warning must be logged when savepoints are off');
+        $this->assertStringContainsString('savepoints', $warnings[0][1][0]);
+
+        // Warn-once: a second nested call on the same service instance must NOT re-warn
+        $service->transaction(function () {
+            return 'ok-again';
+        });
+        $warnings = array_values(array_filter($spy->calls, fn ($c) => $c[0] === 'warning'));
+        $this->assertCount(1, $warnings, 'The savepoints warning must be emitted once per service instance, not per call');
+    }
+
+    /**
+     * A callback that swallowed a nested flush failure returns normally with a
+     * CLOSED EntityManager (ORM closes it on any failed flush). The root must
+     * fail fast with a clear error BEFORE its own flush - instead of the opaque
+     * EntityManagerClosed - must NOT retry, and must leave a live manager in
+     * the registry (closed-EM reset branch).
+     */
+    public function testRootTransactionFailsFastWhenCallbackClosedEntityManager(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        // false: routing check picks the ROOT path; true: rollback check in the inner catch
+        $conn->shouldReceive('isTransactionActive')->andReturn(false, true);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('beginTransaction')->once();
+        $conn->shouldReceive('commit')->never();
+        $conn->shouldReceive('rollBack')->once();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        // true: loop-top open check; false: post-callback fail-fast check (repeats
+        // false so the closed-EM resetManager branch in the outer catch fires too)
+        $em->shouldReceive('isOpen')->andReturn(true, false);
+        $em->shouldReceive('flush')->never();
+        $em->shouldReceive('clear')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($em);
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $callCount = 0;
+        $service = new DoctrineTransactionService('default');
+
+        try {
+            $service->transaction(function () use (&$callCount) {
+                $callCount++;
+                return 'swallowed-inner-failure';
+            });
+            $this->fail('Expected fail-fast RuntimeException was not thrown');
+        } catch (\RuntimeException $e) {
+            $this->assertStringContainsString('EntityManager was closed', $e->getMessage());
+        }
+        $this->assertSame(1, $callCount, 'Fail-fast must not trigger the retry loop');
+    }
+
+    /**
+     * A rollback failure in the NESTED path must never replace the callback's
+     * original exception: masking it would let the root classify the rollback
+     * error (often reconnectable) instead of the business error, re-running
+     * non-idempotent side effects on retry.
+     */
+    public function testNestedTransactionPreservesCallbackExceptionWhenRollbackFails(): void
+    {
+        [$em, $conn] = $this->buildMocks(transactionActive: true);
+
+        $conn->shouldReceive('isTransactionActive')->andReturn(true);
+        $conn->shouldReceive('rollBack')->once()->andThrow(new TestRetryableException('connection died during savepoint rollback'));
+        $conn->shouldReceive('commit')->never();
+        $em->shouldReceive('flush')->never();
+
+        $service = new DoctrineTransactionService('default');
+
+        try {
+            $service->transaction(function () {
+                throw new \LogicException('inner business error');
+            });
+            $this->fail('Expected the callback exception to propagate');
+        } catch (\LogicException $e) {
+            $this->assertStringContainsString('inner business error', $e->getMessage());
+        }
+    }
+
+    /**
+     * PHP \Error throwables from the callback must still reach the closed-EM
+     * recovery branch: a TypeError with a closed EM must reset the manager so
+     * direct Registry consumers get a live one afterwards.
+     */
+    public function testRootTransactionRecoversClosedManagerWhenCallbackThrowsError(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        // false: routing check picks the ROOT path; true: rollback check in the inner catch
+        $conn->shouldReceive('isTransactionActive')->andReturn(false, true);
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('beginTransaction')->once();
+        $conn->shouldReceive('commit')->never();
+        $conn->shouldReceive('rollBack')->once();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        // true: loop-top check; false: closed-EM recovery check in the outer catch
+        $em->shouldReceive('isOpen')->andReturn(true, false);
+        $em->shouldReceive('flush')->never();
+        $em->shouldReceive('clear')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($em);
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $service = new DoctrineTransactionService('default');
+
+        $this->expectException(\TypeError::class);
+        $this->expectExceptionMessage('boom');
+
+        $service->transaction(function () {
+            throw new \TypeError('boom');
+        });
+    }
 }

From 248f8e4536e6f0714fbaf9647ce9bb22ee3e6cae Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 11:52:55 -0300
Subject: [PATCH 03/13] fix(tx): remove DBAL savepoints, propagate native
 isRollbackOnly guard

DoctrineTransactionService no longer enables setNestTransactionsWithSavepoints.
Nested transactions are now pure DBAL nesting-counter bookkeeping; a nested
rollBack() marks the shared connection rollback-only (no SQL), and commit()
at any level - nested, root, or ORM's own internal per-flush() commit - fails
immediately once that flag is set. A nested failure can therefore never be
silently absorbed into a successful root commit, even if an intermediate
callback catches it and continues.

Also:
- transaction() now checks $em->isOpen() (not just isTransactionActive()) when
  deciding root vs nested routing, closing a narrow double-fault gap where a
  closed EntityManager could be routed into runNestedTransaction() with no
  reset path.
- Extracted the duplicated post-callback closed-EM guard and rollback/log
  block (root vs nested) into shared private helpers.
- Added functional tests against real MySQL (SummitOrderServiceTest) covering
  addTickets/createOfflineOrder's real nested-transaction chains: happy path
  commits across nested + sequential root transactions, and a deep failure
  (invalid promo code / missing default badge type) rolls back the entire
  chain, including ticket-type quantity_sold.

Verified: DoctrineTransactionServiceTest 25/25, SummitOrderServiceTest 23/23,
full tests/Unit 249/250 (1 pre-existing unrelated failure).
---
 .../Utils/DoctrineTransactionService.php      | 165 ++++++-------
 tests/SummitOrderServiceTest.php              | 122 ++++++++++
 .../DoctrineTransactionServiceTest.php        | 219 ++++++++++++------
 3 files changed, 351 insertions(+), 155 deletions(-)

diff --git a/app/Services/Utils/DoctrineTransactionService.php b/app/Services/Utils/DoctrineTransactionService.php
index a3394c954..654a3fb0d 100644
--- a/app/Services/Utils/DoctrineTransactionService.php
+++ b/app/Services/Utils/DoctrineTransactionService.php
@@ -43,18 +43,19 @@
  *  - on error, rolls back only the nested level and re-throws,
  *    letting the root transaction decide how to handle the failure
  *
- * Savepoints are enabled (setNestTransactionsWithSavepoints) so that
- * nested rollBack() issues ROLLBACK TO SAVEPOINT — undoing only the
- * inner SQL work without poisoning the outer transaction. The "catch
- * inner error and continue" pattern is safe ONLY for errors thrown
- * BEFORE the nested flush: if the nested flush itself fails, Doctrine
- * closes the EntityManager, and the root transaction will fail fast
- * with a clear RuntimeException before its own flush. In-memory entity
- * state mutated by the failed inner callback is NOT reverted by the
- * savepoint rollback; callers that catch a nested error and continue
- * remain responsible for their own entity-level cleanup.
- * Nested execution logs a warning when it detects savepoints are off
- * (outer transaction started outside this service).
+ * Savepoints are never enabled. Nested transactions are pure DBAL nesting-
+ * counter bookkeeping (beginTransaction / commit / rollBack with no SQL at
+ * nesting level > 1) - this is deliberate, not an oversight: Doctrine's
+ * UnitOfWork is not savepoint-aware, so a savepoint rollback can silently
+ * desynchronize the ORM's belief about what is persisted from what the
+ * database actually holds. Without savepoints, DBAL's own isRollbackOnly
+ * flag does the job instead: a nested rollBack() marks the shared connection
+ * rollback-only (no SQL), and commit() at ANY level - nested, root, or even
+ * Doctrine ORM's own internal per-flush() commit - checks that flag first
+ * and fails immediately. A nested failure can therefore never be silently
+ * absorbed into a successful root commit, even if an intermediate callback
+ * catches it and continues; the whole business operation succeeds or fails
+ * as one atomic unit.
  */
 final class DoctrineTransactionService implements ITransactionService
 {
@@ -63,11 +64,6 @@ final class DoctrineTransactionService implements ITransactionService
      */
     private $manager_name;
 
-    /**
-     * @var bool
-     */
-    private $savepoints_warning_emitted = false;
-
     const MaxRetries = 10;
 
     /**
@@ -139,8 +135,13 @@ public function transaction(Closure $callback, int $isolationLevel = Transaction
         $em   = Registry::getManager($this->manager_name);
         $conn = $em->getConnection();
 
-        // Detect whether we are already inside a DB transaction.
-        $isNested = $conn->isTransactionActive();
+        // Detect whether we are already inside a DB transaction. A closed EntityManager
+        // is never treated as nested, even if the connection still reports an active
+        // transaction (e.g. a prior failure whose rollback attempt itself failed):
+        // runNestedTransaction() has no isOpen()-based reset, so routing a closed EM
+        // there would hand the callback a dead EntityManager instead of going through
+        // runRootTransaction()'s existing reset-and-retry path.
+        $isNested = $em->isOpen() && $conn->isTransactionActive();
 
         if ($isNested) {
             return $this->runNestedTransaction($callback);
@@ -149,6 +150,59 @@ public function transaction(Closure $callback, int $isolationLevel = Transaction
         return $this->runRootTransaction($callback, $isolationLevel);
     }
 
+    /**
+     * A nested/inner flush failure closes the EntityManager (ORM behavior); reaching
+     * this point after a callback returns normally means it caught that error and
+     * continued. Fail fast with the real cause instead of letting the flush below
+     * die with an opaque EntityManagerClosed one or more nesting levels up from
+     * where it actually happened.
+     *
+     * @param \Doctrine\ORM\EntityManagerInterface $em
+     * @param string $context
+     * @throws \RuntimeException
+     */
+    private function failFastIfEntityManagerClosed($em, string $context): void
+    {
+        if (!$em->isOpen()) {
+            // Plain \RuntimeException intentionally - shouldReconnect() does not
+            // match it, so this can never trigger the retry loop.
+            throw new \RuntimeException(sprintf(
+                'DoctrineTransactionService::%s the EntityManager was closed during the callback '
+                . '(typically a nested flush failure that was caught and execution continued); this '
+                . 'transaction cannot be committed. Catching nested errors is only safe for errors '
+                . 'thrown before any flush in this transaction.',
+                $context
+            ));
+        }
+    }
+
+    /**
+     * Rolls back the given connection if it still has an active transaction,
+     * swallowing any rollback failure. Never lets a rollback failure replace the
+     * callback's original exception: a reconnectable rollback error would
+     * otherwise misclassify a business failure as retryable and re-execute
+     * non-idempotent side effects.
+     *
+     * @param \Doctrine\DBAL\Connection $conn
+     * @param \Throwable $cause
+     * @param string $context
+     */
+    private function safeRollback($conn, \Throwable $cause, string $context): void
+    {
+        try {
+            if ($conn->isTransactionActive()) {
+                $conn->rollBack();
+            }
+        } catch (\Throwable $rollbackError) {
+            Log::warning(sprintf(
+                "DoctrineTransactionService::%s rollback failed after '%s': %s",
+                $context,
+                $cause->getMessage(),
+                $rollbackError->getMessage()
+            ));
+        }
+    }
+
     /**
      * Root (outermost) transaction: may retry on transient connection errors,
      * sets isolation level, flushes, and issues the real COMMIT.
@@ -172,53 +226,25 @@ private function runRootTransaction(Closure $callback, int $isolationLevel)
                 }
 
                 $conn = $em->getConnection();
-                $conn->setNestTransactionsWithSavepoints(true);
                 $conn->setTransactionIsolation($isolationLevel);
                 $conn->beginTransaction();
 
                 try {
                     $result = $callback($this);
-                    if (!$em->isOpen()) {
-                        // A nested/inner flush failure closes the EM (ORM behavior);
-                        // reaching this point means the callback caught that error and
-                        // continued. Fail fast with the real cause instead of letting
-                        // the flush below die with an opaque EntityManagerClosed.
-                        // Plain \RuntimeException intentionally - shouldReconnect()
-                        // does not match it, so this can never trigger the retry loop.
-                        throw new \RuntimeException(
-                            'DoctrineTransactionService::runRootTransaction the EntityManager was closed during the callback '
-                            . '(typically a nested flush failure that was caught and execution continued); the transaction '
-                            . 'cannot be committed. Catching nested errors is only safe for errors thrown BEFORE the nested flush.'
-                        );
-                    }
+                    $this->failFastIfEntityManagerClosed($em, 'runRootTransaction');
                     $em->flush();
                     $conn->commit();
                     return $result;
                 } catch (\Throwable $inner) {
+                    $this->safeRollback($conn, $inner, 'runRootTransaction');
+                    // Root only: discard UnitOfWork state so pending persists/changesets
+                    // from the failed callback cannot leak into the next transaction on
+                    // this same EntityManager (phantom writes in catch-and-continue loops).
+                    // A secondary clear() failure must never mask $inner.
+                    // (ORM 3.3's clear() has no isOpen guard - this protects upgrades.)
                     try {
-                        if ($conn->isTransactionActive()) {
-                            $conn->rollBack();
-                        }
-                    } catch (\Throwable $rollbackError) {
-                        // Never let a rollback failure replace the callback's exception:
-                        // a reconnectable rollback error would misclassify a business
-                        // failure as retryable and re-execute the whole callback.
-                        Log::warning(sprintf(
-                            "DoctrineTransactionService::runRootTransaction rollback failed after '%s': %s",
-                            $inner->getMessage(),
-                            $rollbackError->getMessage()
-                        ));
-                    } finally {
-                        // Root only: discard UnitOfWork state so pending persists/changesets
-                        // from the failed callback cannot leak into the next transaction on
-                        // this same EntityManager (phantom writes in catch-and-continue loops).
-                        // Guarded: an exception thrown in a finally supersedes the in-flight
-                        // one, so a secondary clear() failure must never mask $inner.
-                        // (ORM 3.3's clear() has no isOpen guard - this protects upgrades.)
-                        try {
-                            $em->clear();
-                        } catch (\Throwable $ignore) {
-                        }
+                        $em->clear();
+                    } catch (\Throwable $ignore) {
                     }
                     throw $inner;
                 }
@@ -298,39 +324,16 @@ private function runNestedTransaction(Closure $callback)
         $em   = Registry::getManager($this->manager_name);
         $conn = $em->getConnection();
 
-        // Detection only: the flag cannot be enabled here (DBAL throws when a
-        // transaction is active), so surface the misconfiguration instead of
-        // letting it fail later as an opaque rollback-only error on the root commit.
-        // Warned once per service instance to avoid flooding logs when a bulk
-        // loop nests many calls under the same misconfigured outer transaction.
-        if (!$this->savepoints_warning_emitted && !$conn->getNestTransactionsWithSavepoints()) {
-            $this->savepoints_warning_emitted = true;
-            Log::warning('DoctrineTransactionService::runNestedTransaction savepoints are disabled for this connection (outer transaction not started by this service); a nested rollback will mark the outer transaction rollback-only.');
-        }
-
         $conn->beginTransaction();
 
         try {
             $result = $callback($this);
+            $this->failFastIfEntityManagerClosed($em, 'runNestedTransaction');
             $em->flush();
             $conn->commit();
             return $result;
         } catch (\Throwable $ex) {
-            try {
-                if ($conn->isTransactionActive()) {
-                    $conn->rollBack();
-                }
-            } catch (\Throwable $rollbackError) {
-                // Never let a savepoint-rollback failure replace the callback's
-                // exception: a reconnectable rollback error would otherwise be
-                // reclassified by the root as retryable and re-run the callback,
-                // firing non-idempotent side effects again.
-                Log::warning(sprintf(
-                    "DoctrineTransactionService::runNestedTransaction rollback failed after '%s': %s",
-                    $ex->getMessage(),
-                    $rollbackError->getMessage()
-                ));
-            }
+            $this->safeRollback($conn, $ex, 'runNestedTransaction');
             // No resetManager, no close(), no retry — let the root handle recovery.
             throw $ex;
         }
diff --git a/tests/SummitOrderServiceTest.php b/tests/SummitOrderServiceTest.php
index daf42f230..d02702cdb 100644
--- a/tests/SummitOrderServiceTest.php
+++ b/tests/SummitOrderServiceTest.php
@@ -35,6 +35,8 @@
 use Illuminate\Support\Facades\Queue;
 use libs\utils\ITransactionService;
 use Mockery;
+use models\exceptions\EntityNotFoundException;
+use models\exceptions\ValidationException;
 use models\main\ICompanyRepository;
 use models\main\IMemberRepository;
 use models\main\ITagRepository;
@@ -937,4 +939,124 @@ public function testImportTicketDataCreatesBadgeWhenTicketHasNone()
         $this->assertTrue($ticket->hasBadge());
         $this->assertEquals('VIP BADGE', $ticket->getBadge()->getType()->getName());
     }
+
+    public function testAddTicketsCommitsAcrossNestedTransaction()
+    {
+        $service = App::make(ISummitOrderService::class);
+
+        $order = self::$summit->getOrders()[0];
+        $order_id = $order->getId();
+        $initial_ticket_count = $order->getTickets()->count();
+
+        $result = $service->addTickets(self::$summit, $order_id, [
+            'ticket_type_id' => self::$default_ticket_type->getId(),
+            'ticket_qty' => 2,
+        ]);
+
+        $this->assertEquals($initial_ticket_count + 2, $result->getTickets()->count());
+
+        self::$em->clear();
+
+        $reFetched = self::$em->find(SummitOrder::class, $order_id);
+        $this->assertNotNull($reFetched);
+        $this->assertEquals($initial_ticket_count + 2, $reFetched->getTickets()->count());
+    }
+
+    public function testCreateOfflineOrderCommitsAcrossNestedAndSequentialRootTransactions()
+    {
+        $service = App::make(ISummitOrderService::class);
+
+        $owner_email = sprintf('offline-order-%s@test.com', uniqid());
+
+        $payload = [
+            'owner_email' => $owner_email,
+            'owner_first_name' => 'Offline',
+            'owner_last_name' => 'Buyer',
+            'ticket_type_id' => self::$default_ticket_type->getId(),
+            'ticket_qty' => 1,
+        ];
+
+        $order = $service->createOfflineOrder(self::$summit, $payload);
+        $order_id = $order->getId();
+
+        $this->assertTrue($order->isPaid());
+        $this->assertEquals(1, $order->getTickets()->count());
+        $this->assertEquals($owner_email, $order->getFirstTicket()->getOwner()->getEmail());
+
+        self::$em->clear();
+
+        $reFetched = self::$em->find(SummitOrder::class, $order_id);
+        $this->assertNotNull($reFetched);
+        $this->assertTrue($reFetched->isPaid());
+        $this->assertEquals(1, $reFetched->getTickets()->count());
+        $this->assertEquals($owner_email, $reFetched->getFirstTicket()->getOwner()->getEmail());
+    }
+
+    public function testCreateTicketsForOrderRollsBackEntireChainOnInvalidPromoCode()
+    {
+        $service = App::make(ISummitOrderService::class);
+
+        $order = self::$summit->getOrders()[0];
+        $order_id = $order->getId();
+        $initial_ticket_count = $order->getTickets()->count();
+        $ticket_type_id = self::$default_ticket_type->getId();
+        $initial_quantity_sold = self::$default_ticket_type->getQuantitySold();
+        $bogus_promo_code = 'DOES-NOT-EXIST-' . uniqid();
+
+        try {
+            $service->addTickets(self::$summit, $order_id, [
+                'ticket_type_id' => $ticket_type_id,
+                'ticket_qty' => 1,
+                'promo_code' => $bogus_promo_code,
+            ]);
+            $this->fail('Expected EntityNotFoundException was not thrown');
+        } catch (EntityNotFoundException $ex) {
+            $this->assertStringContainsString('Promo code', $ex->getMessage());
+        }
+
+        self::$em->clear();
+
+        $reFetched = self::$em->find(SummitOrder::class, $order_id);
+        $this->assertNotNull($reFetched);
+        $this->assertEquals($initial_ticket_count, $reFetched->getTickets()->count());
+
+        // proves the SummitTicketType::sell(1) mutation made before the promo-code
+        // lookup threw was rolled back too, not just the order's ticket collection
+        $reFetchedTicketType = self::$em->find(SummitTicketType::class, $ticket_type_id);
+        $this->assertNotNull($reFetchedTicketType);
+        $this->assertEquals($initial_quantity_sold, $reFetchedTicketType->getQuantitySold());
+    }
+
+    public function testCreateOfflineOrderRollsBackEntireChainOnMissingDefaultBadgeType()
+    {
+        $service = App::make(ISummitOrderService::class);
+        $attendee_repository = App::make(ISummitAttendeeRepository::class);
+
+        self::$default_badge_type->setIsDefault(false);
+
+        $owner_email = sprintf('offline-order-badge-fail-%s@test.com', uniqid());
+
+        $payload = [
+            'owner_email' => $owner_email,
+            'owner_first_name' => 'Offline',
+            'owner_last_name' => 'BadgeFail',
+            'ticket_type_id' => self::$default_ticket_type->getId(),
+            'ticket_qty' => 1,
+        ];
+
+        try {
+            $service->createOfflineOrder(self::$summit, $payload);
+            $this->fail('Expected ValidationException was not thrown');
+        } catch (ValidationException $ex) {
+            $this->assertStringContainsString('default badge type', $ex->getMessage());
+        } finally {
+            self::$default_badge_type->setIsDefault(true);
+            self::$em->flush();
+        }
+
+        self::$em->clear();
+
+        $attendee = $attendee_repository->getBySummitAndEmail(self::$summit, $owner_email);
+        $this->assertNull($attendee);
+    }
 }
diff --git a/tests/Unit/Services/DoctrineTransactionServiceTest.php b/tests/Unit/Services/DoctrineTransactionServiceTest.php
index d1b85d1df..aaf62140d 100644
--- a/tests/Unit/Services/DoctrineTransactionServiceTest.php
+++ b/tests/Unit/Services/DoctrineTransactionServiceTest.php
@@ -81,8 +81,6 @@ private function buildMocks(bool $transactionActive = false): array
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('isTransactionActive')->andReturn($transactionActive)->byDefault();
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->byDefault();
         $conn->shouldReceive('commit')->byDefault();
@@ -113,8 +111,33 @@ public function testRootTransactionCommitsAndFlushes(): void
     {
         [$em, $conn] = $this->buildMocks(transactionActive: false);
 
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
+        $conn->shouldReceive('setTransactionIsolation')
+            ->once()
+            ->with(TransactionIsolationLevel::READ_COMMITTED);
+        $conn->shouldReceive('beginTransaction')->once();
+        $em->shouldReceive('flush')->once();
+        $conn->shouldReceive('commit')->once();
+
+        $service = new DoctrineTransactionService('default');
+        $result = $service->transaction(function () {
+            return 'success';
+        });
+
+        $this->assertSame('success', $result);
+    }
+
+    /**
+     * DBAL savepoints are never enabled: nested transactions rely entirely on
+     * DBAL's native isRollbackOnly propagation (rollBack() at nesting level > 1
+     * without savepoints just marks the connection + decrements the counter;
+     * commit() at ANY level checks isRollbackOnly first) instead of savepoints,
+     * which desynchronize Doctrine's UnitOfWork from the database.
+     */
+    public function testRootTransactionNeverEnablesSavepoints(): void
+    {
+        [$em, $conn] = $this->buildMocks(transactionActive: false);
+
+        $conn->shouldReceive('setNestTransactionsWithSavepoints')->never();
         $conn->shouldReceive('setTransactionIsolation')
             ->once()
             ->with(TransactionIsolationLevel::READ_COMMITTED);
@@ -155,8 +178,6 @@ public function testRootTransactionRollsBackOnNonRetryableException(): void
     public function testRootTransactionRetriesOnConnectionLost(): void
     {
         $conn = Mockery::mock(Connection::class);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->byDefault();
         $conn->shouldReceive('commit')->byDefault();
@@ -206,8 +227,6 @@ public function testRootTransactionResetsManagerOnConnectionError(): void
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('isTransactionActive')->andReturn(false, true, false);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->byDefault();
         $conn->shouldReceive('commit')->byDefault();
@@ -240,6 +259,52 @@ public function testRootTransactionResetsManagerOnConnectionError(): void
         $this->assertSame(2, $callCount);
     }
 
+    /**
+     * transaction()'s root-vs-nested routing must not trust conn->isTransactionActive()
+     * alone: a prior failure can leave the EntityManager closed while a failed/skipped
+     * rollback leaves the connection still reporting an active transaction. Routing that
+     * state into runNestedTransaction() (which has no isOpen()-based reset) would hand the
+     * callback a closed EntityManager instead of the clear, actionable reset-and-retry root
+     * path runRootTransaction() already provides.
+     */
+    public function testTransactionRoutesToRootWhenEntityManagerClosedDespiteActiveConnectionFlag(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        $conn->shouldReceive('isTransactionActive')->andReturn(true);
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('beginTransaction')->byDefault();
+        $conn->shouldReceive('commit')->byDefault();
+        $conn->shouldReceive('rollBack')->byDefault();
+        $conn->shouldReceive('close')->byDefault();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $em->shouldReceive('isOpen')->andReturn(false); // closed
+        $em->shouldReceive('flush')->byDefault();
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->byDefault();
+
+        $freshEm = Mockery::mock(EntityManagerInterface::class);
+        $freshEm->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        $freshEm->shouldReceive('isOpen')->andReturn(true);
+        $freshEm->shouldReceive('flush')->once();
+        $freshEm->shouldReceive('clear')->byDefault();
+        $freshEm->shouldReceive('close')->byDefault();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($freshEm);
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $service = new DoctrineTransactionService('default');
+        $result = $service->transaction(function () {
+            return 'from-fresh-em';
+        });
+
+        $this->assertSame('from-fresh-em', $result);
+    }
+
     // ─────────────────────────────────────────────────────────────────────────
     // NESTED TRANSACTION TESTS
     // ─────────────────────────────────────────────────────────────────────────
@@ -263,6 +328,28 @@ public function testNestedTransactionFlushesAndCommits(): void
         $this->assertSame('nested-result', $result);
     }
 
+    /**
+     * Nested transactions never inspect the connection's savepoint flag - the
+     * savepoints-disabled warning mechanism (Finding 2) is removed entirely,
+     * since nested no longer relies on savepoints at all.
+     */
+    public function testNestedTransactionNeverQueriesSavepointsFlag(): void
+    {
+        [$em, $conn] = $this->buildMocks(transactionActive: true);
+
+        $conn->shouldReceive('getNestTransactionsWithSavepoints')->never();
+        $conn->shouldReceive('beginTransaction')->once();
+        $em->shouldReceive('flush')->once();
+        $conn->shouldReceive('commit')->once();
+
+        $service = new DoctrineTransactionService('default');
+        $result = $service->transaction(function () {
+            return 'nested-result';
+        });
+
+        $this->assertSame('nested-result', $result);
+    }
+
     public function testNestedTransactionDoesNotRetryOnConnectionError(): void
     {
         [$em, $conn] = $this->buildMocks(transactionActive: true);
@@ -313,6 +400,40 @@ public function testNestedTransactionRollsBackAndRethrowsOnError(): void
         });
     }
 
+    /**
+     * Mirrors testRootTransactionFailsFastWhenCallbackClosedEntityManager for
+     * the nested path (finding #9): with 2+ levels of nesting, a deeper
+     * nested flush failure closes the EntityManager; if a middle callback
+     * catches that and continues, THIS nested transaction must also fail
+     * fast with a clear error - not proceed to flush() and surface an
+     * opaque EntityManagerClosed one level higher than before.
+     *
+     * runNestedTransaction has no retry loop, so there is exactly ONE
+     * isOpen() call after this fix (the post-callback guard) - a single
+     * `false`, not a two-value sequence like the root test.
+     */
+    public function testNestedTransactionFailsFastWhenDeeperNestedFlushClosedEntityManager(): void
+    {
+        [$em, $conn] = $this->buildMocks(transactionActive: true);
+
+        $em->shouldReceive('isOpen')->andReturn(false);
+        $conn->shouldReceive('isTransactionActive')->andReturn(true);
+        $conn->shouldReceive('rollBack')->once();
+        $em->shouldReceive('flush')->never();
+        $conn->shouldReceive('commit')->never();
+
+        $service = new DoctrineTransactionService('default');
+
+        try {
+            $service->transaction(function () {
+                return 'swallowed-deeper-failure';
+            });
+            $this->fail('Expected fail-fast RuntimeException was not thrown');
+        } catch (\RuntimeException $e) {
+            $this->assertStringContainsString('EntityManager was closed', $e->getMessage());
+        }
+    }
+
     // ─────────────────────────────────────────────────────────────────────────
     // DIRECT NESTING: inner transaction() inside outer transaction()
     // Simulates the email-send pattern (SummitPromoCodeService, etc.)
@@ -329,8 +450,6 @@ public function testNestedTransactionRollsBackAndRethrowsOnError(): void
     public function testDirectNestingInnerRunsAsNested(): void
     {
         $conn = Mockery::mock(Connection::class);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->once(); // only root sets isolation
         $conn->shouldReceive('close')->byDefault();
 
@@ -384,12 +503,19 @@ public function testDirectNestingInnerRunsAsNested(): void
      * Direct nesting: inner transaction throws — exception propagates to outer,
      * outer catches and the root handles the full rollback.
      * No EM reset or connection close happens during inner failure.
+     *
+     * This test verifies structural invariants only (no EM reset, no
+     * connection close). It does NOT verify that this outer-catches-and-
+     * continues pattern is safe: on a real DBAL connection, the inner
+     * rollBack() sets isRollbackOnly=true, so the root's eventual commit()
+     * would throw ConnectionException::commitFailedRollbackOnly (or the
+     * ORM's OptimisticLockException wrapping it) instead of silently
+     * succeeding - this mock does not model isRollbackOnly, so it cannot
+     * assert that. See the plan's real-MySQL verification for that proof.
      */
     public function testDirectNestingInnerErrorPropagatesWithoutDestroyingEM(): void
     {
         $conn = Mockery::mock(Connection::class);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('close')->never(); // inner must NOT close connection
 
@@ -447,8 +573,6 @@ public function testDirectNestingInnerErrorPropagatesWithoutDestroyingEM(): void
     public function testDirectNestingConnectionErrorInInnerDoesNotDestroyOuterTx(): void
     {
         $conn = Mockery::mock(Connection::class);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
 
         $txActive = false;
@@ -515,8 +639,6 @@ public function testDirectNestingConnectionErrorInInnerDoesNotDestroyOuterTx():
     public function testIndirectNestingServiceCallRunsAsNested(): void
     {
         $conn = Mockery::mock(Connection::class);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->once(); // only root
         $conn->shouldReceive('close')->byDefault();
 
@@ -571,8 +693,6 @@ public function testIndirectNestingServiceCallRunsAsNested(): void
     public function testIndirectNestingInnerFlushGeneratesIds(): void
     {
         $conn = Mockery::mock(Connection::class);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('close')->byDefault();
 
@@ -632,8 +752,6 @@ public function testIndirectNestingInnerFlushGeneratesIds(): void
     public function testIndirectNestingInnerServiceErrorDoesNotDestroyOuter(): void
     {
         $conn = Mockery::mock(Connection::class);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('close')->byDefault();
 
@@ -691,8 +809,6 @@ public function testIndirectNestingInnerServiceErrorDoesNotDestroyOuter(): void
     public function testIndirectNestingMultipleNestedCallsInSequence(): void
     {
         $conn = Mockery::mock(Connection::class);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->once(); // only root
         $conn->shouldReceive('close')->byDefault();
 
@@ -763,8 +879,6 @@ public function testRootTransactionThrowsAfterMaxRetries(): void
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('isTransactionActive')->andReturn(false, true);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->byDefault();
         $conn->shouldReceive('commit')->never();
@@ -823,8 +937,6 @@ public function testRootTransactionResetsClosedEntityManager(): void
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('isTransactionActive')->andReturn(false);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->byDefault();
         $conn->shouldReceive('commit')->byDefault();
@@ -872,8 +984,6 @@ public function testRootTransactionDiscardsPendingEntitiesAfterNonRetryableFailu
         $conn = Mockery::mock(Connection::class);
         // false: routing check picks the ROOT path; true: rollback check in the inner catch
         $conn->shouldReceive('isTransactionActive')->andReturn(false, true);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->once();
         $conn->shouldReceive('commit')->never();
@@ -902,43 +1012,6 @@ public function testRootTransactionDiscardsPendingEntitiesAfterNonRetryableFailu
         });
     }
 
-    /**
-     * When the connection's savepoint flag is OFF (outer transaction started
-     * outside DoctrineTransactionService), the nested path must log a warning
-     * surfacing the rollback-only poisoning risk, while still executing the
-     * transaction normally. Detection only - it must NOT try to enable
-     * savepoints mid-transaction (throws on DBAL 3.9).
-     */
-    public function testNestedTransactionWarnsWhenSavepointsDisabled(): void
-    {
-        [$em, $conn] = $this->buildMocks(transactionActive: true);
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(false);
-
-        $spy = new class {
-            public array $calls = [];
-            public function __call($name, $args) { $this->calls[] = [$name, $args]; }
-        };
-        $this->container->instance('log', $spy);
-        Facade::clearResolvedInstances();
-
-        $service = new DoctrineTransactionService('default');
-        $result = $service->transaction(function () {
-            return 'ok';
-        });
-
-        $this->assertSame('ok', $result, 'Nested transaction must proceed normally despite the warning');
-        $warnings = array_values(array_filter($spy->calls, fn ($c) => $c[0] === 'warning'));
-        $this->assertCount(1, $warnings, 'Exactly one warning must be logged when savepoints are off');
-        $this->assertStringContainsString('savepoints', $warnings[0][1][0]);
-
-        // Warn-once: a second nested call on the same service instance must NOT re-warn
-        $service->transaction(function () {
-            return 'ok-again';
-        });
-        $warnings = array_values(array_filter($spy->calls, fn ($c) => $c[0] === 'warning'));
-        $this->assertCount(1, $warnings, 'The savepoints warning must be emitted once per service instance, not per call');
-    }
-
     /**
      * A callback that swallowed a nested flush failure returns normally with a
      * CLOSED EntityManager (ORM closes it on any failed flush). The root must
@@ -951,8 +1024,6 @@ public function testRootTransactionFailsFastWhenCallbackClosedEntityManager(): v
         $conn = Mockery::mock(Connection::class);
         // false: routing check picks the ROOT path; true: rollback check in the inner catch
         $conn->shouldReceive('isTransactionActive')->andReturn(false, true);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->once();
         $conn->shouldReceive('commit')->never();
@@ -960,9 +1031,10 @@ public function testRootTransactionFailsFastWhenCallbackClosedEntityManager(): v
 
         $em = Mockery::mock(EntityManagerInterface::class);
         $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
-        // true: loop-top open check; false: post-callback fail-fast check (repeats
-        // false so the closed-EM resetManager branch in the outer catch fires too)
-        $em->shouldReceive('isOpen')->andReturn(true, false);
+        // true: transaction() routing check; true: loop-top open check; false: post-callback
+        // fail-fast check (repeats false so the closed-EM resetManager branch in the outer
+        // catch fires too)
+        $em->shouldReceive('isOpen')->andReturn(true, true, false);
         $em->shouldReceive('flush')->never();
         $em->shouldReceive('clear')->byDefault();
 
@@ -1024,8 +1096,6 @@ public function testRootTransactionRecoversClosedManagerWhenCallbackThrowsError(
         $conn = Mockery::mock(Connection::class);
         // false: routing check picks the ROOT path; true: rollback check in the inner catch
         $conn->shouldReceive('isTransactionActive')->andReturn(false, true);
-        $conn->shouldReceive('setNestTransactionsWithSavepoints')->byDefault();
-        $conn->shouldReceive('getNestTransactionsWithSavepoints')->andReturn(true)->byDefault();
         $conn->shouldReceive('setTransactionIsolation')->byDefault();
         $conn->shouldReceive('beginTransaction')->once();
         $conn->shouldReceive('commit')->never();
@@ -1033,8 +1103,9 @@ public function testRootTransactionRecoversClosedManagerWhenCallbackThrowsError(
 
         $em = Mockery::mock(EntityManagerInterface::class);
         $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
-        // true: loop-top check; false: closed-EM recovery check in the outer catch
-        $em->shouldReceive('isOpen')->andReturn(true, false);
+        // true: transaction() routing check; true: loop-top check; false: closed-EM
+        // recovery check in the outer catch
+        $em->shouldReceive('isOpen')->andReturn(true, true, false);
         $em->shouldReceive('flush')->never();
         $em->shouldReceive('clear')->byDefault();
 

From df45942a06e63d153a166f9145d599e6e614719c Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 15:15:12 -0300
Subject: [PATCH 04/13] test: nested-transaction rollback coverage for
 SummitOrderService, SummitService, SpeakerService, PresentationService,
 SummitPromoCodeService

Extends the isRollbackOnly-based nested-transaction rollback guarantee
(docs/plans/2026-07-10-tx-post-flush-poison-guard.md) to the remaining
outer/inner method pairs identified by investigation:

- SummitOrderService::addTickets -> createTicketsForOrder
  (missing-default-badge-type rollback, exact pair requested)
- SummitOrderService::requestRefundOrder -> requestRefundTicket
  (rolls back entire refund loop when one ticket is free)
- SummitService::processRegistrationCompaniesData -> addCompany
  (per-row isolation - the opposite of full-abort, since this method
  catches each row's transaction failure locally)
- SpeakerService::addSpeakerBySummit -> registerSummitPromoCodeByValue
  (rolls back the just-created speaker when a registration code
  collides with another speaker)
- PresentationService::submitPresentation / updatePresentationSubmission
  -> saveOrUpdatePresentation (rolls back on invalid track)
- SummitPromoCodeService::addPromoCode -> addPromoCodeTicketTypeRule
  (partial-commit: promo code survives even though the rules loop
  rolls back - two separate root transactions, not one)

New dedicated test files: SummitServiceTest.php, PresentationServiceTest.php,
SummitPromoCodeServiceTest.php, SpeakerServiceRegistrationTest.php (kept
separate from the pre-existing SpeakerServiceTest.php, whose 3 tests depend
on non-ephemeral externally-seeded summit ids that InsertSummitTestData's
unscoped DELETE FROM Summit would otherwise destroy).

SummitOrderServiceTest.php also gained disableDefaultBadgeType()/
restoreDefaultBadgeType() helpers to de-duplicate a 3x-repeated fixture
block, per xhigh code-review workflow findings applied during spec-verify.
---
 tests/PresentationServiceTest.php        | 133 ++++++++++++++++++
 tests/SpeakerServiceRegistrationTest.php |  90 ++++++++++++
 tests/SummitOrderServiceTest.php         | 166 ++++++++++++++++++++++-
 tests/SummitPromoCodeServiceTest.php     |  91 +++++++++++++
 tests/SummitServiceTest.php              |  79 +++++++++++
 5 files changed, 554 insertions(+), 5 deletions(-)
 create mode 100644 tests/PresentationServiceTest.php
 create mode 100644 tests/SpeakerServiceRegistrationTest.php
 create mode 100644 tests/SummitPromoCodeServiceTest.php
 create mode 100644 tests/SummitServiceTest.php

diff --git a/tests/PresentationServiceTest.php b/tests/PresentationServiceTest.php
new file mode 100644
index 000000000..dbec8ad8f
--- /dev/null
+++ b/tests/PresentationServiceTest.php
@@ -0,0 +1,133 @@
+<?php namespace Tests;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use App\Models\Foundation\Main\IGroup;
+use Illuminate\Support\Facades\App;
+use models\exceptions\EntityNotFoundException;
+use services\model\IPresentationService;
+
+/**
+ * Class PresentationServiceTest
+ */
+final class PresentationServiceTest extends BrowserKitTestCase
+{
+    use InsertSummitTestData;
+
+    use InsertMemberTestData;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        self::insertMemberTestData(IGroup::FoundationMembers);
+        self::$defaultMember = self::$member;
+        self::insertSummitTestData();
+        // insertSummitTestData() already links a PresentationSpeaker to self::$defaultMember.
+        // The concrete \models\oauth2\ResourceServerContext is final, so Mockery can't mock
+        // it directly for facade replacement - mock the interface instead and rebind it under
+        // the facade's accessor key.
+        $resource_server_context_mock = \Mockery::mock(\models\oauth2\IResourceServerContext::class);
+        $resource_server_context_mock->shouldReceive('getCurrentUser')->with(false)->andReturn(self::$defaultMember);
+        App::instance('resource_server_context', $resource_server_context_mock);
+    }
+
+    protected function tearDown(): void
+    {
+        self::clearSummitTestData();
+        self::clearMemberTestData();
+        parent::tearDown();
+        \Mockery::close();
+    }
+
+    /**
+     * PresentationService::submitPresentation() (PresentationService.php:259) calls the
+     * nested saveOrUpdatePresentation() (:399) with no try/catch. A nonexistent track_id
+     * throws EntityNotFoundException at :443-450, rolling back the entire submission -
+     * including the Presentation entity submitPresentation() itself already built and
+     * attached to the summit BEFORE calling saveOrUpdatePresentation() (:362-368).
+     */
+    public function testSubmitPresentationRollsBackWhenTrackNotFound()
+    {
+        $service = App::make(IPresentationService::class);
+
+        $title = 'Rollback Test Presentation ' . uniqid();
+
+        $data = [
+            'title' => $title,
+            'description' => 'this is a description',
+            'social_description' => 'this is a social description',
+            'level' => 'N/A',
+            'attendees_expected_learnt' => 'super duper',
+            'type_id' => self::$defaultPresentationType->getId(),
+            'track_id' => 999999999,
+            'attending_media' => true,
+            'selection_plan_id' => self::$default_selection_plan->getId(),
+        ];
+
+        try {
+            $service->submitPresentation(self::$summit, $data);
+            $this->fail('Expected EntityNotFoundException was not thrown');
+        } catch (EntityNotFoundException $ex) {
+        }
+
+        self::$em->clear();
+        self::$summit = self::$summit_repository->find(self::$summit->getId());
+
+        foreach (self::$summit->getEvents() as $event) {
+            $this->assertNotEquals($title, $event->getTitle());
+        }
+    }
+
+    /**
+     * PresentationService::updatePresentationSubmission() (PresentationService.php:502)
+     * also calls the shared nested saveOrUpdatePresentation() with no try/catch. Prove the
+     * update path rolls back too, leaving the target presentation's track/title unchanged.
+     */
+    public function testUpdatePresentationSubmissionRollsBackWhenTrackNotFound()
+    {
+        $service = App::make(IPresentationService::class);
+
+        $original_title = 'Original Presentation ' . uniqid();
+
+        $presentation = $service->submitPresentation(self::$summit, [
+            'title' => $original_title,
+            'description' => 'this is a description',
+            'social_description' => 'this is a social description',
+            'level' => 'N/A',
+            'attendees_expected_learnt' => 'super duper',
+            'type_id' => self::$defaultPresentationType->getId(),
+            'track_id' => self::$defaultTrack->getId(),
+            'attending_media' => true,
+            'selection_plan_id' => self::$default_selection_plan->getId(),
+        ]);
+        $presentation_id = $presentation->getId();
+        $original_track_id = $presentation->getCategoryId();
+
+        try {
+            $service->updatePresentationSubmission(self::$summit, $presentation_id, [
+                'title' => 'Updated Title Should Not Persist',
+                'type_id' => self::$defaultPresentationType->getId(),
+                'track_id' => 999999999,
+            ]);
+            $this->fail('Expected EntityNotFoundException was not thrown');
+        } catch (EntityNotFoundException $ex) {
+        }
+
+        self::$em->clear();
+
+        $reFetched = self::$em->find(\models\summit\Presentation::class, $presentation_id);
+        $this->assertNotNull($reFetched);
+        $this->assertEquals($original_title, $reFetched->getTitle());
+        $this->assertEquals($original_track_id, $reFetched->getCategoryId());
+    }
+}
diff --git a/tests/SpeakerServiceRegistrationTest.php b/tests/SpeakerServiceRegistrationTest.php
new file mode 100644
index 000000000..4d3d688e3
--- /dev/null
+++ b/tests/SpeakerServiceRegistrationTest.php
@@ -0,0 +1,90 @@
+<?php namespace Tests;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use Illuminate\Support\Facades\App;
+use models\exceptions\ValidationException;
+use models\summit\ISpeakerRepository;
+use services\model\ISpeakerService;
+
+/**
+ * Class SpeakerServiceRegistrationTest
+ *
+ * Kept separate from tests/SpeakerServiceTest.php: that class's 3 existing tests
+ * hardcode external summit ids (3397/40/3611) and deliberately keep
+ * InsertSummitTestData disabled, because insertSummitTestData() (InsertSummitTestData.php:300-310)
+ * runs unscoped DELETE FROM Summit/Company/Sponsor/etc. with no restoration path -
+ * reusing that class here would permanently wipe the data those tests depend on.
+ */
+final class SpeakerServiceRegistrationTest extends BrowserKitTestCase
+{
+    use InsertSummitTestData;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        self::insertSummitTestData();
+    }
+
+    protected function tearDown(): void
+    {
+        self::clearSummitTestData();
+        parent::tearDown();
+    }
+
+    /**
+     * SpeakerService::addSpeakerBySummit() (SpeakerService.php:300) calls addSpeaker()
+     * (:193, its own nested transaction, creates the speaker) then, if a registration_code
+     * is provided, registerSummitPromoCodeByValue() (:419, its own nested transaction) - no
+     * try/catch around either call. If the code is already assigned to a DIFFERENT speaker
+     * for the same summit (ValidationException at :439-441), the entire addSpeakerBySummit
+     * call rolls back, including the speaker just created by the first nested call.
+     */
+    public function testAddSpeakerBySummitRollsBackEntireChainWhenRegistrationCodeAlreadyAssignedToAnotherSpeaker()
+    {
+        $service = App::make(ISpeakerService::class);
+
+        $registration_code = 'REG-CODE-' . uniqid();
+
+        // speaker A registers first, claiming the code
+        $speaker_a_email = 'speaker-a-' . uniqid() . '@test.com';
+        $service->addSpeakerBySummit(self::$summit, [
+            'title' => 'Developer!',
+            'first_name' => 'Speaker',
+            'last_name' => 'A',
+            'email' => $speaker_a_email,
+            'registration_code' => $registration_code,
+        ]);
+
+        // speaker B tries to use the SAME code, already assigned to speaker A
+        $speaker_b_email = 'speaker-b-' . uniqid() . '@test.com';
+
+        try {
+            $service->addSpeakerBySummit(self::$summit, [
+                'title' => 'Developer!',
+                'first_name' => 'Speaker',
+                'last_name' => 'B',
+                'email' => $speaker_b_email,
+                'registration_code' => $registration_code,
+            ]);
+            $this->fail('Expected ValidationException was not thrown');
+        } catch (ValidationException $ex) {
+            $this->assertStringContainsString('another speaker', $ex->getMessage());
+        }
+
+        self::$em->clear();
+
+        $speaker_b = App::make(ISpeakerRepository::class)->getByEmail($speaker_b_email);
+        $this->assertNull($speaker_b);
+    }
+}
diff --git a/tests/SummitOrderServiceTest.php b/tests/SummitOrderServiceTest.php
index d02702cdb..57918eb2c 100644
--- a/tests/SummitOrderServiceTest.php
+++ b/tests/SummitOrderServiceTest.php
@@ -32,6 +32,7 @@
 use App\Services\Utils\ILockManagerService;
 use Illuminate\Support\Facades\App;
 use Illuminate\Support\Facades\Cache;
+use Illuminate\Support\Facades\Config;
 use Illuminate\Support\Facades\Queue;
 use libs\utils\ITransactionService;
 use Mockery;
@@ -423,6 +424,24 @@ private function buildTicketDataImportService(string $csv_content): ISummitOrder
         );
     }
 
+    private function disableDefaultBadgeType(): int
+    {
+        $badge_type_id = self::$default_badge_type->getId();
+        self::$default_badge_type->setIsDefault(false);
+        return $badge_type_id;
+    }
+
+    private function restoreDefaultBadgeType(int $badge_type_id): void
+    {
+        // The failed transaction's rollback already cleared self::$em, detaching the
+        // original self::$default_badge_type instance - restore via a fresh lookup so
+        // the flush actually persists the flag, instead of silently no-op'ing on a
+        // detached entity.
+        self::$default_badge_type = self::$em->find(SummitBadgeType::class, $badge_type_id);
+        self::$default_badge_type->setIsDefault(true);
+        self::$em->flush();
+    }
+
     /**
      * @param string $name
      * @param string $type
@@ -981,7 +1000,11 @@ public function testCreateOfflineOrderCommitsAcrossNestedAndSequentialRootTransa
 
         $this->assertTrue($order->isPaid());
         $this->assertEquals(1, $order->getTickets()->count());
-        $this->assertEquals($owner_email, $order->getFirstTicket()->getOwner()->getEmail());
+        $ticket = $order->getFirstTicket();
+        $this->assertNotNull($ticket);
+        $owner = $ticket->getOwner();
+        $this->assertNotNull($owner);
+        $this->assertEquals($owner_email, $owner->getEmail());
 
         self::$em->clear();
 
@@ -989,7 +1012,11 @@ public function testCreateOfflineOrderCommitsAcrossNestedAndSequentialRootTransa
         $this->assertNotNull($reFetched);
         $this->assertTrue($reFetched->isPaid());
         $this->assertEquals(1, $reFetched->getTickets()->count());
-        $this->assertEquals($owner_email, $reFetched->getFirstTicket()->getOwner()->getEmail());
+        $reFetchedTicket = $reFetched->getFirstTicket();
+        $this->assertNotNull($reFetchedTicket);
+        $reFetchedOwner = $reFetchedTicket->getOwner();
+        $this->assertNotNull($reFetchedOwner);
+        $this->assertEquals($owner_email, $reFetchedOwner->getEmail());
     }
 
     public function testCreateTicketsForOrderRollsBackEntireChainOnInvalidPromoCode()
@@ -1032,7 +1059,7 @@ public function testCreateOfflineOrderRollsBackEntireChainOnMissingDefaultBadgeT
         $service = App::make(ISummitOrderService::class);
         $attendee_repository = App::make(ISummitAttendeeRepository::class);
 
-        self::$default_badge_type->setIsDefault(false);
+        $badge_type_id = $this->disableDefaultBadgeType();
 
         $owner_email = sprintf('offline-order-badge-fail-%s@test.com', uniqid());
 
@@ -1050,8 +1077,7 @@ public function testCreateOfflineOrderRollsBackEntireChainOnMissingDefaultBadgeT
         } catch (ValidationException $ex) {
             $this->assertStringContainsString('default badge type', $ex->getMessage());
         } finally {
-            self::$default_badge_type->setIsDefault(true);
-            self::$em->flush();
+            $this->restoreDefaultBadgeType($badge_type_id);
         }
 
         self::$em->clear();
@@ -1059,4 +1085,134 @@ public function testCreateOfflineOrderRollsBackEntireChainOnMissingDefaultBadgeT
         $attendee = $attendee_repository->getBySummitAndEmail(self::$summit, $owner_email);
         $this->assertNull($attendee);
     }
+
+    /**
+     * processTicketData() runs each CSV row in its OWN independent root transaction (the
+     * summit lookup and each row's tx_service->transaction() call all commit/rollback
+     * separately - not one nested transaction spanning the whole file). Since the loop has
+     * no per-row try/catch, row 1's uncaught failure stops the foreach immediately, so row 2
+     * is never reached. This test therefore proves loop termination, NOT cross-row atomicity:
+     * because both rows fail for the SAME summit-wide reason (missing default badge type),
+     * it cannot distinguish "nothing commits because everything failed" from "an
+     * already-succeeded earlier row would also roll back if a later row failed" - the latter
+     * is FALSE (each row is an independent root transaction; a genuinely-committed earlier
+     * row survives regardless of a later row's failure). That partial-commit risk is real and
+     * separately documented in docs/plans/2026-07-10-summit-order-api-exception-coverage.md's
+     * Out of Scope section, not covered by this test.
+     */
+    public function testProcessTicketDataStopsProcessingRemainingRowsOnNestedTransactionFailure()
+    {
+        $attendee_repository = App::make(ISummitAttendeeRepository::class);
+
+        $email1 = sprintf('csv-import-row1-%s@test.com', uniqid());
+        $email2 = sprintf('csv-import-row2-%s@test.com', uniqid());
+        $ticket_type_id = self::$default_ticket_type->getId();
+
+        $csv_content = <<<CSV
+attendee_email,attendee_first_name,attendee_last_name,ticket_type_id
+{$email1},CSV,RowOne,{$ticket_type_id}
+{$email2},CSV,RowTwo,{$ticket_type_id}
+CSV;
+
+        $badge_type_id = $this->disableDefaultBadgeType();
+
+        try {
+            $service = $this->buildTicketDataImportService($csv_content);
+            $service->processTicketData(self::$summit->getId(), 'tickets.csv');
+            $this->fail('Expected ValidationException was not thrown');
+        } catch (ValidationException $ex) {
+            $this->assertStringContainsString('default badge type', $ex->getMessage());
+        } finally {
+            $this->restoreDefaultBadgeType($badge_type_id);
+        }
+
+        self::$em->clear();
+
+        $attendee1 = $attendee_repository->getBySummitAndEmail(self::$summit, $email1);
+        $attendee2 = $attendee_repository->getBySummitAndEmail(self::$summit, $email2);
+        $this->assertNull($attendee1);
+        $this->assertNull($attendee2);
+    }
+
+    public function testAddTicketsRollsBackEntireChainOnMissingDefaultBadgeType()
+    {
+        $service = App::make(ISummitOrderService::class);
+
+        $order = self::$summit->getOrders()[0];
+        $order_id = $order->getId();
+        $initial_ticket_count = $order->getTickets()->count();
+
+        $badge_type_id = $this->disableDefaultBadgeType();
+
+        try {
+            $service->addTickets(self::$summit, $order_id, [
+                'ticket_type_id' => self::$default_ticket_type->getId(),
+                'ticket_qty' => 1,
+            ]);
+            $this->fail('Expected ValidationException was not thrown');
+        } catch (ValidationException $ex) {
+            $this->assertStringContainsString('default badge type', $ex->getMessage());
+        } finally {
+            $this->restoreDefaultBadgeType($badge_type_id);
+        }
+
+        self::$em->clear();
+
+        $reFetched = self::$em->find(SummitOrder::class, $order_id);
+        $this->assertNotNull($reFetched);
+        $this->assertEquals($initial_ticket_count, $reFetched->getTickets()->count());
+    }
+
+    public function testRequestRefundOrderRollsBackEntireChainWhenOneTicketIsFree()
+    {
+        Queue::fake();
+        Config::set('registration.admin_email', 'admin@test.com');
+
+        $service = App::make(ISummitOrderService::class);
+
+        // Summit::canEmitRefundRequests() requires a future refund-request deadline;
+        // InsertSummitTestData does not set one by default.
+        self::$summit->setRegistrationAllowedRefundRequestTillDate(new \DateTime('+1 day', new \DateTimeZone('UTC')));
+
+        $order = self::$summit->getOrders()[0];
+        $order_id = $order->getId();
+        $tickets = $order->getTickets();
+        $this->assertGreaterThanOrEqual(2, $tickets->count());
+
+        $refundableTicket = $tickets[0];
+        $freeTicket = $tickets[1];
+        $refundableTicketId = $refundableTicket->getId();
+        $freeTicketId = $freeTicket->getId();
+
+        // isPaid() is a pure status flag (set by SummitOrder::setPaid() on every ticket),
+        // independent of cost - setRawCost(0.0) makes this ticket free without affecting
+        // its already-paid status, so it still enters requestRefundOrder's loop.
+        $freeTicket->setRawCost(0.0);
+
+        // InsertSummitTestData sets the order's owner via SummitOrder::setOwner() only,
+        // which does not populate Member's inverse summit_registration_orders collection;
+        // requestRefundOrder() looks the order up via that collection with no self-healing
+        // fallback (unlike updateMyOrder()), so add it explicitly.
+        self::$defaultMember->addSummitRegistrationOrder($order);
+
+        self::$em->persist($freeTicket);
+        self::$em->persist(self::$defaultMember);
+        self::$em->flush();
+
+        try {
+            $service->requestRefundOrder(self::$defaultMember, $order_id);
+            $this->fail('Expected ValidationException was not thrown');
+        } catch (ValidationException $ex) {
+            $this->assertStringContainsString('free', $ex->getMessage());
+        }
+
+        self::$em->clear();
+
+        $reFetchedRefundable = self::$em->find(SummitAttendeeTicket::class, $refundableTicketId);
+        $reFetchedFree = self::$em->find(SummitAttendeeTicket::class, $freeTicketId);
+        $this->assertNotNull($reFetchedRefundable);
+        $this->assertNotNull($reFetchedFree);
+        $this->assertEquals(0, $reFetchedRefundable->getRefundedRequests()->count());
+        $this->assertEquals(0, $reFetchedFree->getRefundedRequests()->count());
+    }
 }
diff --git a/tests/SummitPromoCodeServiceTest.php b/tests/SummitPromoCodeServiceTest.php
new file mode 100644
index 000000000..5d047deef
--- /dev/null
+++ b/tests/SummitPromoCodeServiceTest.php
@@ -0,0 +1,91 @@
+<?php namespace Tests;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use App\Models\Foundation\Summit\PromoCodes\PromoCodesConstants;
+use Illuminate\Support\Facades\App;
+use models\exceptions\EntityNotFoundException;
+use models\summit\SpeakersSummitRegistrationPromoCode;
+use services\model\ISummitPromoCodeService;
+
+/**
+ * Class SummitPromoCodeServiceTest
+ *
+ * Functional counterpart to tests/Unit/Services/SummitPromoCodeServiceDiscoveryTest.php
+ * (which covers discoverPromoCodes() with pure Mockery, no real DB). This class exercises
+ * addPromoCode() against a real database, needed to prove its two-transaction, partial-commit
+ * behavior - not expressible with mocks.
+ */
+final class SummitPromoCodeServiceTest extends BrowserKitTestCase
+{
+    use InsertSummitTestData;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        self::insertSummitTestData();
+    }
+
+    protected function tearDown(): void
+    {
+        self::clearSummitTestData();
+        parent::tearDown();
+    }
+
+    /**
+     * SummitPromoCodeService::addPromoCode() (SummitPromoCodeService.php:221) is actually TWO
+     * SEPARATE, independent root transactions, not one root with everything nested inside: the
+     * promo code itself commits in the first tx_service->transaction() call (:223-311); the
+     * ticket-type-rules loop runs in a SECOND, separate tx_service->transaction() call
+     * (:313-326) that only starts once the first has already committed. A failure inside the
+     * rules loop (nested addPromoCodeTicketTypeRule() call, EntityNotFoundException at :496-497
+     * for a nonexistent ticket_type_id) rolls back the SECOND transaction only - the
+     * already-committed promo code from the first transaction survives with zero rules applied.
+     * This test proves that partial-commit behavior explicitly, not a full rollback.
+     */
+    public function testAddPromoCodeSurvivesWhenTicketTypeRuleFails()
+    {
+        $service = App::make(ISummitPromoCodeService::class);
+
+        $code = 'TEST_PC_' . uniqid();
+
+        $data = [
+            'type' => PromoCodesConstants::SpeakerSummitRegistrationPromoCodeTypeAlternate,
+            'class_name' => SpeakersSummitRegistrationPromoCode::ClassName,
+            'code' => $code,
+            'description' => 'TEST PROMO CODE',
+            'quantity_available' => 10,
+            'allowed_ticket_types' => [],
+            'badge_features' => [],
+            'valid_since_date' => 1649108093,
+            'valid_until_date' => 1649109093,
+            'ticket_types_rules' => [
+                ['ticket_type_id' => 999999999],
+            ],
+        ];
+
+        try {
+            $service->addPromoCode(self::$summit, $data);
+            $this->fail('Expected EntityNotFoundException was not thrown');
+        } catch (EntityNotFoundException $ex) {
+            $this->assertStringContainsString('Ticket Type', $ex->getMessage());
+        }
+
+        self::$em->clear();
+        self::$summit = self::$summit_repository->find(self::$summit->getId());
+
+        $promo_code = self::$summit->getPromoCodeByCode($code);
+        $this->assertNotNull($promo_code);
+        $this->assertEquals(0, $promo_code->getAllowedTicketTypes()->count());
+    }
+}
diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php
new file mode 100644
index 000000000..f120a40a1
--- /dev/null
+++ b/tests/SummitServiceTest.php
@@ -0,0 +1,79 @@
+<?php namespace Tests;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use Illuminate\Support\Facades\App;
+use Illuminate\Support\Facades\Storage;
+use services\model\ISummitService;
+
+/**
+ * Class SummitServiceTest
+ */
+final class SummitServiceTest extends BrowserKitTestCase
+{
+    use InsertSummitTestData;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        self::insertSummitTestData();
+    }
+
+    protected function tearDown(): void
+    {
+        self::clearSummitTestData();
+        parent::tearDown();
+    }
+
+    /**
+     * SummitService::processRegistrationCompaniesData() (SummitService.php:3586) wraps each
+     * CSV row's tx_service->transaction() call in a LOCAL try/catch (:3608-3646), outside the
+     * transaction's own closure - unlike SummitOrderService::processTicketData(), which has no
+     * per-row catch at all. Prove one bad row (a company already attached to the summit,
+     * triggering addCompany()'s ValidationException at SummitService.php:3334) does NOT stop a
+     * later, valid row from being processed.
+     */
+    public function testProcessRegistrationCompaniesDataSkipsFailingRowButProcessesLaterRows()
+    {
+        Storage::fake('swift');
+
+        $service = App::make(ISummitService::class);
+        $summit_id = self::$summit->getId();
+
+        // pre-attach an existing company to the summit so the CSV row for it
+        // triggers addCompany()'s "already has a company" ValidationException
+        $existing_company = self::$companies[0];
+        $service->addCompany($summit_id, $existing_company->getId());
+
+        $new_company_name = 'New Row Company ' . uniqid();
+
+        $csv_content = <<<CSV
+name,
+{$existing_company->getName()},
+{$new_company_name},
+CSV;
+
+        $filename = 'registration_companies_isolation_test.csv';
+        Storage::disk('swift')->put("tmp/registration_companies_import/{$filename}", $csv_content);
+
+        // processRegistrationCompaniesData() catches each row's transaction failure
+        // locally, so it returns normally even though the first row fails.
+        $service->processRegistrationCompaniesData($summit_id, $filename);
+
+        self::$em->clear();
+        self::$summit = self::$summit_repository->find($summit_id);
+
+        $new_company_on_summit = self::$summit->getRegistrationCompanyByName($new_company_name);
+        $this->assertNotNull($new_company_on_summit);
+    }
+}

From a79bc9ca99af72b7a29441beedc421bdfe1dfde7 Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 15:16:04 -0300
Subject: [PATCH 05/13] test: HTTP-level exception-branch coverage for
 order/attendee ticket creation

Adds business-exception coverage for the API entry points into the
nested-transaction rollback chain (docs/plans/2026-07-10-tx-post-flush-poison-guard.md),
complementing the existing happy-path-only tests:

- OAuth2AttendeesApiTest::testAddAttendeeTicketFailsOnInvalidPromoCode
  (addAttendeeTicket -> createOfflineOrder -> createTicketsForOrder,
  invalid promo code rolls back the whole chain, ticket count unchanged)
- OAuth2SummitOrdersApiTest: un-skips testCreateSingleTicketOrder (fixed
  a 'summit_id' -> 'id' route param bug and a missing ticket_qty) and
  repurposes testCreateSingleTicketOrderNotComplete into
  testCreateSingleTicketOrderFailsOnInvalidPromoCode (order creation
  rolls back entirely on an invalid promo code, order count unchanged)

Both previously-skipped tests had a stale skip reason that no longer
matched current code behavior.
---
 tests/oauth2/OAuth2AttendeesApiTest.php    | 39 +++++++++++++++++++++
 tests/oauth2/OAuth2SummitOrdersApiTest.php | 40 +++++++++-------------
 2 files changed, 56 insertions(+), 23 deletions(-)

diff --git a/tests/oauth2/OAuth2AttendeesApiTest.php b/tests/oauth2/OAuth2AttendeesApiTest.php
index a995314be..62530ec82 100644
--- a/tests/oauth2/OAuth2AttendeesApiTest.php
+++ b/tests/oauth2/OAuth2AttendeesApiTest.php
@@ -378,6 +378,45 @@ public function testAddAttendeeTicket(){
         return $ticket;
     }
 
+    public function testAddAttendeeTicketFailsOnInvalidPromoCode(){
+        $attendee = self::$summit->getAttendeeByMember(self::$defaultMember);
+        $this->assertNotNull($attendee);
+        $attendee_id = $attendee->getId();
+        $initial_ticket_count = $attendee->getTickets()->count();
+
+        $params = [
+            'id'          => self::$summit->getId(),
+            'attendee_id' => $attendee_id,
+        ];
+
+        $data = [
+            'ticket_type_id' => self::$default_ticket_type->getId(),
+            'promo_code'     => 'DOES-NOT-EXIST-' . uniqid(),
+        ];
+
+        $response = $this->action(
+            "POST",
+            "OAuth2SummitAttendeesApiController@addAttendeeTicket",
+            $params,
+            [],
+            [],
+            [],
+            $this->getAuthHeaders(),
+            json_encode($data)
+        );
+
+        $content = $response->getContent();
+        $this->assertResponseStatus(404);
+        $decoded = json_decode($content);
+        $this->assertNotNull($decoded);
+        $this->assertStringContainsString('Promo code', $decoded->message);
+
+        self::$em->clear();
+        $reFetched = App::make(\models\summit\ISummitAttendeeRepository::class)->getById($attendee_id);
+        $this->assertNotNull($reFetched);
+        $this->assertEquals($initial_ticket_count, $reFetched->getTickets()->count());
+    }
+
     public function testDeleteAttendeeTicket(){
         $attendee = self::$summit->getAttendeeByMember(self::$defaultMember);
         $params = [
diff --git a/tests/oauth2/OAuth2SummitOrdersApiTest.php b/tests/oauth2/OAuth2SummitOrdersApiTest.php
index 447aee92c..eb3a217c0 100644
--- a/tests/oauth2/OAuth2SummitOrdersApiTest.php
+++ b/tests/oauth2/OAuth2SummitOrdersApiTest.php
@@ -785,10 +785,8 @@ public function testGetTicketByHash(){
     }
 
     public function testCreateSingleTicketOrder(){
-        $this->markTestSkipped('reserve endpoint is public (no auth.user middleware) but SagaFactory::build() requires non-null Member');
-
         $params = [
-            'summit_id' =>  self::$summit->getId()
+            'id' =>  self::$summit->getId()
         ];
 
         $data = [
@@ -796,15 +794,11 @@ public function testCreateSingleTicketOrder(){
             'owner_last_name' => 'Marcet',
             'owner_email' => 'smarcet@gmail.com',
             'ticket_type_id' => self::$ticketType->getId(),
+            'ticket_qty' => 1,
             "owner_company" => "Pumant",
             //'promo_code' => 'STAFF'
         ];
 
-        $headers = [
-            "HTTP_Authorization" => " Bearer " . $this->access_token,
-            "CONTENT_TYPE"        => "application/json"
-        ];
-
         $response = $this->action(
             "POST",
             "OAuth2SummitOrdersApiController@add",
@@ -812,7 +806,7 @@ public function testCreateSingleTicketOrder(){
             [],
             [],
             [],
-            $headers,
+            $this->getAuthHeaders(),
             json_encode($data)
         );
 
@@ -823,11 +817,11 @@ public function testCreateSingleTicketOrder(){
         return $order;
     }
 
-    public function testCreateSingleTicketOrderNotComplete(){
-        $this->markTestSkipped('reserve endpoint is public (no auth.user middleware) but SagaFactory::build() requires non-null Member');
+    public function testCreateSingleTicketOrderFailsOnInvalidPromoCode(){
+        $initial_order_count = self::$summit->getOrders()->count();
 
         $params = [
-            'summit_id' =>  self::$summit->getId()
+            'id' =>  self::$summit->getId()
         ];
 
         $data = [
@@ -835,13 +829,9 @@ public function testCreateSingleTicketOrderNotComplete(){
             'owner_last_name' => 'Marcet',
             'owner_email' => 'smarcet@gmail.com',
             'ticket_type_id' => self::$ticketType->getId(),
+            'ticket_qty' => 1,
             "owner_company" => "Pumant",
-            //'promo_code' => 'STAFF'
-        ];
-
-        $headers = [
-            "HTTP_Authorization" => " Bearer " . $this->access_token,
-            "CONTENT_TYPE"        => "application/json"
+            'promo_code' => 'DOES-NOT-EXIST-' . uniqid(),
         ];
 
         $response = $this->action(
@@ -851,15 +841,19 @@ public function testCreateSingleTicketOrderNotComplete(){
             [],
             [],
             [],
-            $headers,
+            $this->getAuthHeaders(),
             json_encode($data)
         );
 
         $content = $response->getContent();
-        $this->assertResponseStatus(201);
-        $order = json_decode($content);
-        $this->assertTrue(!is_null($order));
-        return $order;
+        $this->assertResponseStatus(404);
+        $decoded = json_decode($content);
+        $this->assertNotNull($decoded);
+        $this->assertStringContainsString('Promo code', $decoded->message);
+
+        self::$em->clear();
+        self::$summit = self::$summit_repository->find(self::$summit->getId());
+        $this->assertEquals($initial_order_count, self::$summit->getOrders()->count());
     }
 
     /**

From 5b140f76430148c6268ce3f1b68fbd0c6e3195f0 Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 15:32:47 -0300
Subject: [PATCH 06/13] docs(adr): nested transaction rollback safety decision
 record

Documents the 3-iteration decision history behind DoctrineTransactionService's
current no-savepoints design: the initial DBAL-savepoints approach, why it was
discarded (Doctrine's UnitOfWork has no concept of savepoints, so a
ROLLBACK TO SAVEPOINT at the DB level desyncs silently from the in-memory
identity map/changesets), and the final native isRollbackOnly-propagation
design. Lists every service/method pair covered by the resulting test suite,
plus 11 additional outer/inner pairs found in a follow-up audit that are not
yet covered, as future work.
---
 adr/003-nested-transaction-rollback-safety.md | 220 ++++++++++++++++++
 1 file changed, 220 insertions(+)
 create mode 100644 adr/003-nested-transaction-rollback-safety.md

diff --git a/adr/003-nested-transaction-rollback-safety.md b/adr/003-nested-transaction-rollback-safety.md
new file mode 100644
index 000000000..6703609c9
--- /dev/null
+++ b/adr/003-nested-transaction-rollback-safety.md
@@ -0,0 +1,220 @@
+# ADR-003: Nested Transaction Rollback Safety — Remove DBAL Savepoints, Rely on Native `isRollbackOnly`
+
+- **Status:** Accepted
+- **Date:** 2026-07-10
+- **Branch:** `hotfix/doctrine-tx-manager`
+- **Component:** `app/Services/Utils/DoctrineTransactionService.php`
+
+## Context
+
+`DoctrineTransactionService::transaction()` is the single choke point every service in this
+codebase uses to wrap business logic in a DB transaction. It detects whether a transaction is
+already active on the connection and routes to one of two paths:
+
+- **Root** (`runRootTransaction`) — owns the connection lifecycle, sets the isolation level,
+  retries on transient connection errors, flushes the `EntityManager`, and issues the real
+  `COMMIT`.
+- **Nested** (`runNestedTransaction`) — used when a service method wrapped in its own
+  `tx_service->transaction()` call is invoked from inside another service method that is
+  *also* wrapped in its own `tx_service->transaction()` call (an "outer/inner" pair). It
+  flushes so auto-generated IDs are available to the caller, but does not retry, reset the
+  `EntityManager`, or commit at the DB level — that's the root's job.
+
+The question this ADR answers: **when the inner (nested) transaction fails, what happens to
+the outer (root) transaction's eventual commit?** Three iterations were needed to get this
+right.
+
+## Decision Timeline
+
+### Iteration 1 (initial approach) — DBAL savepoints, discarded
+
+Commit `d88de2ce3` introduced the root/nested split above and, alongside it, called
+`$conn->setNestTransactionsWithSavepoints(true)` on the root transaction. The intent: when a
+nested transaction's `rollBack()` runs on a savepoints-enabled connection, DBAL issues
+`ROLLBACK TO SAVEPOINT` instead of rolling back the whole DB transaction — undoing only the
+inner work while leaving the outer transaction free to continue and commit its own writes.
+This was meant to make a "catch the inner failure, log it, and continue" pattern safe.
+
+**Why it was discarded.** `ROLLBACK TO SAVEPOINT` only undoes the *database's* row-level
+changes for that inner transaction. It does nothing to Doctrine ORM's `UnitOfWork` — the
+in-memory identity map of managed entities and pending changesets. Doctrine's `UnitOfWork` has
+**no concept of savepoints at all**: it does not know a savepoint rollback happened, does not
+discard the entities the inner transaction created/mutated, and does not roll back their
+in-memory state. After a savepoint rollback, the outer transaction's `EntityManager` can still
+hold references to entities that:
+
+- Doctrine's identity map believes are managed/persisted, when the DB has actually discarded
+  their rows, or
+- carry partially-applied field mutations from the inner transaction that never made it to
+  disk.
+
+The next `flush()` — root's own, or a *different* nested transaction reusing the same
+`EntityManager` — has no reliable way to know which parts of its unit of work are still
+backed by real rows and which were silently discarded at the DB level by a savepoint
+rollback it never modeled. That is a structural, silent desync between the DB and the object
+graph — exactly the shape of bug that produces the worst kind of data-loss report ("the API
+said success, but the row isn't there" or the inverse, "a stale in-memory object gets
+re-persisted"). It is not fixable by adding more code on top of savepoints; the ORM layer
+Doctrine ships simply doesn't support partial-rollback recovery.
+
+Two follow-up commits tried to hage this safer without removing savepoints:
+
+- `24c1077db` ("harden root/nested split against phantom writes and masked errors") — added
+  `em->clear()` on root failure so a failed callback's pending changes can't leak into the
+  next transaction on the same `EntityManager`; added a fail-fast check when a swallowed
+  nested flush failure left the `EntityManager` closed; guarded rollback failures from masking
+  the original exception; started warning once when savepoints were unexpectedly disabled on
+  the connection (e.g., an outer transaction started outside this service).
+
+These closed several real bugs (masked exceptions, phantom writes leaking across retries,
+opaque `EntityManagerClosed` errors two levels deep) but did not — and structurally could
+not — close the core UnitOfWork/savepoint desync, because the desync is a property of how
+Doctrine ORM's UnitOfWork is built, not a bug in this service's bookkeeping.
+
+### Iteration 2 (final approach) — no savepoints, native `isRollbackOnly` propagation
+
+Commit `248f8e453` deleted `setNestTransactionsWithSavepoints(true)` from the root transaction
+entirely (and the now-pointless "warn when savepoints disabled" mechanism from the nested
+path). Verified directly against the vendored DBAL 3.9.4 source
+(`vendor/doctrine/dbal/src/Connection.php`): with savepoints off, a nested `rollBack()` (any
+nesting level > 1) does exactly this — no SQL, no partial-rollback semantics to desync from:
+
+```php
+$this->isRollbackOnly = true;
+--$this->transactionNestingLevel;
+```
+
+And `commit()`, at **any** nesting level, checks this flag **first**, before anything else:
+
+```php
+if ($this->isRollbackOnly) {
+    throw ConnectionException::commitFailedRollbackOnly();
+}
+```
+
+DBAL already provides "one nested failure poisons the whole chain, unconditionally" natively
+— the savepoints flag was the only thing suppressing that native protection in favor of a
+partial-recovery mechanism the ORM layer cannot safely support. Once the flag is gone, **any**
+subsequent `commit()` call anywhere in the chain — nested, root, or Doctrine ORM's own
+internal per-`flush()` commit — fails immediately and loudly. A caught-and-swallowed nested
+failure can no longer end in a silent, successful root commit; it is structurally impossible,
+not merely tested-for.
+
+**Empirical confirmation** (against a real local MySQL instance, not mocks — Mockery cannot
+model DBAL's internal `isRollbackOnly` flag): a 3-level nesting scenario where the middle
+level internally catches the deepest level's business-exception failure and returns normally
+was re-run before and after this change. Before: the old savepoints-based code reached a
+"successful" root commit with the deepest level's writes silently missing. After: the root's
+own commit throws (`Doctrine\ORM\OptimisticLockException('Commit failed')`, wrapping DBAL's
+`ConnectionException::commitFailedRollbackOnly`) and zero rows are ever durably committed —
+the entire operation succeeds or fails as one atomic unit.
+
+**Trade-off accepted.** This makes "catch a nested failure and continue in the same still-open
+outer transaction" universally unsafe, for *any* nested failure — not just post-flush ones (the
+pre-fix docblock's "safe only for pre-flush errors" carve-out no longer applies either, since
+DBAL sets `isRollbackOnly` unconditionally regardless of whether the nested transaction ever
+flushed). This was checked against real call sites before accepting the trade-off: an
+automated code review flagged `RegistrationIngestionService::ingestExternalAttendee()` as a
+call site that "currently works" with this catch-and-continue pattern and would supposedly
+regress. An empirical A/B test (both the true pre-fix `origin/main` code and this branch's new
+code, same pattern, same MySQL instance) showed **neither version ever actually made this
+pattern safe** — the old code lost the same rows via a double-`EntityManager`-reset/dangling-
+reference cascade instead of a single clean exception. The new code's failure mode is more
+diagnosable, not a regression. That specific site's "race condition" recovery comment has
+never protected against the race in either version; it is logged as a separate, independent
+pre-existing bug, out of scope for this hotfix.
+
+## Consequences
+
+- **Positive:** A nested transaction failure can never be silently absorbed into a successful
+  outer commit. This is now a structural DBAL guarantee, not something every call site has to
+  reason about individually.
+- **Positive:** Diagnosability improved — failures now surface as a single
+  `OptimisticLockException`/`ConnectionException::commitFailedRollbackOnly` instead of a
+  cascade of `EntityManagerClosed` / dangling-reference errors from double-resetting the
+  registry.
+- **Negative / accepted trade-off:** Any existing "log the inner failure and continue" pattern
+  built on the old savepoints behavior is now guaranteed to abort the entire outer operation
+  instead. Verified this was never actually a *working* safety net to begin with (see the A/B
+  test above), so nothing that previously worked correctly was broken.
+- **Negative / accepted trade-off:** Per-row batch operations (CSV imports, bulk approvals,
+  etc.) that call an inner-transaction-wrapped method per item now abort the *entire* batch on
+  the first item's failure, unless the call site wraps that specific call in its own local
+  `try/catch` **outside** the inner `transaction()` closure (i.e., after that item's transaction
+  has already fully committed or rolled back — see `SummitService::processRegistrationCompaniesData`
+  below for the one call site in this codebase that already does this correctly).
+- **Out of scope, tracked separately:** `OptimisticLockException` misclassified as
+  non-retryable (`shouldReconnect()` never unwraps `getPrevious()` to check if the underlying
+  cause was a transient connection blip) — a pre-existing gap this fix reduces one trigger for
+  but does not close.
+
+## Test Coverage Added
+
+Two things were proven for every pair below, empirically (real local MySQL, not mocks — DBAL's
+`isRollbackOnly` propagation cannot be modeled by Mockery): (a) the mechanism itself
+(`DoctrineTransactionService`'s root/nested split honors the new no-savepoints contract), and
+(b) that each specific outer/inner method pair in this codebase actually exhibits the
+guaranteed behavior end-to-end.
+
+### `DoctrineTransactionService` itself (unit-level, mocked)
+
+| File | What's covered |
+|---|---|
+| `tests/Unit/Services/DoctrineTransactionServiceTest.php` | 24 tests: root/nested routing, savepoints never enabled/queried, fail-fast on a closed `EntityManager`, rollback failures never masking the original exception, `\Error` handled alongside `\Exception`, `em->clear()` on root failure. |
+
+### Business-service outer/inner pairs (functional, real DB)
+
+| Outer service → method | Inner method | Test file | Shape proven |
+|---|---|---|---|
+| `SummitOrderService::createOfflineOrder` | `createTicketsForOrder` | `tests/SummitOrderServiceTest.php` | Full rollback — invalid promo code |
+| `SummitOrderService::addTickets` | `createTicketsForOrder` | `tests/SummitOrderServiceTest.php` | Full rollback — missing default badge type |
+| `SummitOrderService::processTicketData` | `createOfflineOrder` | `tests/SummitOrderServiceTest.php` | Full rollback per CSV row (loop terminates on first failure, not cross-row atomicity) |
+| `SummitOrderService::requestRefundOrder` | `requestRefundTicket` | `tests/SummitOrderServiceTest.php` | Full rollback — one free ticket in the loop aborts all refund requests |
+| `SummitService::processRegistrationCompaniesData` | `addCompany` | `tests/SummitServiceTest.php` | **Per-row isolation, not full rollback** — this call site has its own local `try/catch` outside the inner transaction's closure, so one bad row does not block later rows |
+| `SpeakerService::addSpeakerBySummit` | `addSpeaker` + `registerSummitPromoCodeByValue` | `tests/SpeakerServiceRegistrationTest.php` | Full rollback — a registration code already claimed by another speaker undoes the just-created speaker too |
+| `PresentationService::submitPresentation` | `saveOrUpdatePresentation` | `tests/PresentationServiceTest.php` | Full rollback — nonexistent track |
+| `PresentationService::updatePresentationSubmission` | `saveOrUpdatePresentation` | `tests/PresentationServiceTest.php` | Full rollback — nonexistent track, update never partially applies |
+| `SummitPromoCodeService::addPromoCode` | `addPromoCodeTicketTypeRule` | `tests/SummitPromoCodeServiceTest.php` | **Partial commit, not full rollback** — this is actually two separate, sequential root transactions; the promo code from the first survives even when the second (ticket-type-rules) transaction fails |
+
+### API/HTTP-level exception-branch coverage
+
+| Endpoint | Test file | What's covered |
+|---|---|---|
+| `POST .../attendees/{id}/tickets` (`addAttendeeTicket`) | `tests/oauth2/OAuth2AttendeesApiTest.php` (`testAddAttendeeTicketFailsOnInvalidPromoCode`) | Invalid promo code rolls back the whole `createOfflineOrder`→`createTicketsForOrder` chain at the HTTP boundary |
+| `POST .../orders` (`add`) | `tests/oauth2/OAuth2SummitOrdersApiTest.php` (`testCreateSingleTicketOrder`, `testCreateSingleTicketOrderFailsOnInvalidPromoCode`) | Re-enabled two previously `markTestSkipped()` tests whose skip reason no longer matched current code; proves order creation and its invalid-promo-code rollback at the HTTP boundary |
+
+## Known Gaps / Future Work
+
+A follow-up codebase-wide search (same outer/inner shape: method A wrapped in its own
+`tx_service->transaction()` calling, with no local `try/catch` around the call, method B which
+is *also* wrapped in its own separate `tx_service->transaction()`) found **11 additional
+pairs, across 8 more services, that are NOT yet covered by any test** proving the new
+rollback contract:
+
+| # | Outer → Inner | Realistic trigger |
+|---|---|---|
+| 1 | `SelectionPlanOrderExtraQuestionTypeService::updateExtraQuestionBySelectionPlan` → `updateExtraQuestion` | Duplicate question name/label for the selection plan |
+| 2 | `SpeakerService::updateSpeakerBySummit` → `updateSpeaker` | Member already assigned to another speaker |
+| 3 | `SpeakerService::updateSpeakerBySummit` → `registerSummitPromoCodeByValue` | Promo code already redeemed/assigned to another speaker |
+| 4 | `SponsorUserSyncService::addSponsorUserToGroup` → `SummitSponsorService::addSponsorUser` | Member already a sponsor-user on a date-overlapping summit |
+| 5 | `SummitRSVPInvitationService::acceptInvitationBySummitEventAndToken` → `SummitRSVPService::rsvpEvent` | Attendee has no valid tickets / duplicate RSVP |
+| 6 | `SummitRegistrationInvitationService::add`/`update` → `TagService::addTag` | Duplicate tag race |
+| 7 | `SummitSubmissionInvitationService::add`/`update` → `TagService::addTag` | Same as #6 |
+| 8 | `SummitScheduleSettingsService::seedDefaults` (loop) → `add` | A later default's key already exists — rolls back earlier successfully-added defaults in the same loop too |
+| 9 | `SummitSelectedPresentationListService::getTeamSelectionList`/`getIndividualSelectionList`/`assignPresentationToMyIndividualList` → `createTeamSelectionList`/`createIndividualSelectionList` | TOCTOU re-check failure (`AuthzException`/`EntityNotFoundException`) |
+| 10 | `SummitService::unPublishEvents` (batch loop) → `unPublishEvent` | One bad event id in a batch undoes the whole batch |
+| 11 | `SummitService::updateAndPublishEvents` (batch loop) → `publishEvent` | Validation/entity-not-found failure on one event in a batch undoes the whole batch |
+
+Several other call sites share the same *family* (calling a nested-transaction-wrapped method)
+but are already guarded by a local `try/catch` outside the inner closure — the same safe shape
+as `SummitService::processRegistrationCompaniesData` above, not a rollback risk:
+`SummitRegistrationInvitationService`/`SummitSubmissionInvitationService`'s
+`setInvitationMember`/`setInvitationSpeaker` → `MemberService::registerExternalUser` /
+`SpeakerService::addSpeaker`; `SpeakerService::sendEmails` → `generateSpeakerAssistance`;
+`ScheduleService::publishAll` → `SummitService::publishEvent`; `SummitService::processEventData`
+(per-CSV-row) → `SpeakerService::addSpeaker`/`updateSpeaker`.
+
+None of the 11 gaps above are fixed or tested by this ADR's work — they are recorded here as
+the next candidates for the same test-coverage treatment applied to the 9 pairs already closed
+(see `docs/plans/2026-07-10-nested-tx-other-services-coverage.md` and
+`docs/plans/2026-07-10-summit-order-api-exception-coverage.md`).

From 9436246dd0a3df45ca49916dba60bb5211a64f99 Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 17:29:56 -0300
Subject: [PATCH 07/13] test: nested-transaction rollback coverage for
 remaining ADR-003 gaps

Implements the 8 testable outer/inner nested-transaction pairs listed as
Known Gaps in adr/003-nested-transaction-rollback-safety.md, each proving
DoctrineTransactionService's isRollbackOnly-based rollback contract by
showing an inner nested transaction's already-committed write gets undone
by a later failure in the same outer call:

- SelectionPlanOrderExtraQuestionTypeService::updateExtraQuestionBySelectionPlan
  -> updateExtraQuestion (new SelectionPlanOrderExtraQuestionTypeServiceTest.php)
- SpeakerService::updateSpeakerBySummit -> registerSummitPromoCodeByValue
  (tests/SpeakerServiceRegistrationTest.php)
- SponsorUserSyncService::addSponsorUserToGroup ->
  SummitSponsorService::addSponsorUser (tests/Unit/Services/SponsorUserPermissionTrackingTest.php)
- SummitScheduleSettingsService::seedDefaults -> add (new
  SummitScheduleSettingsServiceTest.php)
- SummitSelectedPresentationListService::assignPresentationToMyIndividualList
  -> createIndividualSelectionList (new SummitSelectedPresentationListServiceTest.php)
- SummitService::unPublishEvents -> unPublishEvent and
  updateAndPublishEvents -> updateEvent (tests/SummitServiceTest.php)

3 of the original 11 ADR-listed pairs were excluded after verifying against
real code that no committed-then-rolled-back proof is reachable through
those specific call sites (documented in the plan's Out of Scope section):
TagService::addTag's duplicate check (outer's pre-check already uses the
same normalized comparison), SummitRSVPInvitationService's rsvpEvent call
(no write before or reachable after the nested call), and SpeakerService's
member_id-collision trigger (redundant with the registration_code case
already covering the same production class).

Post-review fix: corrected a mechanism misattribution in the
updateAndPublishEvents test (the exception actually comes from
updateEvent's unconditional location check, not publishEvent's gated one,
since they share the same payload and updateEvent runs first) plus 3
test-duplication cleanups.
---
 ...nPlanOrderExtraQuestionTypeServiceTest.php |  76 +++++++++
 tests/SpeakerServiceRegistrationTest.php      |  67 +++++++-
 tests/SummitScheduleSettingsServiceTest.php   |  72 ++++++++
 ...mitSelectedPresentationListServiceTest.php |  94 +++++++++++
 tests/SummitServiceTest.php                   | 159 ++++++++++++++++++
 .../SponsorUserPermissionTrackingTest.php     |  54 +++++-
 6 files changed, 507 insertions(+), 15 deletions(-)
 create mode 100644 tests/SelectionPlanOrderExtraQuestionTypeServiceTest.php
 create mode 100644 tests/SummitScheduleSettingsServiceTest.php
 create mode 100644 tests/SummitSelectedPresentationListServiceTest.php

diff --git a/tests/SelectionPlanOrderExtraQuestionTypeServiceTest.php b/tests/SelectionPlanOrderExtraQuestionTypeServiceTest.php
new file mode 100644
index 000000000..c9b6463a7
--- /dev/null
+++ b/tests/SelectionPlanOrderExtraQuestionTypeServiceTest.php
@@ -0,0 +1,76 @@
+<?php namespace Tests;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use App\Models\Foundation\ExtraQuestions\ExtraQuestionTypeConstants;
+use App\Services\Model\ISelectionPlanExtraQuestionTypeService;
+use Illuminate\Support\Facades\App;
+use models\exceptions\EntityNotFoundException;
+
+/**
+ * Class SelectionPlanOrderExtraQuestionTypeServiceTest
+ */
+final class SelectionPlanOrderExtraQuestionTypeServiceTest extends BrowserKitTestCase
+{
+    use InsertSummitTestData;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        self::insertSummitTestData();
+    }
+
+    protected function tearDown(): void
+    {
+        self::clearSummitTestData();
+        parent::tearDown();
+    }
+
+    /**
+     * SelectionPlanOrderExtraQuestionTypeService::updateExtraQuestionBySelectionPlan()
+     * (SelectionPlanOrderExtraQuestionTypeService.php:117) calls the nested updateExtraQuestion()
+     * (:89) with no try/catch. updateExtraQuestion() commits its label change BEFORE the outer's
+     * own assignment check runs - a question added to the summit only (via addExtraQuestion(),
+     * which never assigns it to a selection plan) but never assigned to the target selection plan
+     * makes the outer's getAssignedExtraQuestion() check fail AFTER the inner already committed,
+     * rolling back that already-committed label change too.
+     */
+    public function testUpdateExtraQuestionBySelectionPlanRollsBackAlreadyCommittedLabelWhenNotAssignedToPlan()
+    {
+        $service = App::make(ISelectionPlanExtraQuestionTypeService::class);
+
+        $original_label = 'Original Label ' . uniqid();
+
+        $question = $service->addExtraQuestion(self::$summit, [
+            'name' => 'Q_' . uniqid(),
+            'label' => $original_label,
+            'type' => ExtraQuestionTypeConstants::TextQuestionType,
+        ]);
+        $question_id = $question->getId();
+
+        try {
+            $service->updateExtraQuestionBySelectionPlan(self::$default_selection_plan, $question_id, [
+                'label' => 'Updated Label ' . uniqid(),
+            ]);
+            $this->fail('Expected EntityNotFoundException was not thrown');
+        } catch (EntityNotFoundException $ex) {
+            $this->assertStringContainsString('does not belongs to selection plan', $ex->getMessage());
+        }
+
+        self::$em->clear();
+        self::$summit = self::$summit_repository->find(self::$summit->getId());
+        $reFetched = self::$summit->getSelectionPlanExtraQuestionById($question_id);
+        $this->assertNotNull($reFetched);
+        $this->assertEquals($original_label, $reFetched->getLabel());
+    }
+}
diff --git a/tests/SpeakerServiceRegistrationTest.php b/tests/SpeakerServiceRegistrationTest.php
index 4d3d688e3..11c989dfb 100644
--- a/tests/SpeakerServiceRegistrationTest.php
+++ b/tests/SpeakerServiceRegistrationTest.php
@@ -15,6 +15,7 @@
 use Illuminate\Support\Facades\App;
 use models\exceptions\ValidationException;
 use models\summit\ISpeakerRepository;
+use models\summit\PresentationSpeaker;
 use services\model\ISpeakerService;
 
 /**
@@ -42,6 +43,18 @@ protected function tearDown(): void
         parent::tearDown();
     }
 
+    private function registerSpeakerAClaimingCode(string $registration_code): PresentationSpeaker
+    {
+        $speaker_a_email = 'speaker-a-' . uniqid() . '@test.com';
+        return App::make(ISpeakerService::class)->addSpeakerBySummit(self::$summit, [
+            'title' => 'Developer!',
+            'first_name' => 'Speaker',
+            'last_name' => 'A',
+            'email' => $speaker_a_email,
+            'registration_code' => $registration_code,
+        ]);
+    }
+
     /**
      * SpeakerService::addSpeakerBySummit() (SpeakerService.php:300) calls addSpeaker()
      * (:193, its own nested transaction, creates the speaker) then, if a registration_code
@@ -57,14 +70,7 @@ public function testAddSpeakerBySummitRollsBackEntireChainWhenRegistrationCodeAl
         $registration_code = 'REG-CODE-' . uniqid();
 
         // speaker A registers first, claiming the code
-        $speaker_a_email = 'speaker-a-' . uniqid() . '@test.com';
-        $service->addSpeakerBySummit(self::$summit, [
-            'title' => 'Developer!',
-            'first_name' => 'Speaker',
-            'last_name' => 'A',
-            'email' => $speaker_a_email,
-            'registration_code' => $registration_code,
-        ]);
+        $this->registerSpeakerAClaimingCode($registration_code);
 
         // speaker B tries to use the SAME code, already assigned to speaker A
         $speaker_b_email = 'speaker-b-' . uniqid() . '@test.com';
@@ -87,4 +93,49 @@ public function testAddSpeakerBySummitRollsBackEntireChainWhenRegistrationCodeAl
         $speaker_b = App::make(ISpeakerRepository::class)->getByEmail($speaker_b_email);
         $this->assertNull($speaker_b);
     }
+
+    /**
+     * SpeakerService::updateSpeakerBySummit() (SpeakerService.php:364) calls updateSpeaker()
+     * (:368, its own nested transaction) FIRST, then - only if a registration_code is provided -
+     * registerSummitPromoCodeByValue() (:382, its own nested transaction) - no try/catch around
+     * either call. updateSpeaker() commits a real title change; if the registration code is
+     * already assigned to a DIFFERENT speaker (ValidationException at :439-441), the entire
+     * updateSpeakerBySummit call rolls back, including the already-committed title change.
+     */
+    public function testUpdateSpeakerBySummitRollsBackAlreadyCommittedTitleWhenRegistrationCodeAlreadyAssignedToAnotherSpeaker()
+    {
+        $service = App::make(ISpeakerService::class);
+
+        $registration_code = 'REG-CODE-' . uniqid();
+
+        // speaker A claims the code
+        $this->registerSpeakerAClaimingCode($registration_code);
+
+        // speaker B has no registration code yet
+        $original_title = 'Original Title ' . uniqid();
+        $speaker_b_email = 'speaker-b-' . uniqid() . '@test.com';
+        $speaker_b = $service->addSpeakerBySummit(self::$summit, [
+            'title' => $original_title,
+            'first_name' => 'Speaker',
+            'last_name' => 'B',
+            'email' => $speaker_b_email,
+        ]);
+        $speaker_b_id = $speaker_b->getId();
+
+        try {
+            $service->updateSpeakerBySummit(self::$summit, $speaker_b, [
+                'title' => 'Updated Title Should Not Persist',
+                'registration_code' => $registration_code,
+            ]);
+            $this->fail('Expected ValidationException was not thrown');
+        } catch (ValidationException $ex) {
+            $this->assertStringContainsString('another speaker', $ex->getMessage());
+        }
+
+        self::$em->clear();
+
+        $reFetched = self::$em->find(PresentationSpeaker::class, $speaker_b_id);
+        $this->assertNotNull($reFetched);
+        $this->assertEquals($original_title, $reFetched->getTitle());
+    }
 }
diff --git a/tests/SummitScheduleSettingsServiceTest.php b/tests/SummitScheduleSettingsServiceTest.php
new file mode 100644
index 000000000..d738e6836
--- /dev/null
+++ b/tests/SummitScheduleSettingsServiceTest.php
@@ -0,0 +1,72 @@
+<?php namespace Tests;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use App\Services\Model\ISummitScheduleSettingsService;
+use Illuminate\Support\Facades\App;
+use models\exceptions\ValidationException;
+
+/**
+ * Class SummitScheduleSettingsServiceTest
+ */
+final class SummitScheduleSettingsServiceTest extends BrowserKitTestCase
+{
+    use InsertSummitTestData;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        self::insertSummitTestData();
+    }
+
+    protected function tearDown(): void
+    {
+        self::clearSummitTestData();
+        parent::tearDown();
+    }
+
+    /**
+     * SummitScheduleSettingsService::seedDefaults() (SummitScheduleSettingsService.php:141)
+     * loops calling add() (:60, its own nested transaction) for 'schedule-main' then
+     * 'my-schedule-main' - no try/catch. Summit::addScheduleSetting() throws
+     * ValidationException("Key %s already exists") if a config with that key already exists
+     * (Summit.php:6433-6439). Pre-seeding 'my-schedule-main' makes the loop's FIRST iteration
+     * ('schedule-main') commit successfully, then the SECOND ('my-schedule-main') fails -
+     * rolling back the whole seedDefaults() call, including the first, already-committed config.
+     */
+    public function testSeedDefaultsRollsBackAlreadyCommittedScheduleMainWhenMyScheduleMainKeyAlreadyExists()
+    {
+        $service = App::make(ISummitScheduleSettingsService::class);
+
+        $service->add(self::$summit, ['key' => 'my-schedule-main']);
+
+        try {
+            $service->seedDefaults(self::$summit);
+            $this->fail('Expected ValidationException was not thrown');
+        } catch (ValidationException $ex) {
+            $this->assertStringContainsString('my-schedule-main', $ex->getMessage());
+            $this->assertStringContainsString('already exists', $ex->getMessage());
+        }
+
+        self::$em->clear();
+        self::$summit = self::$summit_repository->find(self::$summit->getId());
+
+        $has_schedule_main = false;
+        foreach (self::$summit->getScheduleSettings() as $config) {
+            if ($config->getKey() === 'schedule-main') {
+                $has_schedule_main = true;
+            }
+        }
+        $this->assertFalse($has_schedule_main);
+    }
+}
diff --git a/tests/SummitSelectedPresentationListServiceTest.php b/tests/SummitSelectedPresentationListServiceTest.php
new file mode 100644
index 000000000..99c384b32
--- /dev/null
+++ b/tests/SummitSelectedPresentationListServiceTest.php
@@ -0,0 +1,94 @@
+<?php namespace Tests;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use App\Models\Foundation\Main\IGroup;
+use App\Services\Model\ISummitSelectedPresentationListService;
+use Illuminate\Support\Facades\App;
+use models\exceptions\EntityNotFoundException;
+use models\summit\SummitSelectedPresentation;
+
+/**
+ * Class SummitSelectedPresentationListServiceTest
+ */
+final class SummitSelectedPresentationListServiceTest extends BrowserKitTestCase
+{
+    use InsertSummitTestData;
+    use InsertMemberTestData;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        self::insertMemberTestData(IGroup::SuperAdmins);
+        self::$defaultMember = self::$member;
+        self::insertSummitTestData();
+
+        // Summit::isTrackChair() short-circuits true via Member::isAdmin() for a
+        // SuperAdmins member, avoiding the need to wire a real SummitTrackChair entity.
+        // Unlike PresentationService (which reads the current user via the
+        // \App\Facades\ResourceServerContext static facade), this service takes
+        // IResourceServerContext as a plain constructor dependency - bind the
+        // interface directly in the container instead of rebinding the facade accessor.
+        $resource_server_context_mock = \Mockery::mock(\models\oauth2\IResourceServerContext::class);
+        $resource_server_context_mock->shouldReceive('getCurrentUser')->andReturn(self::$defaultMember);
+        App::instance(\models\oauth2\IResourceServerContext::class, $resource_server_context_mock);
+    }
+
+    protected function tearDown(): void
+    {
+        self::clearSummitTestData();
+        self::clearMemberTestData();
+        parent::tearDown();
+        \Mockery::close();
+    }
+
+    /**
+     * SummitSelectedPresentationListService::assignPresentationToMyIndividualList()
+     * (SummitSelectedPresentationListService.php:379) calls createIndividualSelectionList()
+     * (:443, its own nested transaction) only when no individual list exists yet for the
+     * current member - no try/catch. That call commits a real, new
+     * SummitSelectedPresentationList. AFTER it returns, the outer's own presentation lookup
+     * (:449-455) throws EntityNotFoundException for a nonexistent presentation_id - rolling
+     * back the entire call, including the just-committed individual selection list.
+     */
+    public function testAssignPresentationToMyIndividualListRollsBackAlreadyCommittedListWhenPresentationNotFound()
+    {
+        $service = App::make(ISummitSelectedPresentationListService::class);
+
+        try {
+            $service->assignPresentationToMyIndividualList(
+                self::$summit,
+                self::$default_selection_plan->getId(),
+                self::$defaultTrack->getId(),
+                SummitSelectedPresentation::CollectionMaybe,
+                999999999
+            );
+            $this->fail('Expected EntityNotFoundException was not thrown');
+        } catch (EntityNotFoundException $ex) {
+            $this->assertStringContainsString('999999999', $ex->getMessage());
+        }
+
+        self::$em->clear();
+        self::$summit = self::$summit_repository->find(self::$summit->getId());
+        $selectionPlan = self::$em->find(\App\Models\Foundation\Summit\SelectionPlan::class, self::$default_selection_plan->getId());
+        $category = self::$summit->getPresentationCategory(self::$defaultTrack->getId());
+        $member = self::$member_repository->find(self::$defaultMember->getId());
+
+        $list = $selectionPlan->getSelectionListByTrackAndTypeAndOwner(
+            $category,
+            \models\summit\SummitSelectedPresentationList::Individual,
+            $member
+        );
+        $this->assertNull($list);
+    }
+}
diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php
index f120a40a1..8be006f44 100644
--- a/tests/SummitServiceTest.php
+++ b/tests/SummitServiceTest.php
@@ -14,6 +14,11 @@
 
 use Illuminate\Support\Facades\App;
 use Illuminate\Support\Facades\Storage;
+use models\exceptions\EntityNotFoundException;
+use models\summit\ISummitEventType;
+use models\summit\SummitEvent;
+use models\summit\SummitEventType;
+use App\Models\Foundation\Summit\Events\SummitEventTypeConstants;
 use services\model\ISummitService;
 
 /**
@@ -22,17 +27,58 @@
 final class SummitServiceTest extends BrowserKitTestCase
 {
     use InsertSummitTestData;
+    use InsertMemberTestData;
 
     protected function setUp(): void
     {
         parent::setUp();
+        self::insertMemberTestData(\App\Models\Foundation\Main\IGroup::SuperAdmins);
+        self::$defaultMember = self::$member;
         self::insertSummitTestData();
+
+        // SummitService::saveOrUpdateEvent()/publishEvent() read the current user via the
+        // \App\Facades\ResourceServerContext static facade to stamp created_by/updated_by.
+        // The concrete \models\oauth2\ResourceServerContext is final, so Mockery can't mock
+        // the facade directly - mock the interface instead and rebind it under the facade's
+        // accessor key (established this session in tests/PresentationServiceTest.php).
+        $resource_server_context_mock = \Mockery::mock(\models\oauth2\IResourceServerContext::class);
+        $resource_server_context_mock->shouldReceive('getCurrentUser')->with(false)->andReturn(self::$defaultMember);
+        App::instance('resource_server_context', $resource_server_context_mock);
     }
 
     protected function tearDown(): void
     {
         self::clearSummitTestData();
+        self::clearMemberTestData();
         parent::tearDown();
+        \Mockery::close();
+    }
+
+    /**
+     * self::$defaultEventType has blackout_times='All' (a deliberate InsertSummitTestData
+     * fixture choice meant to conflict with everything else), and a pre-seeded event of that
+     * type already occupies most of the summit's first day - unusable for tests that need to
+     * create their OWN non-conflicting events. Build a dedicated, non-blackout event type.
+     */
+    private function createNonBlackoutEventType(): SummitEventType
+    {
+        $type = new SummitEventType();
+        $type->setType(ISummitEventType::Lunch);
+        $type->setBlackoutTimes(SummitEventTypeConstants::BLACKOUT_TIME_NONE);
+        self::$summit->addEventType($type);
+        self::$em->persist($type);
+        self::$em->flush();
+        return $type;
+    }
+
+    /**
+     * @return \DateTime[] [$start_date, $end_date], both within the summit's date range.
+     */
+    private function nonConflictingEventWindow(): array
+    {
+        $start_date = (clone self::$summit->getBeginDate())->add(new \DateInterval('PT1H'));
+        $end_date = (clone $start_date)->add(new \DateInterval('PT1H'));
+        return [$start_date, $end_date];
     }
 
     /**
@@ -76,4 +122,117 @@ public function testProcessRegistrationCompaniesDataSkipsFailingRowButProcessesL
         $new_company_on_summit = self::$summit->getRegistrationCompanyByName($new_company_name);
         $this->assertNotNull($new_company_on_summit);
     }
+
+    /**
+     * SummitService::unPublishEvents() (SummitService.php:1467) loops calling
+     * unPublishEvent() (:1034, its own nested transaction) per event id - no try/catch.
+     * The FIRST (valid) id's unpublish commits inside its own nested transaction; a SECOND,
+     * nonexistent id then throws EntityNotFoundException (:1041), rolling back the ENTIRE
+     * batch, including the first id's already-committed unpublish.
+     */
+    public function testUnPublishEventsRollsBackAlreadyCommittedUnpublishOnLaterBadEventId()
+    {
+        $service = App::make(ISummitService::class);
+        $event_type = $this->createNonBlackoutEventType();
+        [$start_date, $end_date] = $this->nonConflictingEventWindow();
+
+        // No location_id set - AbstractPublishService::validateBlackOutTimesAndTimes()
+        // only enforces blackout collisions when the event has a non-null location
+        // (Summit fixture data seeds many published events spanning the whole date
+        // range with blackout_times set, so any located event anywhere would collide).
+        $event = $service->addEvent(self::$summit, [
+            'title' => 'Batch Unpublish Test ' . uniqid(),
+            'type_id' => $event_type->getId(),
+            'start_date' => $start_date->getTimestamp(),
+            'end_date' => $end_date->getTimestamp(),
+        ]);
+        $event_id = $event->getId();
+        $service->publishEvent(self::$summit, $event_id, []);
+
+        try {
+            $service->unPublishEvents(self::$summit, ['events' => [$event_id, 999999999]]);
+            $this->fail('Expected EntityNotFoundException was not thrown');
+        } catch (EntityNotFoundException $ex) {
+            $this->assertStringContainsString('999999999', $ex->getMessage());
+        }
+
+        self::$em->clear();
+        $reFetched = self::$em->find(SummitEvent::class, $event_id);
+        $this->assertNotNull($reFetched);
+        $this->assertTrue($reFetched->isPublished());
+    }
+
+    /**
+     * SummitService::updateAndPublishEvents() (SummitService.php:1488) loops calling
+     * updateEvent() (:620, own nested transaction) then publishEvent() (:966, own nested
+     * transaction) per event - no try/catch. Item 1's update+publish commits fully; item 2's
+     * bad location_id then rolls back the whole call - including item 1's already-committed
+     * update+publish.
+     *
+     * The exception is thrown by updateEvent()'s underlying saveOrUpdateEvent()
+     * (SummitService.php:702-708, "location id %s does not exists!") - NOT by publishEvent()'s
+     * own, textually-identical location check (:999-1008). Since updateAndPublishEvents()
+     * passes the SAME $event_data to both calls and calls updateEvent() first (:1495-1496),
+     * saveOrUpdateEvent()'s unconditional (no isAllowsLocation gate) location-existence check
+     * always preempts publishEvent()'s gated one for any bad location_id in that payload -
+     * confirmed via code review during spec-verify; publishEvent()'s own check is not reachable
+     * through this call site. This still proves the pair's core claim (a batch item's nested-tx
+     * failure rolls back an EARLIER item's already-committed nested-tx work), just via
+     * updateEvent()'s nested transaction rather than publishEvent()'s specifically.
+     */
+    public function testUpdateAndPublishEventsRollsBackAlreadyCommittedFirstItemWhenSecondItemsUpdateEventFailsOnBadLocationId()
+    {
+        $service = App::make(ISummitService::class);
+        $event_type = $this->createNonBlackoutEventType();
+        [$start_date, $end_date] = $this->nonConflictingEventWindow();
+
+        // event1 has no location_id - it gets published within the batch call, and
+        // AbstractPublishService::validateBlackOutTimesAndTimes() only enforces blackout
+        // collisions when the event has a non-null location (see the sibling test above).
+        $original_title_1 = 'Original Title 1 ' . uniqid();
+        $event1 = $service->addEvent(self::$summit, [
+            'title' => $original_title_1,
+            'type_id' => $event_type->getId(),
+            'start_date' => $start_date->getTimestamp(),
+            'end_date' => $end_date->getTimestamp(),
+        ]);
+        $event1_id = $event1->getId();
+
+        $event2 = $service->addEvent(self::$summit, [
+            'title' => 'Event 2 ' . uniqid(),
+            'type_id' => $event_type->getId(),
+            'location_id' => self::$mainVenue->getId(),
+            'start_date' => $start_date->getTimestamp(),
+            'end_date' => $end_date->getTimestamp(),
+        ]);
+        $event2_id = $event2->getId();
+
+        try {
+            $service->updateAndPublishEvents(self::$summit, [
+                'events' => [
+                    [
+                        'id' => $event1_id,
+                        'title' => 'Updated Title Should Not Persist',
+                        'start_date' => $start_date->getTimestamp(),
+                        'end_date' => $end_date->getTimestamp(),
+                    ],
+                    [
+                        'id' => $event2_id,
+                        'start_date' => $start_date->getTimestamp(),
+                        'end_date' => $end_date->getTimestamp(),
+                        'location_id' => 999999999,
+                    ],
+                ],
+            ]);
+            $this->fail('Expected EntityNotFoundException was not thrown');
+        } catch (EntityNotFoundException $ex) {
+            $this->assertStringContainsString('location id', $ex->getMessage());
+        }
+
+        self::$em->clear();
+        $reFetchedEvent1 = self::$em->find(SummitEvent::class, $event1_id);
+        $this->assertNotNull($reFetchedEvent1);
+        $this->assertEquals($original_title_1, $reFetchedEvent1->getTitle());
+        $this->assertFalse($reFetchedEvent1->isPublished());
+    }
 }
diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php
index 68a3acef6..32720ae6d 100644
--- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php
+++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php
@@ -14,6 +14,7 @@
 
 use App\Models\Foundation\Main\IGroup;
 use App\Services\Model\ISponsorUserSyncService;
+use models\exceptions\EntityNotFoundException;
 use Tests\InsertMemberTestData;
 use Tests\InsertSummitTestData;
 use Tests\TestCase;
@@ -89,6 +90,16 @@ private function getPermissions(int $sponsor_id, int $member_id): array
         return json_decode($raw, true) ?? [];
     }
 
+    private function assertNoSponsorUsersRowExists(int $sponsor_id, int $member_id): void
+    {
+        $conn = self::$em->getConnection();
+        $exists = $conn->executeQuery(
+            'SELECT COUNT(*) FROM Sponsor_Users WHERE SponsorID = ? AND MemberID = ?',
+            [$sponsor_id, $member_id]
+        )->fetchOne();
+        $this->assertEquals(0, (int)$exists, 'Pre-condition: no Sponsor_Users row should exist');
+    }
+
     // -------------------------------------------------------------------------
     // addSponsorUserToGroup
     // -------------------------------------------------------------------------
@@ -108,14 +119,8 @@ public function testAddSponsorUserToGroupEagerlyCreatesRowAndWritesPermissionOnR
         $external_id = self::$member->getUserExternalId();
         $summit_id   = self::$summit->getId();
 
-        $conn = self::$em->getConnection();
-
         // Confirm no row exists before the call.
-        $exists = $conn->executeQuery(
-            'SELECT COUNT(*) FROM Sponsor_Users WHERE SponsorID = ? AND MemberID = ?',
-            [$sponsor_id, $member_id]
-        )->fetchOne();
-        $this->assertEquals(0, (int)$exists, 'Pre-condition: no Sponsor_Users row should exist');
+        $this->assertNoSponsorUsersRowExists($sponsor_id, $member_id);
 
         $this->getService()->addSponsorUserToGroup($external_id, IGroup::Sponsors, $sponsor_id, $summit_id);
 
@@ -123,6 +128,41 @@ public function testAddSponsorUserToGroupEagerlyCreatesRowAndWritesPermissionOnR
         $this->assertContains(IGroup::Sponsors, $this->getPermissions($sponsor_id, $member_id));
     }
 
+    /**
+     * SponsorUserSyncService::addSponsorUserToGroup() (SponsorUserSyncService.php:147) calls
+     * SummitSponsorService::addSponsorUser() (SummitSponsorService.php:459, its own nested
+     * transaction) at :171 when no Sponsor_Users row exists yet - no try/catch. That nested
+     * call commits a real Sponsor_Users row (same eager-creation path as the test above, this
+     * time with a valid sponsor). If the outer's own subsequent group lookup then fails
+     * (EntityNotFoundException at :191 for an unresolvable group_slug), the entire
+     * addSponsorUserToGroup call rolls back, including the just-committed Sponsor_Users row.
+     */
+    public function testAddSponsorUserToGroupRollsBackAlreadyCommittedSponsorUserRowWhenGroupNotFound(): void
+    {
+        $sponsor_id  = self::$sponsors[1]->getId();
+        $member_id   = self::$member->getId();
+        $external_id = self::$member->getUserExternalId();
+        $summit_id   = self::$summit->getId();
+        $bogus_group_slug = 'no-such-group-' . uniqid();
+
+        $this->assertNoSponsorUsersRowExists($sponsor_id, $member_id);
+
+        try {
+            $this->getService()->addSponsorUserToGroup($external_id, $bogus_group_slug, $sponsor_id, $summit_id);
+            $this->fail('Expected EntityNotFoundException was not thrown');
+        } catch (EntityNotFoundException $ex) {
+            // Assert the specific "Group ... not found" message - addSponsorUserToGroup()
+            // has two OTHER EntityNotFoundException throw sites sharing the same "not found"
+            // substring (member lookup, summit lookup), so a generic substring check can't
+            // distinguish this scenario from those.
+            $this->assertStringContainsString($bogus_group_slug, $ex->getMessage());
+            $this->assertStringContainsString('not found', $ex->getMessage());
+        }
+
+        self::$em->clear();
+        $this->assertEmpty($this->getPermissions($sponsor_id, $member_id));
+    }
+
     /**
      * The group slug must be written into the Sponsor_Users.Permissions JSON
      * column for the correct (SponsorID, MemberID) row.

From 7e7967fd28093b2c1d1e06e938fd91ac50738e6f Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 17:32:51 -0300
Subject: [PATCH 08/13] docs(adr): update nested-tx rollback ADR with remaining
 test coverage

Moves the 8 newly-covered outer/inner pairs from docs/plans/2026-07-10-remaining-nested-tx-coverage.md
into the Test Coverage Added table, including the mechanism nuance found
during implementation (updateAndPublishEvents' rollback actually fires via
updateEvent's unconditional location check, not publishEvent's gated one,
since both run against the same payload and updateEvent executes first).

Known Gaps / Future Work now lists only the 3 pairs confirmed structurally
unreachable for a genuine committed-then-rolled-back test (TagService's
duplicate check has no exploitable gap vs its caller's identical
pre-check; SummitRSVPInvitationService's rsvpEvent call has no write
before or reachable after the nested failure), plus notes on why two
other candidates were dropped as redundant or non-distinguishing rather
than untested gaps. The codebase-wide sweep for this transaction shape is
now complete.
---
 adr/003-nested-transaction-rollback-safety.md | 60 ++++++++++++-------
 1 file changed, 39 insertions(+), 21 deletions(-)

diff --git a/adr/003-nested-transaction-rollback-safety.md b/adr/003-nested-transaction-rollback-safety.md
index 6703609c9..4df5fac3f 100644
--- a/adr/003-nested-transaction-rollback-safety.md
+++ b/adr/003-nested-transaction-rollback-safety.md
@@ -175,6 +175,13 @@ guaranteed behavior end-to-end.
 | `PresentationService::submitPresentation` | `saveOrUpdatePresentation` | `tests/PresentationServiceTest.php` | Full rollback — nonexistent track |
 | `PresentationService::updatePresentationSubmission` | `saveOrUpdatePresentation` | `tests/PresentationServiceTest.php` | Full rollback — nonexistent track, update never partially applies |
 | `SummitPromoCodeService::addPromoCode` | `addPromoCodeTicketTypeRule` | `tests/SummitPromoCodeServiceTest.php` | **Partial commit, not full rollback** — this is actually two separate, sequential root transactions; the promo code from the first survives even when the second (ticket-type-rules) transaction fails |
+| `SelectionPlanOrderExtraQuestionTypeService::updateExtraQuestionBySelectionPlan` | `updateExtraQuestion` | `tests/SelectionPlanOrderExtraQuestionTypeServiceTest.php` | Full rollback — a question not assigned to the target plan lets the inner label change commit, then the outer's assignment check fails and undoes it |
+| `SpeakerService::updateSpeakerBySummit` | `registerSummitPromoCodeByValue` | `tests/SpeakerServiceRegistrationTest.php` | Full rollback — `updateSpeaker`'s already-committed title change is undone when the registration code is already claimed by another speaker |
+| `SponsorUserSyncService::addSponsorUserToGroup` | `SummitSponsorService::addSponsorUser` | `tests/Unit/Services/SponsorUserPermissionTrackingTest.php` | Full rollback — the eagerly-created `Sponsor_Users` row commits, then an unresolvable `group_slug` undoes it |
+| `SummitScheduleSettingsService::seedDefaults` | `add` (looped) | `tests/SummitScheduleSettingsServiceTest.php` | Full rollback — the loop's first successfully-added default is undone when the second's key already exists |
+| `SummitSelectedPresentationListService::assignPresentationToMyIndividualList` | `createIndividualSelectionList` | `tests/SummitSelectedPresentationListServiceTest.php` | Full rollback — the just-committed new individual list is undone when the presentation lookup afterward fails |
+| `SummitService::unPublishEvents` | `unPublishEvent` (looped) | `tests/SummitServiceTest.php` | Full rollback — an earlier item's already-committed unpublish is undone when a later item's event id doesn't exist |
+| `SummitService::updateAndPublishEvents` | `updateEvent` (looped) | `tests/SummitServiceTest.php` | Full rollback — an earlier item's already-committed update+publish is undone when a later item's `location_id` doesn't exist. **Nuance found during implementation:** the exception actually fires inside `updateEvent()`'s own location check, not `publishEvent()`'s — both do the identical existence check on the same payload, but `updateEvent()` runs first and its check has no `isAllowsLocation()` gate, so `publishEvent()`'s own check is structurally unreachable via this call site for this trigger |
 
 ### API/HTTP-level exception-branch coverage
 
@@ -187,23 +194,33 @@ guaranteed behavior end-to-end.
 
 A follow-up codebase-wide search (same outer/inner shape: method A wrapped in its own
 `tx_service->transaction()` calling, with no local `try/catch` around the call, method B which
-is *also* wrapped in its own separate `tx_service->transaction()`) found **11 additional
-pairs, across 8 more services, that are NOT yet covered by any test** proving the new
-rollback contract:
-
-| # | Outer → Inner | Realistic trigger |
-|---|---|---|
-| 1 | `SelectionPlanOrderExtraQuestionTypeService::updateExtraQuestionBySelectionPlan` → `updateExtraQuestion` | Duplicate question name/label for the selection plan |
-| 2 | `SpeakerService::updateSpeakerBySummit` → `updateSpeaker` | Member already assigned to another speaker |
-| 3 | `SpeakerService::updateSpeakerBySummit` → `registerSummitPromoCodeByValue` | Promo code already redeemed/assigned to another speaker |
-| 4 | `SponsorUserSyncService::addSponsorUserToGroup` → `SummitSponsorService::addSponsorUser` | Member already a sponsor-user on a date-overlapping summit |
-| 5 | `SummitRSVPInvitationService::acceptInvitationBySummitEventAndToken` → `SummitRSVPService::rsvpEvent` | Attendee has no valid tickets / duplicate RSVP |
-| 6 | `SummitRegistrationInvitationService::add`/`update` → `TagService::addTag` | Duplicate tag race |
-| 7 | `SummitSubmissionInvitationService::add`/`update` → `TagService::addTag` | Same as #6 |
-| 8 | `SummitScheduleSettingsService::seedDefaults` (loop) → `add` | A later default's key already exists — rolls back earlier successfully-added defaults in the same loop too |
-| 9 | `SummitSelectedPresentationListService::getTeamSelectionList`/`getIndividualSelectionList`/`assignPresentationToMyIndividualList` → `createTeamSelectionList`/`createIndividualSelectionList` | TOCTOU re-check failure (`AuthzException`/`EntityNotFoundException`) |
-| 10 | `SummitService::unPublishEvents` (batch loop) → `unPublishEvent` | One bad event id in a batch undoes the whole batch |
-| 11 | `SummitService::updateAndPublishEvents` (batch loop) → `publishEvent` | Validation/entity-not-found failure on one event in a batch undoes the whole batch |
+is *also* wrapped in its own separate `tx_service->transaction()`) originally found 11
+additional pairs across 8 more services with no test proving the new rollback contract. 8 of
+those 11 are now covered (see Test Coverage Added above,
+`docs/plans/2026-07-10-remaining-nested-tx-coverage.md`). The remaining 3 were verified against
+the real code and found to have **no committed-then-rolled-back proof reachable through their
+specific call site** — the same structural reason a genuine rollback test can't be built for
+them, not merely undiscovered test cases:
+
+| Outer → Inner | Why no test is possible here |
+|---|---|
+| `SummitRegistrationInvitationService::add`/`update` → `TagService::addTag` | The outer's own pre-check (`$this->tag_repository->getByTag($tag_value)`) already uses the identical normalized comparison (`UPPER(TRIM(...))`, `DoctrineTagRepository.php:54-63`) that `addTag()`'s own duplicate check uses internally — no case/whitespace gap exists between them for a single synchronous request. `addTag()`'s own `ValidationException("Tag %s already exists!")` can only fire via a genuine concurrent race between two overlapping requests, which cannot be expressed deterministically in a test. |
+| `SummitSubmissionInvitationService::add`/`update` → `TagService::addTag` | Same reason as above. |
+| `SummitRSVPInvitationService::acceptInvitationBySummitEventAndToken` → `SummitRSVPService::rsvpEvent` | The outer method has no write before the nested `rsvpEvent()` call (its own guards, `:312-336`, are pure reads), and its only post-call write (`markAsAcceptedWithRSVP()`, `:343`) runs strictly *after* `rsvpEvent()` returns — so when the inner throws, there is nothing already committed for a rollback to undo. Asserting `isAccepted() === false` afterward would hold identically true with `isRollbackOnly` fully disabled. |
+
+Also dropped as **redundant, not unreachable**: `SpeakerService::updateSpeakerBySummit` →
+`updateSpeaker` via the member-already-assigned trigger. `updateSpeaker`'s check throws before
+any write and is the outer's first operation, so this specific trigger can't prove rollback
+either — but the *same* outer/inner pair is already proven via the `registration_code` trigger
+(`updateSpeaker`'s title change commits first, then `registerSummitPromoCodeByValue` fails and
+rolls it back), so no second test was needed for this production class.
+
+`SummitSelectedPresentationListService::getTeamSelectionList`/`getIndividualSelectionList` →
+`createTeamSelectionList`/`createIndividualSelectionList` were also dropped: both outer methods
+return immediately after the inner create call with no additional outer-side writes, so testing
+them would only re-prove the inner methods' own already-covered nested-transaction mechanism,
+not a distinct outer/inner rollback risk. Only `assignPresentationToMyIndividualList` (which
+does real work — presentation validation — after the nested call) was covered.
 
 Several other call sites share the same *family* (calling a nested-transaction-wrapped method)
 but are already guarded by a local `try/catch` outside the inner closure — the same safe shape
@@ -214,7 +231,8 @@ as `SummitService::processRegistrationCompaniesData` above, not a rollback risk:
 `ScheduleService::publishAll` → `SummitService::publishEvent`; `SummitService::processEventData`
 (per-CSV-row) → `SpeakerService::addSpeaker`/`updateSpeaker`.
 
-None of the 11 gaps above are fixed or tested by this ADR's work — they are recorded here as
-the next candidates for the same test-coverage treatment applied to the 9 pairs already closed
-(see `docs/plans/2026-07-10-nested-tx-other-services-coverage.md` and
-`docs/plans/2026-07-10-summit-order-api-exception-coverage.md`).
+With the 8 testable gaps closed and the remaining 3 confirmed structurally unreachable, this
+codebase-wide sweep for the outer/inner nested-transaction shape is complete (see
+`docs/plans/2026-07-10-nested-tx-other-services-coverage.md`,
+`docs/plans/2026-07-10-summit-order-api-exception-coverage.md`, and
+`docs/plans/2026-07-10-remaining-nested-tx-coverage.md`).

From f4a125cd2633ec3ac1fecf8dc65c9f04c2f054a0 Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 17:35:03 -0300
Subject: [PATCH 09/13] docs(adr): remove docs/plans references from ADR-003

docs/plans/ is gitignored workflow state, not part of the committed repo -
referencing those paths from a tracked ADR pointed readers at files that
don't exist for them.
---
 adr/003-nested-transaction-rollback-safety.md | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/adr/003-nested-transaction-rollback-safety.md b/adr/003-nested-transaction-rollback-safety.md
index 4df5fac3f..35b2df71a 100644
--- a/adr/003-nested-transaction-rollback-safety.md
+++ b/adr/003-nested-transaction-rollback-safety.md
@@ -196,8 +196,7 @@ A follow-up codebase-wide search (same outer/inner shape: method A wrapped in it
 `tx_service->transaction()` calling, with no local `try/catch` around the call, method B which
 is *also* wrapped in its own separate `tx_service->transaction()`) originally found 11
 additional pairs across 8 more services with no test proving the new rollback contract. 8 of
-those 11 are now covered (see Test Coverage Added above,
-`docs/plans/2026-07-10-remaining-nested-tx-coverage.md`). The remaining 3 were verified against
+those 11 are now covered (see Test Coverage Added above). The remaining 3 were verified against
 the real code and found to have **no committed-then-rolled-back proof reachable through their
 specific call site** — the same structural reason a genuine rollback test can't be built for
 them, not merely undiscovered test cases:
@@ -232,7 +231,4 @@ as `SummitService::processRegistrationCompaniesData` above, not a rollback risk:
 (per-CSV-row) → `SpeakerService::addSpeaker`/`updateSpeaker`.
 
 With the 8 testable gaps closed and the remaining 3 confirmed structurally unreachable, this
-codebase-wide sweep for the outer/inner nested-transaction shape is complete (see
-`docs/plans/2026-07-10-nested-tx-other-services-coverage.md`,
-`docs/plans/2026-07-10-summit-order-api-exception-coverage.md`, and
-`docs/plans/2026-07-10-remaining-nested-tx-coverage.md`).
+codebase-wide sweep for the outer/inner nested-transaction shape is complete.

From dab54b0df9115bd226683230cdd2579075b15cbd Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 17:40:15 -0300
Subject: [PATCH 10/13] chore: add nested-tx rollback test classes to push.yml
 CI matrix

These test classes live directly under tests/ (not under any of the
existing directory-based buckets: tests/oauth2/, tests/Unit/Entities/,
tests/Unit/Audit/, tests/Repositories/, tests/Unit/Services/), so CI was
silently skipping them - they only ever ran locally. Adds one matrix
entry per class, matching the existing SummitOrderServiceTest/
SummitRSVPServiceTest pattern:

- SummitServiceTest
- SpeakerServiceRegistrationTest
- PresentationServiceTest
- SummitPromoCodeServiceTest
- SummitScheduleSettingsServiceTest
- SummitSelectedPresentationListServiceTest
- SelectionPlanOrderExtraQuestionTypeServiceTest

Verified each --filter matches exactly its intended class with no overlap
with existing filters (--list-tests against the local Docker instance).
---
 .github/workflows/push.yml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml
index 51960448f..f204db189 100644
--- a/.github/workflows/push.yml
+++ b/.github/workflows/push.yml
@@ -58,6 +58,13 @@ jobs:
                     - { name: "SummitOrderServiceTest", filter: "--filter SummitOrderServiceTest" }
                     - { name: "SummitRSVPServiceTest", filter: "--filter SummitRSVPServiceTest" }
                     - { name: "SummitRSVPInvitationServiceTest", filter: "--filter SummitRSVPInvitationServiceTest" }
+                    - { name: "SummitServiceTest", filter: "--filter SummitServiceTest" }
+                    - { name: "SpeakerServiceRegistrationTest", filter: "--filter SpeakerServiceRegistrationTest" }
+                    - { name: "PresentationServiceTest", filter: "--filter PresentationServiceTest" }
+                    - { name: "SummitPromoCodeServiceTest", filter: "--filter SummitPromoCodeServiceTest" }
+                    - { name: "SummitScheduleSettingsServiceTest", filter: "--filter SummitScheduleSettingsServiceTest" }
+                    - { name: "SummitSelectedPresentationListServiceTest", filter: "--filter SummitSelectedPresentationListServiceTest" }
+                    - { name: "SelectionPlanOrderExtraQuestionTypeServiceTest", filter: "--filter SelectionPlanOrderExtraQuestionTypeServiceTest" }
                     - { name: "EntityModelUnitTests", filter: "tests/Unit/Entities/" }
                     - { name: "AuditUnitTests", filter: "tests/Unit/Audit/" }
                     - { name: "AuditOtlpStrategyTest", filter: "--filter AuditOtlpStrategyTest" }

From da8b2210f0202630bdaeeed05a51e20297f90850 Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 17:50:02 -0300
Subject: [PATCH 11/13] docs(adr): clarify origin/main's actual pre-branch
 behavior in ADR-003

Adds a "Baseline (origin/main)" section before Iteration 1, verified
directly against origin/main (988a6d3e6): main never used DBAL savepoints
and had no root/nested distinction at all - every transaction() call,
nested or not, ran an identical retry loop that unconditionally closed
the connection and EntityManager and reset the registry on ANY exception,
not just connection errors. Savepoints were introduced (and later
discarded) entirely within this branch's own Iteration 1, not present on
main. Prevents a reader from assuming main already had some form of
scoped/partial nested-rollback handling.
---
 adr/003-nested-transaction-rollback-safety.md | 47 +++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/adr/003-nested-transaction-rollback-safety.md b/adr/003-nested-transaction-rollback-safety.md
index 35b2df71a..0d2a525e9 100644
--- a/adr/003-nested-transaction-rollback-safety.md
+++ b/adr/003-nested-transaction-rollback-safety.md
@@ -26,10 +26,51 @@ right.
 
 ## Decision Timeline
 
-### Iteration 1 (initial approach) — DBAL savepoints, discarded
+### Baseline (`origin/main`) — no nested-transaction awareness at all
 
-Commit `d88de2ce3` introduced the root/nested split above and, alongside it, called
-`$conn->setNestTransactionsWithSavepoints(true)` on the root transaction. The intent: when a
+Before this branch, `DoctrineTransactionService::transaction()` (`origin/main`, verified at
+`988a6d3e6`) had **no root/nested distinction whatsoever** — every call, whether the outermost
+one or one invoked from inside another already-open `transaction()` call, ran the identical
+retry loop:
+
+```php
+$em->getConnection()->beginTransaction();
+$result = $callback($this);
+$em->flush();
+$em->getConnection()->commit();
+```
+
+And on **any** exception at all — a connection drop, or an ordinary business-rule
+`ValidationException` with no connection problem whatsoever — the catch block did this
+**unconditionally**, regardless of nesting depth or exception type:
+
+```php
+$em->getConnection()->close();
+$em->close();
+if ($em->getConnection()->isTransactionActive()) $em->getConnection()->rollBack();
+Registry::resetManager($this->manager_name);
+```
+
+No `setNestTransactionsWithSavepoints` call exists anywhere in this baseline — savepoints were
+never part of `main`'s design; they were introduced (and later discarded) entirely within this
+branch, see Iteration 1 below.
+
+**Why this is unsafe for nested calls.** Because a nested `transaction()` call is just a normal
+recursive call to this same method, a failure at the INNER level closes the entire physical DB
+connection and the shared `EntityManager` before the exception even propagates back to the
+OUTER call. The outer call's own local `$em` reference now points at an already-closed manager
+— its next `flush()` throws `EntityManagerClosed`, which its own catch block then tears down
+*again* (a second `close()+close()+resetManager()`), on top of whatever work the outer callback
+had pending. This is the mechanism the empirical A/B test later in this document refers to as
+"`main` (old code): ... calls `Registry::resetManager()` **twice**, loses everything." The
+blast radius is total and applies to every nested failure uniformly, not just connection-level
+ones — there is no scoped, partial-rollback concept here at all.
+
+### Iteration 1 — DBAL savepoints, discarded
+
+Commit `d88de2ce3` introduced the root/nested split above (specifically to stop a nested
+failure from destroying the outer `EntityManager`, per that commit's own message) and, alongside
+it, called `$conn->setNestTransactionsWithSavepoints(true)` on the root transaction. The intent: when a
 nested transaction's `rollBack()` runs on a savepoints-enabled connection, DBAL issues
 `ROLLBACK TO SAVEPOINT` instead of rolling back the whole DB transaction — undoing only the
 inner work while leaving the outer transaction free to continue and commit its own writes.

From 16cbe1618d6f94fc1be6ca957c0ec0cb38ff5a7b Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 18:25:01 -0300
Subject: [PATCH 12/13] fix(transactions): never retry ambiguous commits,
 refuse closed-EM re-entry

Closes the three findings from the PR #533 deep review:

- Root transactions no longer retry once the real COMMIT has been
  attempted: a connection failure during COMMIT is ambiguous (the server
  may have already made the transaction durable and only the ack was
  lost), so re-executing the callback could duplicate every write and
  side effect. Commit-phase failures now propagate as "operation state
  unknown".
- transaction() now refuses to run when the EntityManager is closed
  while its connection still holds an active transaction: resetting onto
  a brand-new EM/connection would produce durable commits that survive
  the outer rollback (split-brain partial commit escaping the
  isRollbackOnly guarantee).
- failFastIfEntityManagerClosed()'s message no longer repeats the
  retracted "safe before any flush" carve-out; catching a nested
  transaction() failure and continuing is never safe.

Each fix landed with a unit test that reproduced the failure first
(callback executed 10x on ambiguous commit; resetManager reached on
closed-EM re-entry). The mis-modeled nested fail-fast test now
genuinely exercises the nested path. ADR-003 documents the hardening.
---
 adr/003-nested-transaction-rollback-safety.md | 30 +++++-
 .../Utils/DoctrineTransactionService.php      | 67 ++++++++++++--
 .../DoctrineTransactionServiceTest.php        | 92 +++++++++++++------
 3 files changed, 149 insertions(+), 40 deletions(-)

diff --git a/adr/003-nested-transaction-rollback-safety.md b/adr/003-nested-transaction-rollback-safety.md
index 0d2a525e9..9903e450a 100644
--- a/adr/003-nested-transaction-rollback-safety.md
+++ b/adr/003-nested-transaction-rollback-safety.md
@@ -189,6 +189,34 @@ pre-existing bug, out of scope for this hotfix.
   cause was a transient connection blip) — a pre-existing gap this fix reduces one trigger for
   but does not close.
 
+## Post-Review Hardening (same branch)
+
+Three gaps surfaced by the deep review of PR #533 were closed on top of Iteration 2, each with
+a unit test that reproduced the failure first:
+
+1. **Ambiguous commit failures are never retried.** A connection failure during the root's
+   real `COMMIT` is ambiguous — the server may have already made the transaction durable and
+   only the acknowledgment was lost (DBAL decrements its nesting counter in a `finally` even
+   when the physical commit fails, so nothing is left to roll back). The retry loop used to
+   re-execute the entire callback in that state, duplicating every write and side effect up to
+   `MaxRetries` times; `runRootTransaction()` now tracks the commit phase and propagates the
+   failure instead. Callers must treat it as "operation state unknown", not as a clean failure.
+   Covered by `testRootTransactionDoesNotRetryWhenCommitFails`.
+2. **Closed-EM re-entry can no longer escape the atomicity guarantee.** `isRollbackOnly` is a
+   per-connection flag. When a nested flush failure closed the `EntityManager` and an
+   intermediate callback caught it and called `transaction()` again, the closed-EM branch used
+   to `resetManager()` onto a brand-new `EntityManager` **and a brand-new DBAL connection**
+   (`EntityManagerFactory` → `DriverManager::getConnection`) — the "recovered" root's commits
+   were durable even though the outer, rollback-only transaction on the old connection rolled
+   back: a split-brain partial commit. `transaction()` now refuses that state (closed EM while
+   its connection still has an active transaction) with a descriptive `RuntimeException`.
+   Covered by `testTransactionRefusesWhenEntityManagerClosedWithActiveTransaction`.
+3. **The fail-fast message no longer repeats the retracted carve-out.** It used to claim
+   catching nested errors is "only safe for errors thrown before any flush" — contradicting
+   this ADR's own conclusion that DBAL sets `isRollbackOnly` unconditionally, pre-flush or not.
+   The message now states that catching a nested `transaction()` failure and continuing is
+   never safe and directs the reader to let the failure propagate to the root.
+
 ## Test Coverage Added
 
 Two things were proven for every pair below, empirically (real local MySQL, not mocks — DBAL's
@@ -201,7 +229,7 @@ guaranteed behavior end-to-end.
 
 | File | What's covered |
 |---|---|
-| `tests/Unit/Services/DoctrineTransactionServiceTest.php` | 24 tests: root/nested routing, savepoints never enabled/queried, fail-fast on a closed `EntityManager`, rollback failures never masking the original exception, `\Error` handled alongside `\Exception`, `em->clear()` on root failure. |
+| `tests/Unit/Services/DoctrineTransactionServiceTest.php` | 26 tests: root/nested routing, savepoints never enabled/queried, fail-fast on a closed `EntityManager`, no retry after an ambiguous commit failure, refusal of closed-EM re-entry while a transaction is still active, rollback failures never masking the original exception, `\Error` handled alongside `\Exception`, `em->clear()` on root failure. |
 
 ### Business-service outer/inner pairs (functional, real DB)
 
diff --git a/app/Services/Utils/DoctrineTransactionService.php b/app/Services/Utils/DoctrineTransactionService.php
index 654a3fb0d..259290d12 100644
--- a/app/Services/Utils/DoctrineTransactionService.php
+++ b/app/Services/Utils/DoctrineTransactionService.php
@@ -35,6 +35,10 @@
  *  - flushes the EntityManager and issues the real COMMIT
  *  - on failure, rolls back and clears the EntityManager so no
  *    UnitOfWork state leaks into subsequent transactions
+ *  - never retries once the real COMMIT has been attempted: a connection
+ *    failure during COMMIT is ambiguous (the server may have already made
+ *    the transaction durable), so re-executing the callback could duplicate
+ *    every write and side effect
  *
  * Nested transaction (called while a DB transaction is already active):
  *  - uses Doctrine DBAL nesting counter (beginTransaction / commit)
@@ -136,17 +140,36 @@ public function transaction(Closure $callback, int $isolationLevel = Transaction
         $conn = $em->getConnection();
 
         // Detect whether we are already inside a DB transaction. A closed EntityManager
-        // is never treated as nested, even if the connection still reports an active
-        // transaction (e.g. a prior failure whose rollback attempt itself failed):
-        // runNestedTransaction() has no isOpen()-based reset, so routing a closed EM
-        // there would hand the callback a dead EntityManager instead of going through
-        // runRootTransaction()'s existing reset-and-retry path.
-        $isNested = $em->isOpen() && $conn->isTransactionActive();
+        // is never treated as nested: runNestedTransaction() has no isOpen()-based
+        // reset, so routing a closed EM there would hand the callback a dead
+        // EntityManager. Both states are read exactly once so the routing decision
+        // and the refusal check below cannot disagree.
+        $emIsOpen = $em->isOpen();
 
-        if ($isNested) {
+        if ($emIsOpen && $conn->isTransactionActive()) {
             return $this->runNestedTransaction($callback);
         }
 
+        if (!$emIsOpen && $conn->isTransactionActive()) {
+            // A closed EM whose connection still holds an open transaction means a
+            // nested failure was caught mid-propagation (the outer transaction()
+            // frame has not unwound yet). Routing this into runRootTransaction()
+            // would resetManager() onto a BRAND-NEW connection whose commits are
+            // durable even though the outer, rollback-only transaction on the old
+            // connection rolls back - a split-brain partial commit that escapes the
+            // atomicity guarantee documented above. Refuse instead; the original
+            // nested failure must propagate to the root.
+            // Plain \RuntimeException intentionally - shouldReconnect() does not
+            // match it, so this can never trigger the retry loop.
+            throw new \RuntimeException(
+                'DoctrineTransactionService::transaction the EntityManager was closed while its '
+                . 'connection still has an active transaction (typically a nested failure that was '
+                . 'caught and execution continued); refusing to start an independent root transaction '
+                . 'whose writes would survive the outer rollback. Let the original failure propagate '
+                . 'to the root transaction instead.'
+            );
+        }
+
         return $this->runRootTransaction($callback, $isolationLevel);
     }
 
@@ -169,8 +192,10 @@ private function failFastIfEntityManagerClosed($em, string $context): void
             throw new \RuntimeException(sprintf(
                 'DoctrineTransactionService::%s the EntityManager was closed during the callback '
                 . '(typically a nested flush failure that was caught and execution continued); this '
-                . 'transaction cannot be committed. Catching nested errors is only safe for errors '
-                . 'thrown before any flush in this transaction.',
+                . 'transaction cannot be committed. Catching a failure from a nested transaction() '
+                . 'call and continuing is never safe - the nested rollback already marked the '
+                . 'connection rollback-only, so a commit at any level fails. Let the failure '
+                . 'propagate to the root transaction instead.',
                 $context
             ));
         }
@@ -206,6 +231,8 @@ private function safeRollback($conn, \Throwable $cause, string $context): void
     /**
      * Root (outermost) transaction: may retry on transient connection errors,
      * sets isolation level, flushes, and issues the real COMMIT.
+     * Retries never happen once the real COMMIT has been attempted (ambiguous
+     * outcome - see the class docblock).
      *
      * @param Closure $callback
      * @param int $isolationLevel
@@ -218,6 +245,7 @@ private function runRootTransaction(Closure $callback, int $isolationLevel)
 
         while ($retry < self::MaxRetries) {
             $em = Registry::getManager($this->manager_name);
+            $commitStarted = false;
 
             try {
                 if (!$em->isOpen()) {
@@ -233,6 +261,11 @@ private function runRootTransaction(Closure $callback, int $isolationLevel)
                     $result = $callback($this);
                     $this->failFastIfEntityManagerClosed($em, 'runRootTransaction');
                     $em->flush();
+                    // Anything past this point is the real COMMIT: a connection
+                    // failure here is ambiguous (the server may have already made
+                    // the transaction durable and only the ack was lost), so the
+                    // retry path below must never re-execute the callback.
+                    $commitStarted = true;
                     $conn->commit();
                     return $result;
                 } catch (\Throwable $inner) {
@@ -251,6 +284,22 @@ private function runRootTransaction(Closure $callback, int $isolationLevel)
             } catch (Exception $ex) {
                 $retry++;
 
+                if ($commitStarted) {
+                    // The COMMIT itself failed after being sent to the server: its
+                    // outcome is unknown, so retrying could duplicate every write
+                    // and side effect of the callback. Propagate instead - callers
+                    // must treat this as "operation state unknown", not as a clean
+                    // failure.
+                    Log::error(sprintf(
+                        "DoctrineTransactionService::runRootTransaction commit outcome unknown after '%s'; not retrying.",
+                        $ex->getMessage()
+                    ));
+                    if (!$em->isOpen()) {
+                        Registry::resetManager($this->manager_name);
+                    }
+                    throw $ex;
+                }
+
                 if ($this->shouldReconnect($ex)) {
                     Log::warning(sprintf(
                         "DoctrineTransactionService::runRootTransaction reconnectable error '%s', retry %d/%d",
diff --git a/tests/Unit/Services/DoctrineTransactionServiceTest.php b/tests/Unit/Services/DoctrineTransactionServiceTest.php
index aaf62140d..7d32a5328 100644
--- a/tests/Unit/Services/DoctrineTransactionServiceTest.php
+++ b/tests/Unit/Services/DoctrineTransactionServiceTest.php
@@ -261,48 +261,46 @@ public function testRootTransactionResetsManagerOnConnectionError(): void
 
     /**
      * transaction()'s root-vs-nested routing must not trust conn->isTransactionActive()
-     * alone: a prior failure can leave the EntityManager closed while a failed/skipped
-     * rollback leaves the connection still reporting an active transaction. Routing that
-     * state into runNestedTransaction() (which has no isOpen()-based reset) would hand the
-     * callback a closed EntityManager instead of the clear, actionable reset-and-retry root
-     * path runRootTransaction() already provides.
+     * alone: a closed EntityManager while its connection still reports an active
+     * transaction means a nested failure was caught mid-propagation (the outer
+     * transaction() frame has not unwound yet). Routing that state into
+     * runNestedTransaction() would hand the callback a dead EntityManager, and
+     * routing it into runRootTransaction() would be worse: resetManager() builds a
+     * brand-new EntityManager AND a brand-new DBAL connection, so the "recovered"
+     * root would commit durable writes on the fresh connection while the outer,
+     * rollback-only transaction on the old connection rolls back - a split-brain
+     * partial commit. The only safe move is to refuse and let the original nested
+     * failure propagate to the root.
      */
-    public function testTransactionRoutesToRootWhenEntityManagerClosedDespiteActiveConnectionFlag(): void
+    public function testTransactionRefusesWhenEntityManagerClosedWithActiveTransaction(): void
     {
         $conn = Mockery::mock(Connection::class);
         $conn->shouldReceive('isTransactionActive')->andReturn(true);
-        $conn->shouldReceive('setTransactionIsolation')->byDefault();
-        $conn->shouldReceive('beginTransaction')->byDefault();
-        $conn->shouldReceive('commit')->byDefault();
-        $conn->shouldReceive('rollBack')->byDefault();
-        $conn->shouldReceive('close')->byDefault();
+        $conn->shouldReceive('beginTransaction')->never();
 
         $em = Mockery::mock(EntityManagerInterface::class);
         $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
         $em->shouldReceive('isOpen')->andReturn(false); // closed
-        $em->shouldReceive('flush')->byDefault();
-        $em->shouldReceive('clear')->byDefault();
-        $em->shouldReceive('close')->byDefault();
-
-        $freshEm = Mockery::mock(EntityManagerInterface::class);
-        $freshEm->shouldReceive('getConnection')->andReturn($conn)->byDefault();
-        $freshEm->shouldReceive('isOpen')->andReturn(true);
-        $freshEm->shouldReceive('flush')->once();
-        $freshEm->shouldReceive('clear')->byDefault();
-        $freshEm->shouldReceive('close')->byDefault();
 
         $registry = Mockery::mock(ManagerRegistry::class);
         $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
-        $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($freshEm);
+        $registry->shouldReceive('resetManager')->never();
 
         $this->container->instance(ManagerRegistry::class, $registry);
 
+        $callCount = 0;
         $service = new DoctrineTransactionService('default');
-        $result = $service->transaction(function () {
-            return 'from-fresh-em';
-        });
 
-        $this->assertSame('from-fresh-em', $result);
+        try {
+            $service->transaction(function () use (&$callCount) {
+                $callCount++;
+                return 'must-never-run';
+            });
+            $this->fail('Expected refusal when the EM is closed while a transaction is still active');
+        } catch (\RuntimeException $e) {
+            $this->assertStringContainsString('refusing to start an independent root transaction', $e->getMessage());
+            $this->assertSame(0, $callCount, 'Callback must never execute in this state');
+        }
     }
 
     // ─────────────────────────────────────────────────────────────────────────
@@ -408,15 +406,15 @@ public function testNestedTransactionRollsBackAndRethrowsOnError(): void
      * fast with a clear error - not proceed to flush() and surface an
      * opaque EntityManagerClosed one level higher than before.
      *
-     * runNestedTransaction has no retry loop, so there is exactly ONE
-     * isOpen() call after this fix (the post-callback guard) - a single
-     * `false`, not a two-value sequence like the root test.
+     * isOpen() sequence: true (transaction() routing sees the still-open EM,
+     * so this call runs as NESTED), then false (the deeper flush failure has
+     * closed the EM by the time the post-callback guard runs).
      */
     public function testNestedTransactionFailsFastWhenDeeperNestedFlushClosedEntityManager(): void
     {
         [$em, $conn] = $this->buildMocks(transactionActive: true);
 
-        $em->shouldReceive('isOpen')->andReturn(false);
+        $em->shouldReceive('isOpen')->andReturn(true, false);
         $conn->shouldReceive('isTransactionActive')->andReturn(true);
         $conn->shouldReceive('rollBack')->once();
         $em->shouldReceive('flush')->never();
@@ -913,6 +911,40 @@ public function testRootTransactionThrowsAfterMaxRetries(): void
         }
     }
 
+    /**
+     * A connection failure during the root COMMIT itself is ambiguous: the server
+     * may have already made the transaction durable even though the client never
+     * received the acknowledgment (ack-lost scenario). Retrying would re-execute
+     * the whole callback and duplicate every write and side effect (orders,
+     * tickets, emails), so commit-phase failures must bypass the reconnect/retry
+     * path and propagate immediately.
+     */
+    public function testRootTransactionDoesNotRetryWhenCommitFails(): void
+    {
+        [$em, $conn] = $this->buildMocks(transactionActive: false);
+
+        // DBAL decrements the nesting level in a finally block even when the
+        // physical COMMIT fails, so by the time the exception is caught no
+        // transaction is active - buildMocks' byDefault(false) models this.
+        $conn->shouldReceive('commit')
+            ->once()
+            ->andThrow(new TestRetryableException('server has gone away during COMMIT'));
+        $conn->shouldReceive('rollBack')->never();
+
+        $callCount = 0;
+        $service = new DoctrineTransactionService('default');
+
+        try {
+            $service->transaction(function () use (&$callCount) {
+                $callCount++;
+                return 'ok';
+            });
+            $this->fail('Expected the commit-phase failure to propagate');
+        } catch (TestRetryableException $e) {
+            $this->assertSame(1, $callCount, 'Callback must not be re-executed after an ambiguous commit failure');
+        }
+    }
+
     /**
      * Nested transaction that returns null — ensures null is properly propagated.
      * Covers the SummitPromoCodeService pattern where inner TX returns null

From 646ae982180563a87170ed325a84a1d7b52d687c Mon Sep 17 00:00:00 2001
From: smarcet <smarcet@gmail.com>
Date: Fri, 10 Jul 2026 18:39:28 -0300
Subject: [PATCH 13/13] fix(transactions): discard broken manager/connection
 when rollback fails

Closes the Codex companion review P2: safeRollback() swallows rollback
failures by design (the original exception must never be masked or
re-classified as retryable), but cleanup afterwards looked only at the
original exception and em->isOpen(). A business exception followed by a
rollback failure (connection died mid-callback) left an OPEN EM wired to
a dead physical handle registered - and DBAL zeroes the nesting level
before the physical rollback while clearing isRollbackOnly only after it
succeeds, so the flag could be left stuck too. transaction() calls
self-heal via the reconnect path, but direct Registry consumers
(repositories, serializers, queue jobs reading outside transaction())
have no retry path and would fail in a chain on a long-lived worker.

- safeRollback() now reports success/failure.
- runRootTransaction() discards the broken pair on rollback failure
  (close EM, close connection, reset a fresh manager - best-effort,
  never masking the original exception, never retrying).
- Same hygiene for connection-level commit-phase failures, which also
  left the dead handle registered.
- Root-only by construction: with savepoints off a nested rollBack()
  executes no SQL, so it cannot fail on a dead connection; that case
  surfaces as the root's own rollback failing, which this covers.

TDD: testRootTransactionDiscardsManagerWhenRollbackFails (new) and
testRootTransactionDoesNotRetryWhenCommitFails (extended) reproduced the
missing cleanup first. ADR-003 Post-Review Hardening updated (item 4).
---
 adr/003-nested-transaction-rollback-safety.md | 21 +++++-
 .../Utils/DoctrineTransactionService.php      | 71 +++++++++++++++++--
 .../DoctrineTransactionServiceTest.php        | 65 ++++++++++++++++-
 3 files changed, 149 insertions(+), 8 deletions(-)

diff --git a/adr/003-nested-transaction-rollback-safety.md b/adr/003-nested-transaction-rollback-safety.md
index 9903e450a..016abee27 100644
--- a/adr/003-nested-transaction-rollback-safety.md
+++ b/adr/003-nested-transaction-rollback-safety.md
@@ -216,6 +216,25 @@ a unit test that reproduced the failure first:
    this ADR's own conclusion that DBAL sets `isRollbackOnly` unconditionally, pre-flush or not.
    The message now states that catching a nested `transaction()` failure and continuing is
    never safe and directs the reader to let the failure propagate to the root.
+4. **A failed rollback discards the manager/connection pair** (found by a Codex companion
+   review). `safeRollback()` swallows rollback failures by design (the original exception
+   must never be masked or re-classified as retryable), but the cleanup decision afterwards
+   looked only at the original exception and `$em->isOpen()`. A business exception followed
+   by a rollback failure (connection died mid-callback) therefore left an **open** EM wired
+   to a dead physical handle registered — and DBAL zeroes `transactionNestingLevel` *before*
+   the physical rollback while clearing `isRollbackOnly` only *after* it succeeds, so the
+   flag can also be left stuck. Subsequent `transaction()` calls self-heal via the reconnect
+   path, but direct Registry consumers (repositories, serializers, queue jobs reading
+   outside `transaction()`) have no retry path and would fail in a chain on a long-lived
+   worker. `safeRollback()` now reports success/failure and `runRootTransaction()` discards
+   the broken pair (close EM, close connection, reset a fresh manager into the registry —
+   best-effort, never masking the original exception, never retrying). The same hygiene
+   applies to connection-level commit-phase failures, which previously also left the dead
+   handle registered. Root-only by construction: with savepoints off, a nested `rollBack()`
+   executes no SQL (flag + counter only), so it cannot fail on a dead connection — a dead
+   connection during a nested transaction surfaces as the root's own rollback failing, which
+   this covers. Covered by `testRootTransactionDiscardsManagerWhenRollbackFails` and the
+   extended `testRootTransactionDoesNotRetryWhenCommitFails`.
 
 ## Test Coverage Added
 
@@ -229,7 +248,7 @@ guaranteed behavior end-to-end.
 
 | File | What's covered |
 |---|---|
-| `tests/Unit/Services/DoctrineTransactionServiceTest.php` | 26 tests: root/nested routing, savepoints never enabled/queried, fail-fast on a closed `EntityManager`, no retry after an ambiguous commit failure, refusal of closed-EM re-entry while a transaction is still active, rollback failures never masking the original exception, `\Error` handled alongside `\Exception`, `em->clear()` on root failure. |
+| `tests/Unit/Services/DoctrineTransactionServiceTest.php` | 27 tests: root/nested routing, savepoints never enabled/queried, fail-fast on a closed `EntityManager`, no retry after an ambiguous commit failure, refusal of closed-EM re-entry while a transaction is still active, rollback failures never masking the original exception, broken manager/connection discarded when the rollback itself fails, `\Error` handled alongside `\Exception`, `em->clear()` on root failure. |
 
 ### Business-service outer/inner pairs (functional, real DB)
 
diff --git a/app/Services/Utils/DoctrineTransactionService.php b/app/Services/Utils/DoctrineTransactionService.php
index 259290d12..65357782e 100644
--- a/app/Services/Utils/DoctrineTransactionService.php
+++ b/app/Services/Utils/DoctrineTransactionService.php
@@ -39,6 +39,11 @@
  *    failure during COMMIT is ambiguous (the server may have already made
  *    the transaction durable), so re-executing the callback could duplicate
  *    every write and side effect
+ *  - when the rollback itself fails (or a commit-phase failure is
+ *    connection-level), the manager/connection pair is discarded and a
+ *    fresh manager is reset into the registry, so direct Registry
+ *    consumers (which have no reconnect path of their own) never keep
+ *    working against a dead handle
  *
  * Nested transaction (called while a DB transaction is already active):
  *  - uses Doctrine DBAL nesting counter (beginTransaction / commit)
@@ -211,13 +216,19 @@ private function failFastIfEntityManagerClosed($em, string $context): void
      * @param \Doctrine\DBAL\Connection $conn
      * @param \Throwable $cause
      * @param string $context
+     * @return bool true when the connection is clean (rolled back, or nothing to
+     *              roll back); false when the rollback attempt itself failed and
+     *              the connection state is unknown (dead handle, possibly a stuck
+     *              DBAL rollback-only flag - DBAL zeroes the nesting level before
+     *              the physical rollback and only clears the flag after it succeeds)
      */
-    private function safeRollback($conn, \Throwable $cause, string $context): void
+    private function safeRollback($conn, \Throwable $cause, string $context): bool
     {
         try {
             if ($conn->isTransactionActive()) {
                 $conn->rollBack();
             }
+            return true;
         } catch (\Throwable $rollbackError) {
             Log::warning(sprintf(
                 "DoctrineTransactionService::%s rollback failed after '%s': %s",
@@ -225,6 +236,42 @@ private function safeRollback($conn, \Throwable $cause, string $context): void
                 $cause->getMessage(),
                 $rollbackError->getMessage()
             ));
+            return false;
+        }
+    }
+
+    /**
+     * Best-effort destructive cleanup for a manager/connection pair left in an
+     * unknown or broken state (failed rollback, connection-level commit failure):
+     * closes the EntityManager, closes the physical connection and swaps a fresh
+     * manager into the registry so subsequent work on this process - including
+     * direct Registry consumers (repositories, serializers, queue jobs) reading
+     * OUTSIDE transaction(), which have no reconnect/retry path of their own -
+     * gets a live pair instead of a dead handle.
+     *
+     * Root-only: never call while an outer transaction still owns this
+     * connection. Never throws - it must not mask the exception that led here.
+     *
+     * @param \Doctrine\ORM\EntityManagerInterface $em
+     */
+    private function discardBrokenManager($em): void
+    {
+        try {
+            if ($em->isOpen()) {
+                $em->clear();
+                $em->close();
+            }
+        } catch (\Throwable $ignore) {
+        }
+
+        try {
+            $em->getConnection()->close();
+        } catch (\Throwable $ignore) {
+        }
+
+        try {
+            Registry::resetManager($this->manager_name);
+        } catch (\Throwable $ignore) {
         }
     }
 
@@ -245,7 +292,8 @@ private function runRootTransaction(Closure $callback, int $isolationLevel)
 
         while ($retry < self::MaxRetries) {
             $em = Registry::getManager($this->manager_name);
-            $commitStarted = false;
+            $commitStarted   = false;
+            $rollbackFailed  = false;
 
             try {
                 if (!$em->isOpen()) {
@@ -269,7 +317,7 @@ private function runRootTransaction(Closure $callback, int $isolationLevel)
                     $conn->commit();
                     return $result;
                 } catch (\Throwable $inner) {
-                    $this->safeRollback($conn, $inner, 'runRootTransaction');
+                    $rollbackFailed = !$this->safeRollback($conn, $inner, 'runRootTransaction');
                     // Root only: discard UnitOfWork state so pending persists/changesets
                     // from the failed callback cannot leak into the next transaction on
                     // this same EntityManager (phantom writes in catch-and-continue loops).
@@ -294,7 +342,11 @@ private function runRootTransaction(Closure $callback, int $isolationLevel)
                         "DoctrineTransactionService::runRootTransaction commit outcome unknown after '%s'; not retrying.",
                         $ex->getMessage()
                     ));
-                    if (!$em->isOpen()) {
+                    if ($rollbackFailed || $this->shouldReconnect($ex)) {
+                        // Connection-level failure: the physical handle is dead (or
+                        // its state unknown). Cleanup only - still never retry.
+                        $this->discardBrokenManager($em);
+                    } elseif (!$em->isOpen()) {
                         Registry::resetManager($this->manager_name);
                     }
                     throw $ex;
@@ -336,7 +388,12 @@ private function runRootTransaction(Closure $callback, int $isolationLevel)
 
                 Log::warning("DoctrineTransactionService::runRootTransaction rolling back TX");
                 Log::warning($ex);
-                if (!$em->isOpen()) {
+                if ($rollbackFailed) {
+                    // The rollback attempt itself failed: the manager/connection pair
+                    // is in an unknown state (dead handle, possibly a stuck DBAL
+                    // rollback-only flag) and would poison direct Registry consumers.
+                    $this->discardBrokenManager($em);
+                } elseif (!$em->isOpen()) {
                     // A failed flush closes the EM (ORM behavior); reset so direct
                     // Registry consumers (serializers, factories, workers) get a live
                     // manager instead of an EntityManagerClosed on their next operation.
@@ -348,7 +405,9 @@ private function runRootTransaction(Closure $callback, int $isolationLevel)
                 // reconnect handling above, which typehints shouldReconnect(\Exception).
                 // They are never retried, but a callback that closed the EM before
                 // throwing one must still leave a live manager in the registry.
-                if (!$em->isOpen()) {
+                if ($rollbackFailed) {
+                    $this->discardBrokenManager($em);
+                } elseif (!$em->isOpen()) {
                     Registry::resetManager($this->manager_name);
                 }
                 throw $throwable;
diff --git a/tests/Unit/Services/DoctrineTransactionServiceTest.php b/tests/Unit/Services/DoctrineTransactionServiceTest.php
index 7d32a5328..5b87e118b 100644
--- a/tests/Unit/Services/DoctrineTransactionServiceTest.php
+++ b/tests/Unit/Services/DoctrineTransactionServiceTest.php
@@ -918,10 +918,15 @@ public function testRootTransactionThrowsAfterMaxRetries(): void
      * the whole callback and duplicate every write and side effect (orders,
      * tickets, emails), so commit-phase failures must bypass the reconnect/retry
      * path and propagate immediately.
+     *
+     * Because the failure is connection-level, the dead physical handle must not
+     * stay registered either: the manager/connection pair is discarded (closed +
+     * resetManager) so subsequent work on this worker - including direct Registry
+     * reads outside transaction() - gets a live pair. Cleanup only: still no retry.
      */
     public function testRootTransactionDoesNotRetryWhenCommitFails(): void
     {
-        [$em, $conn] = $this->buildMocks(transactionActive: false);
+        [$em, $conn, $registry] = $this->buildMocks(transactionActive: false);
 
         // DBAL decrements the nesting level in a finally block even when the
         // physical COMMIT fails, so by the time the exception is caught no
@@ -930,6 +935,10 @@ public function testRootTransactionDoesNotRetryWhenCommitFails(): void
             ->once()
             ->andThrow(new TestRetryableException('server has gone away during COMMIT'));
         $conn->shouldReceive('rollBack')->never();
+        $conn->shouldReceive('close')->once();
+
+        $em->shouldReceive('close')->once();
+        $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($em);
 
         $callCount = 0;
         $service = new DoctrineTransactionService('default');
@@ -945,6 +954,60 @@ public function testRootTransactionDoesNotRetryWhenCommitFails(): void
         }
     }
 
+    /**
+     * When the ROOT rollback itself fails (typically the connection died during
+     * the callback), safeRollback() must keep the original business exception -
+     * but the manager/connection pair is now in an unknown state: DBAL zeroes the
+     * nesting level before the physical rollback and only clears its rollback-only
+     * flag after it succeeds, so the registry would otherwise keep an open EM
+     * wired to a dead handle. Direct Registry consumers (repositories, serializers,
+     * queue jobs reading outside transaction()) have no retry path and would fail
+     * in a chain on a long-lived worker. The pair must be discarded: EM closed,
+     * connection closed, fresh manager reset into the registry - while the
+     * ORIGINAL exception still propagates and no retry happens.
+     */
+    public function testRootTransactionDiscardsManagerWhenRollbackFails(): void
+    {
+        $conn = Mockery::mock(Connection::class);
+        // false: routing check picks the ROOT path; true: rollback check in the inner catch
+        $conn->shouldReceive('isTransactionActive')->andReturn(false, true);
+        $conn->shouldReceive('setTransactionIsolation')->byDefault();
+        $conn->shouldReceive('beginTransaction')->once();
+        $conn->shouldReceive('commit')->never();
+        $conn->shouldReceive('rollBack')
+            ->once()
+            ->andThrow(new TestRetryableException('connection died during rollback'));
+        $conn->shouldReceive('close')->once();
+
+        $em = Mockery::mock(EntityManagerInterface::class);
+        $em->shouldReceive('getConnection')->andReturn($conn)->byDefault();
+        // Stays open throughout: a business error is not a flush failure
+        $em->shouldReceive('isOpen')->andReturn(true);
+        $em->shouldReceive('flush')->never();
+        $em->shouldReceive('clear')->byDefault();
+        $em->shouldReceive('close')->once();
+
+        $registry = Mockery::mock(ManagerRegistry::class);
+        $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault();
+        $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($em);
+
+        $this->container->instance(ManagerRegistry::class, $registry);
+
+        $callCount = 0;
+        $service = new DoctrineTransactionService('default');
+
+        try {
+            $service->transaction(function () use (&$callCount) {
+                $callCount++;
+                throw new \LogicException('business error with dying connection');
+            });
+            $this->fail('Expected the original business exception to propagate');
+        } catch (\LogicException $e) {
+            $this->assertStringContainsString('business error with dying connection', $e->getMessage());
+        }
+        $this->assertSame(1, $callCount, 'A rollback failure must never trigger the retry loop');
+    }
+
     /**
      * Nested transaction that returns null — ensures null is properly propagated.
      * Covers the SummitPromoCodeService pattern where inner TX returns null