Skip to content

fix: keep writes transactional after MySQL deadlock 1213 - #10651

Open
KevinBlakeLopez wants to merge 3 commits into
codeigniter4:developfrom
KevinBlakeLopez:fix/10649-innodb-deadlock-1213
Open

KevinBlakeLopez wants to merge 3 commits into
codeigniter4:developfrom
KevinBlakeLopez:fix/10649-innodb-deadlock-1213

Conversation

@KevinBlakeLopez

@KevinBlakeLopez KevinBlakeLopez commented Oct 22, 2026 •

Copy link
Copy Markdown

Description

Fixes #10649.

InnoDB errno 1213 rolls the victim transaction back. CodeIgniter left transDepth at 1, so the next write on that connection autocommitted. transComplete() returned false and did not remove that write. transCommit() called mysqli::commit(), which returns true, and reported success while the victim row was already gone.

This is only errno 1213. Errno 1205 is not handled here, because innodb_rollback_on_timeout decides whether InnoDB rolled that transaction back.

After 1213, the next statement on that connection starts a new server transaction, so it does not autocommit. transCommit() returns false without calling mysqli::commit(). transComplete() still sees the failed status and rolls the new server transaction back.

On develop (df8b96bee) the new live tests failed before this change. transCommit():

-    'visible_before' => [],
-    'finished' => false,
-    'visible_after' => [],
+    'visible_before' => [102],
+    'finished' => true,
+    'visible_after' => [102],

transComplete() already returned false, and the following row stayed visible:

-    'visible_before' => [],
+    'visible_before' => [102],
     'finished' => false,
-    'visible_after' => [],
+    'visible_after' => [102],

The victim id 101 was absent in both runs. After the change, DeadlockTransactionTest passes, and the existing TransactionTest, TransactionDBDebugTrueTest, and TransactionDBDebugFalseTest still pass (local MySQL 8.0.45, PHP 8.5.12, MySQLi).

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

Made with Cursor

@carson-codeigniter4 carson-codeigniter4 Bot added the bug Verified issues on the current code behavior or pull requests that will fix them label Oct 22, 2026
@carson-codeigniter4

Copy link
Copy Markdown

Hi there, @KevinBlakeLopez! 👋

Thank you for sending this PR!

We expect the following in all Pull Requests (PRs).

Important

We expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works.

If pull requests do not comply with the above, they will likely be closed. Since we are a team of volunteers, we don't have any more time to work
on the framework than you do. Please make it as painless for your contributions to be included as possible.

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md

KevinBlakeLopez and others added 2 commits October 22, 2026 16:35
InnoDB rolls the victim transaction back and mysqli::commit() still
returns true. The next statement now starts a new server transaction,
and transCommit() does not report success.

Fixes codeigniter4#10649

Co-authored-by: Cursor <cursoragent@cursor.com>
mysqli_sql_exception::getCode() is already an int. Rector rejects the redundant cast.

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

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.

🟡 Changes recommended

Prepared statements and failed transaction restarts can still allow subsequent writes to autocommit.

3 open issues
What changed in this PR

Addresses #10649 by tracking MySQLi transactions rolled back by deadlock error 1213, preventing later writes from autocommitting and avoiding false commit success. Reviewed under the assumed develop patch-release policy.

Changes:

  • Adds rollback tracking, deferred transaction restart, and commit protection.
  • Adds live MySQLi deadlock regression tests.
  • Documents recovery behavior while leaving error 1205 unchanged.

Validation used targeted source inspection; live database tests and full validation remain with CI.

File Description
user_guide_src/​source/​database/​transactions.rst Documents MySQL deadlock recovery behavior.
user_guide_src/​source/​changelogs/​v4.7.6.rst Records the errno 1213 fix.
tests/​system/​Database/​Live/​MySQLi/​DeadlockTransactionTest.php Adds deadlock and subsequent-write regression tests.
system/​Database/​MySQLi/​Connection.php Detects deadlocks and restarts server transactions.
system/​Database/​BaseConnection.php Tracks server rollback state and prevents false commit success.

Review effort: Balanced

Comment thread system/Database/MySQLi/Connection.php Outdated
Comment on lines +290 to +291
if ($this->connID instanceof mysqli && $this->consumeServerTransactionRestart()) {
$this->connID->begin_transaction();

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in 8089966. begin_transaction() now runs inside the try, and the restart flag is cleared only after it succeeds. A failed BEGIN goes through the normal error path, so query() returns false or throws DatabaseException as usual, and ping()/reconnect() work. testLostConnectionAfterDeadlockDoesNotEscapeAsDriverException kills the connection after the 1213 and covers it.

Same commit, one more change: when the server has rolled the transaction back, transCommit() now rolls back the restarted server transaction before returning false. Before, it left transDepth at 1, so a caller that retried without transRollback() nested inside the open transaction, got true from transCommit(), and nothing was committed. testFalseTransCommitEndsTheTransactionSoARetryCommits covers it.

Comment thread system/Database/MySQLi/Connection.php Outdated
Comment on lines +290 to +292
if ($this->connID instanceof mysqli && $this->consumeServerTransactionRestart()) {
$this->connID->begin_transaction();
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not changed in this PR. Prepared statements are left out on purpose to keep the change small. In 8089966 the guide and changelog now say so: the fix covers query(), simpleQuery() and the Query Builder, and prepared queries are not covered. PreparedQuery::_execute() can be handled in a separate PR.

{
for ($attempt = 1; $attempt <= 3; $attempt++) {
$this->releaseDeadlock();
$this->resetProbe($table);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in 8089966. resetProbe() records the table before recreating it, so teardown drops it, including when setup fails.

The restart BEGIN ran outside the query try block, after the restart flag
was cleared. If the connection dropped after errno 1213, query(), ping()
and reconnect() threw a raw mysqli_sql_exception, even with DBDebug false,
and the next write skipped BEGIN. BEGIN now runs inside the try, and the
restart stays pending until it succeeds.

transCommit() returned false and left transDepth at 1 with the restarted
server transaction open. A caller that retried without transRollback()
nested inside it, got true from transCommit(), and nothing was committed.
transCommit() now rolls that transaction back before returning false.

The guide and changelog now limit the fix to query(), simpleQuery() and
the Query Builder. Prepared queries are not covered.

The deadlock test records the probe table before recreating it, so
teardown drops it.

Refs codeigniter4#10649

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

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

Labels

bug Verified issues on the current code behavior or pull requests that will fix them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: MySQL errno 1213 leaves transDepth set so the next write autocommits

2 participants