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

@coderabbitai coderabbitai Bot Apr 20, 2026

Copy link
Copy Markdown

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

@coderabbitai coderabbitai Bot Apr 20, 2026 •
edited
Loading

Copy link
Copy Markdown

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.

@smarcet smarcet Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@coderabbitai The suggested fix cannot be applied on the DBAL version this repo pins (3.9.4): Connection::setNestTransactionsWithSavepoints() throws ConnectionException::mayNotAlterNestedTransactionWithSavepointsInTransaction() whenever transactionNestingLevel > 0 (see dbal/src/Connection.php:1346-1348), and runNestedTransaction() only executes while a transaction is active — that is its routing condition — so committing this suggestion would make every nested transaction fail immediately with that exception. Verified empirically against the vendored DBAL 3.9.4.

The underlying concern is also narrower than stated: the savepoint flag is sticky per Connection instance and runRootTransaction() always sets it before beginTransaction(), so any nested call whose outer transaction went through this service already runs with savepoints enabled (same EntityManager → same connection instance). The gap only opens when an outer transaction is started outside this service (wrapInTransaction(), raw beginTransaction()), and there are no such call sites under app/ today.

Instead of the suggested (throwing) set call, we are adding a read-only detection guard — getNestTransactionsWithSavepoints() is legal mid-transaction — that logs a warning when savepoints are off, so an externally-started outer transaction surfaces immediately rather than failing later as an opaque "rollback only" error on the root commit:

if (!$conn->getNestTransactionsWithSavepoints()) {
    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.');
}

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd -p 'vendor/doctrine/dbal/src/Connection.php' 2>/dev/null
rg -n 'function setNestTransactionsWithSavepoints|mayNotAlterNestedTransactionWithSavepointsInTransaction|transactionNestingLevel' vendor/doctrine/dbal/src/Connection.php 2>/dev/null | head -50

Length of output: 5223

Confirmed via the vendored DBAL source — setNestTransactionsWithSavepoints() at Line 1346 does throw mayNotAlterNestedTransactionWithSavepointsInTransaction() when transactionNestingLevel > 0, and since runNestedTransaction() only executes when a transaction is already active (per the routing check in transaction()), calling that setter there would indeed break every nested call. My original suggestion was incorrect for this DBAL version.

Your reasoning about the sticky per-connection flag also holds: since runRootTransaction() sets setNestTransactionsWithSavepoints(true) before beginTransaction(), and the connection instance is shared via the same EntityManager, any nested call originating from this service already inherits savepoints. The read-only getNestTransactionsWithSavepoints() guard with a warning log is the right call for surfacing the edge case of externally-started transactions without risking a hard failure. Thanks for verifying this empirically against 3.9.4 — withdrawing the original suggestion.

🧬 Code Graph Analysis Results

app/Services/Utils/DoctrineTransactionService.php

Savepoints intent (docblock)

Lines 30-47

/**
 * 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.
 */

Root vs nested routing (transaction method)

Lines 122-135

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

Where savepoints are enabled for root transactions

Lines 146-175

private function runRootTransaction(Closure $callback, int $isolationLevel)
{
    $retry = 0;

    while ($retry < self::MaxRetries) {
        $em = Registry::getManager($this->manager_name);

        try {
            if (!$em->isOpen()) {
                Log::warning("DoctrineTransactionService::runRootTransaction: entity manager is closed, resetting...");
                $em = Registry::resetManager($this->manager_name);
            }

            $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;
            }

Nested transaction behavior (no savepoint flag set here)

Lines 221-250

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;
    }
}
✏️ Learnings added 
Learnt from: smarcet
Repo: OpenStackweb/summit-api PR: 533
File: app/Services/Utils/DoctrineTransactionService.php:231-237
Timestamp: 2026-07-10T01:52:23.875Z
Learning: In `app/Services/Utils/DoctrineTransactionService.php`, do not call `$conn->setNestTransactionsWithSavepoints(true)` inside `runNestedTransaction()`. On the pinned Doctrine DBAL version (3.9.4), `Connection::setNestTransactionsWithSavepoints()` throws `ConnectionException::mayNotAlterNestedTransactionWithSavepointsInTransaction()` whenever `transactionNestingLevel > 0`, and `runNestedTransaction()` only runs while a transaction is already active, so calling the setter there breaks every nested transaction. The savepoint flag is sticky per DBAL `Connection` instance; `runRootTransaction()` already calls `setNestTransactionsWithSavepoints(true)` before `beginTransaction()`, so nested calls sharing the same EntityManager/connection inherit it. For nested transactions, use the read-only `$conn->getNestTransactionsWithSavepoints()` to detect and log a warning if savepoints are disabled (e.g., when an outer transaction was started outside this service), rather than attempting to set the flag mid-transaction.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

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

@coderabbitai coderabbitai Bot Apr 20, 2026

Copy link
Copy Markdown

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