Skip to content

fix(transactions): prevent nested transaction from destroying outer EntityManager on connection error #533

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
smarcet wants to merge 1 commit into
base: main
Choose a base branch
from
+975 −68
Open

fix(transactions): prevent nested transaction from destroying outer EntityManager on connection error #533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 131 additions & 68 deletions app/Services/Utils/DoctrineTransactionService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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){
Expand Down Expand Up @@ -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)) {
Comment on lines +164 to +178
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Inspect Doctrine DBAL version and transaction API surface without executing repo code.

composer show doctrine/dbal doctrine/orm 2>/dev/null || true

php -r '
$lock = json_decode(file_get_contents("composer.lock"), true);
foreach ($lock["packages"] ?? [] as $pkg) {
    if (in_array($pkg["name"], ["doctrine/dbal", "doctrine/orm"], true)) {
        echo $pkg["name"], " ", $pkg["version"], PHP_EOL;
    }
}
' 2>/dev/null || true

Repository: OpenStackweb/summit-api

Length of output: 1109

🏁 Script executed:

cat -n app/Services/Utils/DoctrineTransactionService.php

Repository: OpenStackweb/summit-api

Length of output: 10359

Do not retry after an ambiguous commit() failure.

Line 167: commit() can throw an exception after the database has already committed but before the client receives the response. Because this exception flows into the reconnect retry path at line 178, the entire callback will execute again, duplicating writes and side effects.

Track when commit() has started and bypass retry logic for commit-phase failures:

🐛 Suggested guard for ambiguous commit failures 
+                $commitStarted = false;
                 try {
                     $result = $callback($this);
                     $em->flush();
+                    $commitStarted = true;
                     $conn->commit();
                     return $result;
                 } catch (\Throwable $inner) {
-                    if ($conn->isTransactionActive()) {
+                    if (!$commitStarted && $conn->isTransactionActive()) {
                         $conn->rollBack();
                     }
                     throw $inner;
                 }
             } catch (Exception $ex) {
                 $retry++;
 
+                if (isset($commitStarted) && $commitStarted) {
+                    Log::error("DoctrineTransactionService::runRootTransaction commit outcome is unknown; not retrying.");
+                    throw $ex;
+                }
+
                 if ($this->shouldReconnect($ex)) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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)) {
$commitStarted = false;
try {
$result = $callback($this);
$em->flush();
$commitStarted = true;
$conn->commit();
return $result;
} catch (\Throwable $inner) {
if (!$commitStarted && $conn->isTransactionActive()) {
$conn->rollBack();
}
throw $inner;
}
} catch (Exception $ex) {
$retry++;
if (isset($commitStarted) && $commitStarted) {
Log::error("DoctrineTransactionService::runRootTransaction commit outcome is unknown; not retrying.");
throw $ex;
}
if ($this->shouldReconnect($ex)) {
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@app/Services/Utils/DoctrineTransactionService.php` around lines 164 - 178,
The retry path is incorrectly triggered for exceptions thrown during commit(),
causing duplicate callback execution; add a commit-phase guard (e.g. a boolean
$commitStarted or $inCommitPhase) in DoctrineTransactionService around the call
to $conn->commit() inside the transaction block (set it true immediately before
calling commit and false only after success) and update the outer catch that
inspects shouldReconnect($ex) so that if the exception occurred while
$commitStarted is true you do not retry/re-execute the callback but instead
rethrow/propagate the commit failure immediately; reference the inner
transaction block where $result = $callback($this), $em->flush(),
$conn->commit() occur and the outer catch handling $ex and shouldReconnect().

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();

Comment on lines +231 to +237
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Confirm DBAL version and whether Connection exposes the savepoint nesting method.

composer show doctrine/dbal 2>/dev/null || true

php -r '
$lock = json_decode(file_get_contents("composer.lock"), true);
foreach ($lock["packages"] ?? [] as $pkg) {
    if ($pkg["name"] === "doctrine/dbal") {
        echo "doctrine/dbal ", $pkg["version"], PHP_EOL;
        break;
    }
}
' 2>/dev/null || true

rg -nP 'function\s+setNestTransactionsWithSavepoints\b|setNestTransactionsWithSavepoints\s*\(' -C2

Repository: OpenStackweb/summit-api

Length of output: 2121

🏁 Script executed:

# Read the DoctrineTransactionService.php file to see the full runNestedTransaction() method
wc -l app/Services/Utils/DoctrineTransactionService.php

Repository: OpenStackweb/summit-api

Length of output: 120

🏁 Script executed:

# Read the complete runNestedTransaction method (lines around 231-237 and beyond)
sed -n '230,280p' app/Services/Utils/DoctrineTransactionService.php

Repository: OpenStackweb/summit-api

Length of output: 670

🏁 Script executed:

# Also check the root transaction method around line 160 for comparison
sed -n '150,170p' app/Services/Utils/DoctrineTransactionService.php

Repository: OpenStackweb/summit-api

Length of output: 940

🏁 Script executed:

# Search for all calls to setNestTransactionsWithSavepoints in the file
rg -n 'setNestTransactionsWithSavepoints' app/Services/Utils/DoctrineTransactionService.php

Repository: OpenStackweb/summit-api

Length of output: 207

Ensure savepoints are enabled before nested beginTransaction().

The nested transaction path (line 236) does not enable savepoints before calling beginTransaction(), unlike the root transaction path (line 160) which explicitly calls setNestTransactionsWithSavepoints(true). When an outer transaction is opened outside DoctrineTransactionService, the nested beginTransaction() may not use savepoints, risking transaction poisoning on nested rollback.

Add the savepoint configuration to the nested path:

Suggested fix 
     private function runNestedTransaction(Closure $callback)
     {
         $em   = Registry::getManager($this->manager_name);
         $conn = $em->getConnection();

+        $conn->setNestTransactionsWithSavepoints(true);
         $conn->beginTransaction();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private function runNestedTransaction(Closure $callback)
{
$em = Registry::getManager($this->manager_name);
$conn = $em->getConnection();
$conn->beginTransaction();
private function runNestedTransaction(Closure $callback)
{
$em = Registry::getManager($this->manager_name);
$conn = $em->getConnection();
$conn->setNestTransactionsWithSavepoints(true);
$conn->beginTransaction();
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@app/Services/Utils/DoctrineTransactionService.php` around lines 231 - 237,
The nested transaction path in runNestedTransaction does not enable savepoints
before calling beginTransaction, which can cause nested rollbacks to poison
outer transactions; update the runNestedTransaction implementation (the method
named runNestedTransaction and its use of $em and $conn) to call
$em->getConnection()->setNestTransactionsWithSavepoints(true) (or
$conn->setNestTransactionsWithSavepoints(true)) prior to
$conn->beginTransaction(), mirroring the root transaction path where
setNestTransactionsWithSavepoints(true) is set before beginning the transaction
so savepoints are used for nested transactions.

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;
Comment on lines +243 to +248
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Nested rollback does not revert Doctrine ORM in-memory changes.

A savepoint rollback only restores DB state; it does not undo mutations already made to managed entities in the EntityManager. If the outer callback catches this exception and continues, the root flush() can still persist changes from the failed nested callback. Please either disallow catch-and-continue after mutating nested callbacks, refresh/clear affected entities explicitly, or document/enforce a safe pattern.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@app/Services/Utils/DoctrineTransactionService.php` around lines 243 - 248,
The catch block in DoctrineTransactionService currently rolls back the DB
transaction but leaves the EntityManager’s in-memory state intact (catch
(\Throwable $ex) ... $conn->rollBack(); throw $ex;), which can allow failed
nested mutations to be persisted later; after performing $conn->rollBack()
update the catch to also reset or refresh the ORM state—either call the
EntityManager reset/close/clear (e.g. $this->entityManager->clear() or via
ManagerRegistry->resetManager()) or explicitly refresh/clear the specific
managed entities mutated by the nested callback—then rethrow; alternatively
enforce/document that nested callbacks must not be caught-and-continued so root
callers recreate a fresh EntityManager.

}
}
}
}
Loading
Loading