Reset counter on reconnect or rollback failure

[abnegate]

Summary by CodeRabbit

• Bug Fixes
  • Improved database transaction handling to ensure multi-step operations complete reliably.
  • Strengthened error recovery so failed transactions reset internal state to prevent stuck transactions.
  • Reset connection state after reconnection to avoid lingering transaction state and improve stability.

[coderabbitai]

Walkthrough

Commit control flow was restructured to consolidate the nested-transaction check; rollback now resets the inTransaction counter on PDOExceptions before rethrowing; reconnect clears inTransaction after re-establishing the PDO connection.

Changes

Cohort / File(s)	Change Summary
Transaction state updates src/Database/Adapter/SQL.php	Reworked commitTransaction to remove an early guard and perform the inTransaction > 1 decrement/return in a single post-PDO check; on rollbackTransaction a caught PDOException now sets inTransaction = 0 before rethrowing as DatabaseException; reconnect resets inTransaction to 0 after restoring PDO.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant SQL as SQL Adapter
  participant PDO
  Note over SQL: commitTransaction (new flow)
  Caller->>SQL: commitTransaction()
  SQL->>PDO: check PDO inTransaction(true?)
  alt PDO not in transaction
    SQL->>SQL: decrement inTransaction? (if >1) and return true
  else PDO in transaction
    SQL->>PDO: commit()
    SQL->>SQL: decrement inTransaction and return result
  end

Loading

sequenceDiagram
  participant Caller
  participant SQL as SQL Adapter
  participant PDO
  Note over SQL: rollbackTransaction error path
  Caller->>SQL: rollbackTransaction()
  SQL->>PDO: rollback()
  alt PDOException thrown
    SQL->>SQL: set inTransaction = 0
    SQL-->>Caller: throw DatabaseException
  else success
    SQL->>SQL: set inTransaction appropriately
    SQL-->>Caller: return true
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Areas to inspect:
  • commitTransaction post-check logic for correct handling of nested transactions
  • rollbackTransaction exception paths to ensure inTransaction reset is correct and safe
  • reconnect side-effect to confirm no pending transaction assumptions are violated

Possibly related PRs

• With transactions optimization #677 — Appears to modify transaction handling in the same src/Database/Adapter/SQL.php (start/rollback/commit flow and inTransaction resets).

Poem

🐇 I hopped through code with careful paws,

Counters trimmed without a cause.
When rollbacks shout and sockets mend,
I zero out and greet the end.
— A cheerful rabbit in the schema garden 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "Reset counter on reconnect or rollback failure" accurately reflects two significant changes in the changeset: the inTransaction counter being reset to 0 after reconnection and being reset to 0 when a PDOException occurs during rollback. The title is specific, concise, and directly related to real, substantive modifications present in the code. While the changeset also includes a CommitTransaction flow adjustment, the title appropriately focuses on the most impactful behavioral changes, which aligns with the expectation that titles don't need to cover every detail of a changeset.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-rollbacks

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6207355 and 00b6877.

📒 Files selected for processing (1)

• src/Database/Adapter/SQL.php (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/Database/Adapter/SQL.php

⏰ 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). (14)

• GitHub Check: Adapter Tests (Pool)
• GitHub Check: Adapter Tests (SharedTables/Postgres)
• GitHub Check: Adapter Tests (SharedTables/SQLite)
• GitHub Check: Adapter Tests (Schemaless/MongoDB)
• GitHub Check: Adapter Tests (SharedTables/MySQL)
• GitHub Check: Adapter Tests (SharedTables/MongoDB)
• GitHub Check: Adapter Tests (SharedTables/MariaDB)
• GitHub Check: Adapter Tests (Mirror)
• GitHub Check: Adapter Tests (Postgres)
• GitHub Check: Adapter Tests (MySQL)
• GitHub Check: Adapter Tests (SQLite)
• GitHub Check: Adapter Tests (MariaDB)
• GitHub Check: Adapter Tests (MongoDB)
• GitHub Check: Unit Test

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/Database/Adapter/SQL.php (1)

101-102: Consider resetting counter on commit failure for consistency.

For consistency with the rollback error handling (line 130), consider resetting inTransaction to 0 when commit fails. If a commit throws a PDOException, the transaction state is uncertain (typically rolled back by the database), and maintaining the counter could lead to inconsistent state tracking.

Apply this diff to improve consistency:

     try {
         $result = $this->getPDO()->commit();
         $this->inTransaction = 0;
     } catch (PDOException $e) {
+        $this->inTransaction = 0;
         throw new TransactionException('Failed to commit transaction: ' . $e->getMessage(), $e->getCode(), $e);
     }

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4bbd108 and 6207355.

⛔ Files ignored due to path filters (1)

• composer.lock is excluded by !**/*.lock

📒 Files selected for processing (1)

• src/Database/Adapter/SQL.php (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/Database/Adapter/SQL.php (1)

src/Database/Adapter.php (1)

• inTransaction (366-369)

🔇 Additional comments (3)

src/Database/Adapter/SQL.php (3)

93-96: LGTM! Simplified nested transaction handling.

The control flow simplification correctly handles nested transactions by decrementing the counter and returning early when inTransaction > 1, deferring the actual PDO commit to the outermost transaction level.

---

130-130: LGTM! Correct error handling for rollback failure.

Resetting inTransaction to 0 when rollback fails is essential because the transaction state becomes uncertain after a rollback exception. This prevents the adapter from maintaining stale transaction state.

---

154-154: LGTM! Essential counter reset after reconnection.

Resetting inTransaction to 0 after reconnecting is critical because the new database connection has no transaction state from the previous connection. This prevents the adapter from incorrectly assuming an active transaction exists.