From 81f02e6b15ae88bcef2a80ca43ea432c9ec53765 Mon Sep 17 00:00:00 2001 From: Kevin Lopez Date: Thu, 22 Oct 2026 16:35:33 -0700 Subject: [PATCH 1/3] fix: keep writes transactional after MySQL deadlock 1213 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 #10649 Co-authored-by: Cursor --- system/Database/BaseConnection.php | 62 +++++- system/Database/MySQLi/Connection.php | 12 + .../Live/MySQLi/DeadlockTransactionTest.php | 207 ++++++++++++++++++ user_guide_src/source/changelogs/v4.7.6.rst | 4 + .../source/database/transactions.rst | 12 + 5 files changed, 296 insertions(+), 1 deletion(-) create mode 100644 tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 002cf661226d..193b3b5993e0 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -337,6 +337,20 @@ abstract class BaseConnection implements ConnectionInterface */ protected $transFailure = false; + /** + * The server has already rolled this transaction back (MySQLi errno 1213). + * mysqli::commit() still returns true, so transCommit() must not report success. + */ + protected bool $transServerRolledBack = false; + + /** + * The next statement must open a server transaction before it runs. + * InnoDB ended the previous one and the connection is back in autocommit. + * Errno 1205 does not set this: innodb_rollback_on_timeout decides whether + * that error rolled the transaction back. + */ + protected bool $transNeedsServerRestart = false; + /** * Whether to throw exceptions during transaction */ @@ -1022,6 +1036,7 @@ public function transBegin(bool $testMode = false): bool if ($this->_transBegin()) { $this->transDepth++; + $this->clearServerRolledBackTransaction(); return true; } @@ -1039,7 +1054,19 @@ public function transCommit(): bool } // When transactions are nested we only begin/commit/rollback the outermost ones - if ($this->transDepth > 1 || $this->_transCommit()) { + if ($this->transDepth > 1) { + $this->transDepth--; + + return true; + } + + // InnoDB already removed the victim transaction. mysqli::commit() + // returns true anyway, which would publish later writes and report success. + if ($this->transServerRolledBack) { + return false; + } + + if ($this->_transCommit()) { $this->transDepth--; return true; @@ -1061,6 +1088,10 @@ public function transRollback(): bool if ($this->transDepth > 1 || $this->_transRollback()) { $this->transDepth--; + if ($this->transDepth === 0) { + $this->clearServerRolledBackTransaction(); + } + return true; } @@ -1089,6 +1120,35 @@ public function handleTransStatus(): void } } + /** + * Remember that the server has rolled the open transaction back. + */ + protected function markServerRolledBackTransaction(): void + { + $this->transServerRolledBack = true; + $this->transNeedsServerRestart = true; + } + + /** + * True once, when the next statement must BEGIN before it runs. + */ + protected function consumeServerTransactionRestart(): bool + { + if (! $this->transNeedsServerRestart || $this->transDepth < 1) { + return false; + } + + $this->transNeedsServerRestart = false; + + return true; + } + + private function clearServerRolledBackTransaction(): void + { + $this->transServerRolledBack = false; + $this->transNeedsServerRestart = false; + } + /** * Begin Transaction */ diff --git a/system/Database/MySQLi/Connection.php b/system/Database/MySQLi/Connection.php index 10e56e944dd4..e47e1591a778 100644 --- a/system/Database/MySQLi/Connection.php +++ b/system/Database/MySQLi/Connection.php @@ -283,6 +283,14 @@ public function getVersion(): string */ protected function execute(string $sql) { + // Errno 1213 ends the server transaction. Begin again before this + // statement so it does not autocommit. Errno 1205 is not handled: + // innodb_rollback_on_timeout decides whether InnoDB rolled that + // transaction back. + if ($this->connID instanceof mysqli && $this->consumeServerTransactionRestart()) { + $this->connID->begin_transaction(); + } + while ($this->connID->more_results()) { $this->connID->next_result(); if ($res = $this->connID->store_result()) { @@ -300,6 +308,10 @@ protected function execute(string $sql) 'trace' => render_backtrace($e->getTrace()), ]); + if ((int) $e->getCode() === 1213 && $this->transDepth > 0) { + $this->markServerRolledBackTransaction(); + } + if ($this->DBDebug) { throw new DatabaseException($e->getMessage(), $e->getCode(), $e); } diff --git a/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php b/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php new file mode 100644 index 000000000000..b65b7a5b865a --- /dev/null +++ b/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php @@ -0,0 +1,207 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Database\Live\MySQLi; + +use CodeIgniter\Database\BaseConnection; +use CodeIgniter\Test\CIUnitTestCase; +use CodeIgniter\Test\DatabaseTestTrait; +use Config\Database; +use mysqli; +use PHPUnit\Framework\Attributes\Group; +use Throwable; + +/** + * InnoDB errno 1213 rolls the victim transaction back. These tests lock two + * rows in opposite orders so the CodeIgniter connection is that victim. + * + * Errno 1205 is a different outcome and is not a passing result here. + * + * @internal + */ +#[Group('DatabaseLive')] +final class DeadlockTransactionTest extends CIUnitTestCase +{ + use DatabaseTestTrait; + + protected $migrate = false; + private ?BaseConnection $waiter = null; + private ?mysqli $held = null; + private ?string $probeTable = null; + + protected function setUp(): void + { + $config = config('Database'); + + if ($config->tests['DBDriver'] !== 'MySQLi') { + $this->markTestSkipped('Only MySQLi can complete this test.'); + } + + $this->db = Database::connect($this->DBGroup, false); + + parent::setUp(); + } + + protected function tearDown(): void + { + $this->releaseDeadlock(); + + parent::tearDown(); + } + + public function testFollowingWriteDoesNotAutocommitAndTransCommitReturnsFalse(): void + { + $this->assertDeadlockVictimDoesNotPublishTheNextWrite(false); + } + + public function testFollowingWriteDoesNotAutocommitAndTransCompleteRollsItBack(): void + { + $this->assertDeadlockVictimDoesNotPublishTheNextWrite(true); + } + + private function assertDeadlockVictimDoesNotPublishTheNextWrite(bool $useComplete): void + { + $table = $this->db->escapeIdentifier($this->db->prefixTable('deadlock_probe')); + $this->probeTable = $table; + $this->forceDeadlock($table); + + $this->assertSame(1213, (int) $this->db->error()['code']); + $this->assertSame(1, $this->db->transDepth); + + $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (102, 'after')"); + + $visibleBefore = $this->committedIds($table); + $finished = $useComplete ? $this->db->transComplete() : $this->db->transCommit(); + $visibleAfter = $this->committedIds($table); + + $this->assertSame( + ['visible_before' => [], 'finished' => false, 'visible_after' => []], + [ + 'visible_before' => $visibleBefore, + 'finished' => $finished, + 'visible_after' => $visibleAfter, + ], + ); + } + + /** + * Make $this->db the InnoDB deadlock victim. The victim row is id 101. + */ + private function forceDeadlock(string $table): void + { + for ($attempt = 1; $attempt <= 3; $attempt++) { + $this->releaseDeadlock(); + $this->resetProbe($table); + + $this->waiter = Database::connect($this->DBGroup, false); + $this->waiter->initialize(); + $this->waiter->query('SET SESSION innodb_lock_wait_timeout = 3'); + $this->waiter->transStart(); + + for ($i = 0; $i < 20; $i++) { + $this->waiter->query('UPDATE ' . $table . " SET note = 'w{$i}' WHERE id = 1"); + } + + $held = $this->waiter->getConnection(); + $this->assertInstanceOf(mysqli::class, $held); + $this->held = $held; + + // The victim has to lock row 2 before the waiter asks for it. + // Sending that request first lets the waiter take row 2, and the + // victim then waits out innodb_lock_wait_timeout (errno 1205). + $this->db->query('SET SESSION innodb_lock_wait_timeout = 3'); + $this->db->transException(false); + $this->db->transStart(); + $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (101, 'victim')"); + $this->db->query('UPDATE ' . $table . " SET note = 'held' WHERE id = 2"); + + $this->held->query('UPDATE ' . $table . " SET note = 'waiting' WHERE id = 2", MYSQLI_ASYNC); + usleep(400000); + + $this->db->query('UPDATE ' . $table . " SET note = 'closing' WHERE id = 1"); + + if ((int) $this->db->error()['code'] === 1213) { + return; + } + } + + $this->fail('Precondition: InnoDB did not report errno 1213 on the CodeIgniter connection.'); + } + + private function resetProbe(string $table): void + { + if ($this->db->transDepth > 0) { + $this->db->transRollback(); + } + + $this->db->query('DROP TABLE IF EXISTS ' . $table); + $this->db->query( + 'CREATE TABLE ' . $table . ' (' + . 'id INT NOT NULL, ' + . 'note VARCHAR(32) NOT NULL, ' + . 'PRIMARY KEY (id)' + . ') ENGINE=InnoDB', + ); + $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (1, 'one'), (2, 'two')"); + } + + /** + * @return list + */ + private function committedIds(string $table): array + { + $observer = Database::connect($this->DBGroup, false); + $observer->initialize(); + $rows = $observer->query('SELECT id FROM ' . $table . ' WHERE id IN (101, 102) ORDER BY id')->getResultArray(); + $observer->close(); + + $ids = []; + + foreach ($rows as $row) { + $ids[] = (int) $row['id']; + } + + return $ids; + } + + private function releaseDeadlock(): void + { + if ($this->held instanceof mysqli) { + try { + $this->held->reap_async_query(); + } catch (Throwable) { + // The waiting statement already finished, or the socket is closed. + } + + $this->held = null; + } + + if ($this->waiter instanceof BaseConnection) { + if ($this->waiter->transDepth > 0) { + $this->waiter->transRollback(); + } + + $this->waiter->close(); + $this->waiter = null; + } + + if ($this->db instanceof BaseConnection && $this->db->transDepth > 0) { + $this->db->transRollback(); + } + + if ($this->probeTable !== null && $this->db instanceof BaseConnection) { + $this->db->query('DROP TABLE IF EXISTS ' . $this->probeTable); + $this->probeTable = null; + } + } +} diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index 8739e9546d4c..666f623d0e37 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -32,6 +32,10 @@ Deprecations Bugs Fixed ********** +- **Database:** Fixed MySQLi errno 1213 (``ER_LOCK_DEADLOCK``) so a following + write on the victim connection does not autocommit, and ``transCommit()`` + does not return true after InnoDB has rolled the victim transaction back. + Errno 1205 is unchanged. See `#10649 `_. - **Commands:** Fixed a bug where generator commands did not sort the imports of a template with CRLF line endings. - **Format:** Fixed ``XMLFormatter`` so that an array key that becomes empty after sanitization no longer triggers a ``ValueError``; the fallback tag name ``item`` diff --git a/user_guide_src/source/database/transactions.rst b/user_guide_src/source/database/transactions.rst index 5c48f2806240..aad742435242 100644 --- a/user_guide_src/source/database/transactions.rst +++ b/user_guide_src/source/database/transactions.rst @@ -145,6 +145,18 @@ if you would like to run transactions manually you can do so as follows: .. note:: Make sure to use ``$this->db->transBegin()`` when running manual transactions, **NOT** ``$this->db->transStart()``. +MySQL Deadlocks +=============== + +InnoDB error 1213 rolls the victim transaction back. CodeIgniter keeps that +transaction open. The next statement on the same connection starts a new +server transaction, so the write does not autocommit, and ``transCommit()`` +returns false. ``transComplete()`` rolls the new server transaction back. + +Error 1205 is not handled this way. ``innodb_rollback_on_timeout`` decides +whether InnoDB rolled that transaction back, and the message is the same +either way. + Nested Transactions =================== From 7750791d9b18bb35f8cb41da02aad228240ab0e3 Mon Sep 17 00:00:00 2001 From: Kevin Lopez Date: Thu, 22 Oct 2026 16:35:33 -0700 Subject: [PATCH 2/3] fix: compare the MySQL deadlock code without an int cast mysqli_sql_exception::getCode() is already an int. Rector rejects the redundant cast. Co-authored-by: Cursor --- system/Database/MySQLi/Connection.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Database/MySQLi/Connection.php b/system/Database/MySQLi/Connection.php index e47e1591a778..12a71d16a260 100644 --- a/system/Database/MySQLi/Connection.php +++ b/system/Database/MySQLi/Connection.php @@ -308,7 +308,7 @@ protected function execute(string $sql) 'trace' => render_backtrace($e->getTrace()), ]); - if ((int) $e->getCode() === 1213 && $this->transDepth > 0) { + if ($e->getCode() === 1213 && $this->transDepth > 0) { $this->markServerRolledBackTransaction(); } From 8089966b7fe33c1109ec99e522e29f930153de30 Mon Sep 17 00:00:00 2001 From: Kevin Lopez Date: Fri, 23 Oct 2026 06:45:45 -0700 Subject: [PATCH 3/3] fix: end the transaction when transCommit() returns false after 1213 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 #10649 Co-Authored-By: Claude Opus 5.5 --- system/Database/BaseConnection.php | 21 ++++-- system/Database/MySQLi/Connection.php | 18 ++--- .../Live/MySQLi/DeadlockTransactionTest.php | 72 ++++++++++++++++++- user_guide_src/source/changelogs/v4.7.6.rst | 7 +- .../source/database/transactions.rst | 12 +++- 5 files changed, 108 insertions(+), 22 deletions(-) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 193b3b5993e0..c6b7a573cf2c 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -1062,7 +1062,12 @@ public function transCommit(): bool // InnoDB already removed the victim transaction. mysqli::commit() // returns true anyway, which would publish later writes and report success. + // Roll back the restarted server transaction so a false return ends the + // transaction, as transComplete() does. Otherwise a caller that retries + // without transRollback() nests inside it and nothing commits. if ($this->transServerRolledBack) { + $this->transRollback(); + return false; } @@ -1130,17 +1135,19 @@ protected function markServerRolledBackTransaction(): void } /** - * True once, when the next statement must BEGIN before it runs. + * True while the next statement must BEGIN before it runs. */ - protected function consumeServerTransactionRestart(): bool + protected function serverTransactionRestartPending(): bool { - if (! $this->transNeedsServerRestart || $this->transDepth < 1) { - return false; - } + return $this->transNeedsServerRestart && $this->transDepth > 0; + } + /** + * BEGIN succeeded. Clear the restart only now, so a failed BEGIN keeps it pending. + */ + protected function serverTransactionRestarted(): void + { $this->transNeedsServerRestart = false; - - return true; } private function clearServerRolledBackTransaction(): void diff --git a/system/Database/MySQLi/Connection.php b/system/Database/MySQLi/Connection.php index 12a71d16a260..c8bb0aa3d941 100644 --- a/system/Database/MySQLi/Connection.php +++ b/system/Database/MySQLi/Connection.php @@ -283,14 +283,6 @@ public function getVersion(): string */ protected function execute(string $sql) { - // Errno 1213 ends the server transaction. Begin again before this - // statement so it does not autocommit. Errno 1205 is not handled: - // innodb_rollback_on_timeout decides whether InnoDB rolled that - // transaction back. - if ($this->connID instanceof mysqli && $this->consumeServerTransactionRestart()) { - $this->connID->begin_transaction(); - } - while ($this->connID->more_results()) { $this->connID->next_result(); if ($res = $this->connID->store_result()) { @@ -299,6 +291,16 @@ protected function execute(string $sql) } try { + // Errno 1213 ends the server transaction. Begin again before this + // statement so it does not autocommit. A failed BEGIN goes through + // the normal error path below, and the restart stays pending. + // Errno 1205 is not handled: innodb_rollback_on_timeout decides + // whether InnoDB rolled that transaction back. + if ($this->connID instanceof mysqli && $this->serverTransactionRestartPending()) { + $this->connID->begin_transaction(); + $this->serverTransactionRestarted(); + } + return $this->connID->query($this->prepQuery($sql), $this->resultMode); } catch (mysqli_sql_exception $e) { log_message('error', "{message}\nin {exFile} on line {exLine}.\n{trace}", [ diff --git a/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php b/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php index b65b7a5b865a..921651a83c2e 100644 --- a/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php +++ b/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php @@ -69,6 +69,72 @@ public function testFollowingWriteDoesNotAutocommitAndTransCompleteRollsItBack() $this->assertDeadlockVictimDoesNotPublishTheNextWrite(true); } + public function testFalseTransCommitEndsTheTransactionSoARetryCommits(): void + { + $table = $this->db->escapeIdentifier($this->db->prefixTable('deadlock_probe')); + $this->probeTable = $table; + $this->forceDeadlock($table); + + $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (102, 'after')"); + + $firstCommit = $this->db->transCommit(); + $depthAfter = $this->db->transDepth; + + // A caller that retries after the false commit must get a real transaction, + // not a nested one inside the restarted server transaction. + $this->db->transBegin(); + $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (103, 'retry')"); + $retryCommit = $this->db->transCommit(); + + $this->assertSame( + ['first_commit' => false, 'depth_after' => 0, 'retry_commit' => true, 'visible' => [103]], + [ + 'first_commit' => $firstCommit, + 'depth_after' => $depthAfter, + 'retry_commit' => $retryCommit, + 'visible' => $this->committedIds($table), + ], + ); + } + + public function testLostConnectionAfterDeadlockDoesNotEscapeAsDriverException(): void + { + $table = $this->db->escapeIdentifier($this->db->prefixTable('deadlock_probe')); + $this->probeTable = $table; + $this->forceDeadlock($table); + $this->killConnection($this->db); + + // The restart BEGIN fails on the dead socket. That must surface through the + // normal query error path, not as a raw mysqli_sql_exception. + $insert = $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (102, 'after')"); + $this->db->reconnect(); + + $this->assertSame( + ['insert' => false, 'complete' => false], + ['insert' => $insert, 'complete' => $this->db->transComplete()], + ); + } + + private function killConnection(BaseConnection $db): void + { + $threadId = $db->getConnection()->thread_id; + $observer = Database::connect($this->DBGroup, false); + $observer->initialize(); + $observer->query('KILL ' . $threadId); + + for ($i = 0; $i < 50; $i++) { + $alive = $observer->query('SELECT COUNT(*) AS n FROM information_schema.processlist WHERE id = ' . $threadId)->getRow()->n; + + if ((int) $alive === 0) { + break; + } + + usleep(20000); + } + + $observer->close(); + } + private function assertDeadlockVictimDoesNotPublishTheNextWrite(bool $useComplete): void { $table = $this->db->escapeIdentifier($this->db->prefixTable('deadlock_probe')); @@ -140,6 +206,10 @@ private function forceDeadlock(string $table): void private function resetProbe(string $table): void { + // releaseDeadlock() clears this marker after it drops the table. + // Record the name again before recreating it, including when setup fails. + $this->probeTable = $table; + if ($this->db->transDepth > 0) { $this->db->transRollback(); } @@ -162,7 +232,7 @@ private function committedIds(string $table): array { $observer = Database::connect($this->DBGroup, false); $observer->initialize(); - $rows = $observer->query('SELECT id FROM ' . $table . ' WHERE id IN (101, 102) ORDER BY id')->getResultArray(); + $rows = $observer->query('SELECT id FROM ' . $table . ' WHERE id > 100 ORDER BY id')->getResultArray(); $observer->close(); $ids = []; diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index 666f623d0e37..25f64ff88ffc 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -33,9 +33,10 @@ Bugs Fixed ********** - **Database:** Fixed MySQLi errno 1213 (``ER_LOCK_DEADLOCK``) so a following - write on the victim connection does not autocommit, and ``transCommit()`` - does not return true after InnoDB has rolled the victim transaction back. - Errno 1205 is unchanged. See `#10649 `_. + write through ``query()``, ``simpleQuery()`` or the Query Builder on the + victim connection does not autocommit, and ``transCommit()`` rolls back and + returns false after InnoDB has rolled the victim transaction back. Prepared + queries and errno 1205 are unchanged. See `#10649 `_. - **Commands:** Fixed a bug where generator commands did not sort the imports of a template with CRLF line endings. - **Format:** Fixed ``XMLFormatter`` so that an array key that becomes empty after sanitization no longer triggers a ``ValueError``; the fallback tag name ``item`` diff --git a/user_guide_src/source/database/transactions.rst b/user_guide_src/source/database/transactions.rst index aad742435242..984ff27a98be 100644 --- a/user_guide_src/source/database/transactions.rst +++ b/user_guide_src/source/database/transactions.rst @@ -149,9 +149,15 @@ MySQL Deadlocks =============== InnoDB error 1213 rolls the victim transaction back. CodeIgniter keeps that -transaction open. The next statement on the same connection starts a new -server transaction, so the write does not autocommit, and ``transCommit()`` -returns false. ``transComplete()`` rolls the new server transaction back. +transaction open. The next statement run through ``query()``, ``simpleQuery()`` +or the Query Builder on the same connection starts a new server transaction, +so the write does not autocommit. ``transCommit()`` rolls that server +transaction back and returns false, so the transaction has ended when it +returns. ``transComplete()`` also rolls it back. + +Prepared queries are not covered. A deadlock raised by a prepared query is not +detected, and a prepared query run after a deadlock does not start a new server +transaction. Error 1205 is not handled this way. ``innodb_rollback_on_timeout`` decides whether InnoDB rolled that transaction back, and the message is the same