Skip to content

feat: classify retryable transaction exceptions#10162

Open
memleakd wants to merge 2 commits intocodeigniter4:4.8from
memleakd:feat/retryable-transaction-exceptions
Open

feat: classify retryable transaction exceptions#10162
memleakd wants to merge 2 commits intocodeigniter4:4.8from
memleakd:feat/retryable-transaction-exceptions

Conversation

@memleakd
Copy link
Copy Markdown
Contributor

@memleakd memleakd commented May 6, 2026

Description

This PR proposes adding isRetryableTransactionException() to database connections.

The goal is to give CodeIgniter a small, driver-aware way to recognize transaction failures that are reasonable candidates for a whole-transaction retry, such as deadlocks and serialization failures.

This is useful on its own for applications that already want to implement their own retry policy:

try {
    $orderId = $db->transException(true)->transaction(static function ($db) use ($order) {
        $db->table('orders')->insert($order);

        return $db->insertID();
    });
} catch (DatabaseException $e) {
    if ($db->isRetryableTransactionException($e)) {
        // Retry the whole transaction according to the application's policy.
    }

    throw $e;
}

It is also intended as groundwork for a follow-up proposal to add opt-in retry support to the closure-based transaction() helper. Establishing this driver classification policy first keeps that later retry proposal smaller: which database errors CodeIgniter considers retryable can be discussed separately from retry control flow, attempt counts, delay/backoff behavior, and callback side-effect concerns.

The classifier is intentionally conservative. It only includes errors that represent transaction-level concurrency failures where retrying the whole transaction is a reasonable default.

This PR does not retry anything automatically. It only gives applications and future framework APIs a common way to ask: "is this database exception one of the transaction failures the framework considers retryable?"

API Shape Feedback

One intentional question in this PR is whether this classifier should be public now.

I proposed it as ConnectionInterface::isRetryableTransactionException() because it has standalone value for applications and packages that already want to implement their own retry policy, and it gives future framework retry behavior a shared driver-aware classification point.

That said, if the team would rather avoid exposing this as public API before automatic transaction retry support is discussed, I am happy to adjust the PR so the driver-specific classification stays internal/protected for now, and leave the public surface to a later transaction() retry proposal.

Feedback on this API shape would be very welcome.

Changes

  • Added ConnectionInterface::isRetryableTransactionException().
  • Added the default implementation in BaseConnection.
  • Added driver-specific retryable transaction classification.
  • Added focused tests for both retryable and intentionally non-retryable codes.
  • Updated the transaction docs and v4.8.0 changelog.

Driver Policy

Included by default:

  • MySQLi deadlock: 1213
  • Postgre serialization failure/deadlock: 40001, 40P01
  • SQLite busy: 5
  • SQLSRV deadlock/snapshot conflict: 1205, 3960
  • OCI8 deadlock/serialization failure: 60, 8177

Not included by default: lock timeouts, unique constraint violations, resource-busy errors, or SQLite extended busy codes. Those can be valid retry cases in some applications, but they are more policy-dependent.

References: MySQL, PostgreSQL, SQLite, SQL Server deadlocks, SQL Server 3960, Oracle ORA-00060, Oracle ORA-08177.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Signed-off-by: memleakd <121398829+memleakd@users.noreply.github.com>
@github-actions github-actions Bot added the 4.8 PRs that target the `4.8` branch. label May 6, 2026
@michalsn
Copy link
Copy Markdown
Member

michalsn commented May 7, 2026

Thanks for working on this. The goal makes sense. It's useful to flag transaction errors that are worth retrying, like deadlocks and serialization failures. However, I wonder if this should follow the same approach we recently added with UniqueConstraintViolationException in #9979.

Instead of adding a public method like $db->isRetryableTransactionException($e) could the drivers throw a clearer exception type? For example:

try {
    $result = $db->transException(true)->transaction(static function ($db) {
        // ...
    });
} catch (RetryableTransactionException $e) {
    // Retry the whole transaction
}

This seems doable because the drivers already inspect native database codes when creating UniqueConstraintViolationException. The same place could classify deadlocks or serialization failures and throw RetryableTransactionException instead of a plain DatabaseException.

Overall, this looks useful, especially the conservative list of retryable codes. My main suggestion is about the public API shape. I would prefer named exception over isRetryableTransactionException().

- Replace the public retryable transaction classifier with a named exception
- Classify retryable driver errors through the database exception factory
- Update docs and tests for exception-based retry handling

Signed-off-by: memleakd <121398829+memleakd@users.noreply.github.com>
@memleakd
Copy link
Copy Markdown
Contributor Author

memleakd commented May 7, 2026

Thanks a lot for pointing this out. That makes total sense, and I agree it's a better direction. I missed the previous UniqueConstraintViolationException work, so thank you for connecting it here.

I refactored the PR around that idea. The drivers now classify the known retryable transaction errors and throw RetryableTransactionException, and the docs/tests were updated to show the exception-based usage.

One thing I noticed while doing this is that prepared queries still have separate exception paths, similar to the existing unique-constraint behavior. I kept that out of this PR so it stays focused, but I'd be happy to work on prepared-query consistency as a follow-up after this one.

Comment on lines 330 to +332
$exception = $e->getCode() === 1062
? new UniqueConstraintViolationException($e->getMessage(), $e->getCode(), $e)
: new DatabaseException($e->getMessage(), $e->getCode(), $e);
: $this->createDatabaseException($e->getMessage(), $e->getCode(), $e);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One thing I think can be improved is, let $this->createDatabaseException handle all types of exception including UniqueConstraintViolation. Similar to isRetryableTransactionErrorCode, there could be a protected function like isUniqueConstraintViolation(int|string $code): bool and this function will be used by createDatabaseException function and it returns UniqueConstraintViolationException for that particular error code. In this way, we can avoid this ternary operator in all connection implementations, and in future, if we need to add more refined exceptions, we can do so just in BaseConnection without messy ternary operators

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, definitely a good idea.

@patel-vansh
Copy link
Copy Markdown
Contributor

Thanks a lot for pointing this out. That makes total sense, and I agree it's a better direction. I missed the previous UniqueConstraintViolationException work, so thank you for connecting it here.

I refactored the PR around that idea. The drivers now classify the known retryable transaction errors and throw RetryableTransactionException, and the docs/tests were updated to show the exception-based usage.

One thing I noticed while doing this is that prepared queries still have separate exception paths, similar to the existing unique-constraint behavior. I kept that out of this PR so it stays focused, but I'd be happy to work on prepared-query consistency as a follow-up after this one.

Also, please update the PR description as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4.8 PRs that target the `4.8` branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants