With transactions optimization #677
Conversation
WalkthroughRefactors transaction handling. Adapter::withTransaction adds explicit retry loop with backoff and immediate bail-outs for specific exceptions, resetting inTransaction as needed. SQL adapter simplifies startTransaction and rollbackTransaction to direct PDO calls, adjusts inTransaction, and changes rollback exception wrapping to DatabaseException. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client Code
participant A as Adapter::withTransaction
participant DB as SQL Adapter / PDO
Note over A: New retry flow with backoff
C->>A: withTransaction(callback)
loop up to 3 attempts
A->>DB: startTransaction()
alt callback succeeds
A->>DB: commitTransaction()
A-->>C: return result
else callback throws
alt Non-retriable exception (Duplicate/Restricted/Auth/Relation/Conflict/Limit)
A->>DB: rollbackTransaction()
A-->>C: rethrow immediately
else Retriable failure
A->>DB: rollbackTransaction()
Note over A: usleep(50ms × (attempt+1))
end
end
end
A-->>C: throw TransactionException("Failed to execute transaction")
sequenceDiagram
autonumber
participant A as SQL Adapter
participant PDO as PDO
Note over A: Simplified start/rollback
A->>PDO: beginTransaction() or SAVEPOINT
A->>A: inTransaction++
A-->>A: return true
A->>PDO: ROLLBACK TO savepoint or rollBack()
A->>A: adjust inTransaction (decrement/reset)
A-->>A: return true
Note over A: On PDOException during rollback -> wrap as DatabaseException
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Adapter.php (2)
351-359: Doc contradicts implementation of nested rollbacksDoc says "roll back all transactions," but SQL adapters roll back only one level via SAVEPOINT. This discrepancy can mislead callers and breaks expectations in withTransaction.
Action: either update the doc to "single-level rollback (savepoint)" semantics or change adapters to roll back all levels. Given existing nested semantics elsewhere, updating the doc is likely correct.
379-421: Drain all nested transactions before retry or bail-outResetting $this->inTransaction to 0 without fully rolling back leaves the underlying connection in a transaction when callbacks used nested transactions. Ensure you drain to the top by looping rollbackTransaction until 0.
Apply this diff:
for ($attempts = 0; $attempts <= $retries; $attempts++) { try { $this->startTransaction(); $result = $callback(); $this->commitTransaction(); return $result; } catch (\Throwable $action) { try { - $this->rollbackTransaction(); + $this->rollbackTransaction(); + // Ensure we fully reset DB state (drain nested savepoints) + while ($this->inTransaction > 0) { + $this->rollbackTransaction(); + } if ( $action instanceof DuplicateException || $action instanceof RestrictedException || $action instanceof AuthorizationException || $action instanceof RelationshipException || $action instanceof ConflictException || $action instanceof LimitException ) { $this->inTransaction = 0; throw $action; } } catch (\Throwable $rollback) { if ($attempts < $retries) { \usleep($sleep * ($attempts + 1)); continue; } $this->inTransaction = 0; throw $rollback; } if ($attempts < $retries) { \usleep($sleep * ($attempts + 1)); continue; } $this->inTransaction = 0; throw $action; } }
🧹 Nitpick comments (3)
src/Database/Adapter/SQL.php (2)
59-72: Avoid unconditional ROLLBACK when no transaction is activeIssuing ROLLBACK outside an active transaction can error on some drivers. Since you already guard with inTransaction(), the else-branch is unnecessary and risky.
Apply this diff:
if ($this->getPDO()->inTransaction()) { $this->getPDO()->rollBack(); - } else { - // If no active transaction, this has no effect. - $this->getPDO()->prepare('ROLLBACK')->execute(); } $this->getPDO()->beginTransaction();
131-131: Use TransactionException for rollback errors for consistencystartTransaction/commitTransaction throw TransactionException. Align rollback to the same specialized exception.
- throw new DatabaseException('Failed to rollback transaction: ' . $e->getMessage(), $e->getCode(), $e); + throw new TransactionException('Failed to rollback transaction: ' . $e->getMessage(), $e->getCode(), $e);src/Database/Adapter.php (1)
379-381: Make backoff configurable (optional)Expose $retries and $sleep as adapter-level configurables (properties or setter) rather than hardcoding, so callers can tune per environment.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Database/Adapter.php(3 hunks)src/Database/Adapter/SQL.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Database/Adapter.php (4)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Conflict.php (1)
Conflict(7-9)src/Database/Exception/Relationship.php (1)
Relationship(7-9)src/Database/Exception/Restricted.php (1)
Restricted(7-9)
src/Database/Adapter/SQL.php (1)
src/Database/Adapter.php (1)
inTransaction(366-369)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (1)
src/Database/Adapter.php (1)
7-12: LGTM: granular bail-out importsAdding Authorization/Conflict/Relationship/Restricted/Limit exceptions enables clearer early exits. No concerns.
Summary by CodeRabbit
Bug Fixes
Refactor