fix(transactions): prevent nested transaction from destroying outer EntityManager on connection error#533
Open
fix(transactions): prevent nested transaction from destroying outer EntityManager on connection error#533
Conversation
…ntityManager on connection error
📝 WalkthroughWalkthroughThis refactoring separates transaction handling into root and nested execution paths. Root transactions enable savepoints, support retries for reconnectable failures with destructive recovery, and explicitly manage lifecycle. Nested transactions execute without retries or resource cleanup, deferring error handling to the root. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Service as DoctrineTransactionService
participant Conn as DBAL Connection
participant EM as EntityManager
participant Callback as User Callback
Caller->>Service: transaction(callback, isolationLevel)
Service->>Conn: isTransactionActive()?
alt Root Transaction (No Active)
Conn-->>Service: false
Service->>Service: runRootTransaction()
Service->>Conn: setNestTransactionsWithSavepoints(true)
Service->>Conn: setTransactionIsolation(level)
Service->>Conn: beginTransaction()
Service->>Callback: execute callback
rect rgba(76, 175, 80, 0.5)
Callback-->>Service: result/exception
end
alt Success
Service->>EM: flush()
Service->>Conn: commit()
Service-->>Caller: result
else Non-Retryable Exception
Service->>Conn: rollback()
Service-->>Caller: exception
else Retryable Exception
loop Until Success or MaxRetries
Service->>EM: clear()
Service->>EM: close()
Service->>Service: resetManager()
Service->>Conn: beginTransaction()
Service->>Callback: retry callback
end
end
else Nested Transaction (Active)
Conn-->>Service: true
Service->>Service: runNestedTransaction()
Service->>Conn: beginTransaction() [nested level]
Service->>Callback: execute callback
rect rgba(76, 175, 80, 0.5)
Callback-->>Service: result/exception
end
alt Success
Service->>EM: flush()
Service->>Conn: commit() [nested level]
Service-->>Caller: result
else Exception
Service->>Conn: rollback() [nested level]
Service-->>Caller: exception
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-533/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/Unit/Services/DoctrineTransactionServiceTest.php (1)
74-80: Fix the helper array-shape annotation.
buildMocks()returns three values, but the PHPDoc only declares two. Please include the registry in the shape so destructuring and static analysis stay accurate.♻️ Suggested PHPDoc update
- * `@return` array{EntityManagerInterface&\Mockery\MockInterface, Connection&\Mockery\MockInterface} + * `@return` array{ + * EntityManagerInterface&\Mockery\MockInterface, + * Connection&\Mockery\MockInterface, + * ManagerRegistry&\Mockery\MockInterface + * }Also applies to: 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Services/DoctrineTransactionServiceTest.php` around lines 74 - 80, The PHPDoc for buildMocks() declares only two return elements but the method actually returns three (EntityManager, Connection, and the Registry), so update the `@return` array shape to include the third element as RegistryInterface&\Mockery\MockInterface (e.g. array{EntityManagerInterface&\Mockery\MockInterface, Connection&\Mockery\MockInterface, RegistryInterface&\Mockery\MockInterface}) so destructuring and static analysis are correct; apply the same correction to the other identical PHPDoc occurrence (the second buildMocks annotation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Services/Utils/DoctrineTransactionService.php`:
- Around line 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.
- Around line 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().
- Around line 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.
In `@tests/Unit/Services/DoctrineTransactionServiceTest.php`:
- Around line 775-783: The test currently asserts $callCount after calling
$service->transaction while expecting TestRetryableException, so that assertion
is never reached; wrap the transaction call in a try/catch (catching
TestRetryableException) and inside the catch assert that $callCount ===
DoctrineTransactionService::MaxRetries, then rethrow or let the test still
satisfy expectException; alternatively remove expectException and assert the
exception was thrown and $callCount equals
DoctrineTransactionService::MaxRetries within the caught exception path to
verify the retry behavior of the transaction() callback.
---
Nitpick comments:
In `@tests/Unit/Services/DoctrineTransactionServiceTest.php`:
- Around line 74-80: The PHPDoc for buildMocks() declares only two return
elements but the method actually returns three (EntityManager, Connection, and
the Registry), so update the `@return` array shape to include the third element as
RegistryInterface&\Mockery\MockInterface (e.g.
array{EntityManagerInterface&\Mockery\MockInterface,
Connection&\Mockery\MockInterface, RegistryInterface&\Mockery\MockInterface}) so
destructuring and static analysis are correct; apply the same correction to the
other identical PHPDoc occurrence (the second buildMocks annotation).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d750a232-4142-4f82-b785-2fcb08e10a67
📒 Files selected for processing (2)
app/Services/Utils/DoctrineTransactionService.phptests/Unit/Services/DoctrineTransactionServiceTest.php
Comment on lines
+164
to
+178
| 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)) { |
There was a problem hiding this comment.
🧩 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 || trueRepository: OpenStackweb/summit-api
Length of output: 1109
🏁 Script executed:
cat -n app/Services/Utils/DoctrineTransactionService.phpRepository: 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().
Comment on lines
+231
to
+237
| private function runNestedTransaction(Closure $callback) | ||
| { | ||
| $em = Registry::getManager($this->manager_name); | ||
| $conn = $em->getConnection(); | ||
|
|
||
| $conn->beginTransaction(); | ||
|
|
There was a problem hiding this comment.
🧩 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*\(' -C2Repository: 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.phpRepository: 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.phpRepository: 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.phpRepository: 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.phpRepository: 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.
Comment on lines
+243
to
+248
| } catch (\Throwable $ex) { | ||
| if ($conn->isTransactionActive()) { | ||
| $conn->rollBack(); | ||
| } | ||
| // No resetManager, no close(), no retry — let the root handle recovery. | ||
| throw $ex; |
There was a problem hiding this comment.
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.
Comment on lines
+775
to
+783
| $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); |
There was a problem hiding this comment.
Move the retry-count assertion into the exception path.
The assertion after transaction() is unreachable once the expected exception is thrown, so this test passes without proving MaxRetries attempts occurred.
🧪 Suggested test fix
- $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);
+ }📝 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
| $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); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Unit/Services/DoctrineTransactionServiceTest.php` around lines 775 -
783, The test currently asserts $callCount after calling $service->transaction
while expecting TestRetryableException, so that assertion is never reached; wrap
the transaction call in a try/catch (catching TestRetryableException) and inside
the catch assert that $callCount === DoctrineTransactionService::MaxRetries,
then rethrow or let the test still satisfy expectException; alternatively remove
expectException and assert the exception was thrown and $callCount equals
DoctrineTransactionService::MaxRetries within the caught exception path to
verify the retry behavior of the transaction() callback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/86b9gvnev
Summary by CodeRabbit
Improvements
Tests