From 6369d6b541edce430aa9fc037ba5f7f807c2ddde Mon Sep 17 00:00:00 2001 From: Kevin Lopez Date: Thu, 22 Oct 2026 16:35:34 -0700 Subject: [PATCH 1/2] fix: clear transaction state when reconnect() replaces the socket The server rolls the open transaction back with the dead connection. Keeping transDepth and transStatus made transComplete() return true. Fixes #10648 Co-authored-by: Cursor --- system/Database/BaseConnection.php | 11 ++ .../Live/ReconnectDuringTransactionTest.php | 132 ++++++++++++++++++ user_guide_src/source/changelogs/v4.7.6.rst | 4 + user_guide_src/source/database/connecting.rst | 4 +- 4 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 tests/system/Database/Live/ReconnectDuringTransactionTest.php diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 002cf661226d..7832f541768e 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -625,13 +625,24 @@ public function close() * Keep or establish the connection if no queries have been sent for * a length of time exceeding the server's idle timeout. * + * A replaced socket is not in the transaction that was open on the old + * one. The server has already rolled that work back. Clear the depth and + * mark the transaction failed so transComplete() cannot report success. + * * @return void */ public function reconnect() { if ($this->ping() === false) { + $lostTransaction = $this->transDepth > 0; + $this->close(); $this->initialize(); + + if ($lostTransaction) { + $this->transDepth = 0; + $this->transStatus = false; + } } } diff --git a/tests/system/Database/Live/ReconnectDuringTransactionTest.php b/tests/system/Database/Live/ReconnectDuringTransactionTest.php new file mode 100644 index 000000000000..280d68d6f034 --- /dev/null +++ b/tests/system/Database/Live/ReconnectDuringTransactionTest.php @@ -0,0 +1,132 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Database\Live; + +use CodeIgniter\Test\CIUnitTestCase; +use CodeIgniter\Test\DatabaseTestTrait; +use Config\Database; +use PHPUnit\Framework\Attributes\Group; + +/** + * @internal + */ +#[Group('DatabaseLive')] +final class ReconnectDuringTransactionTest extends CIUnitTestCase +{ + use DatabaseTestTrait; + + protected $migrate = false; + + protected function setUp(): void + { + // Dedicated connection so a forced wait_timeout cannot drop the shared one. + $this->db = Database::connect($this->DBGroup, false); + + parent::setUp(); + } + + public function testReconnectInsideTransactionDoesNotReportSuccess(): void + { + if ($this->db->DBDriver !== 'MySQLi') { + $this->markTestSkipped('Requires the MySQLi driver.'); + } + + $table = $this->db->escapeIdentifier($this->db->prefixTable('reconnect_probe')); + $this->db->query('DROP TABLE IF EXISTS ' . $table); + $this->db->query( + 'CREATE TABLE ' . $table . ' (' + . 'id INT NOT NULL AUTO_INCREMENT, ' + . 'note VARCHAR(32) NOT NULL, ' + . 'PRIMARY KEY (id)' + . ') ENGINE=InnoDB', + ); + + $observer = Database::connect($this->DBGroup, false); + $observer->initialize(); + + try { + $this->db->transStart(); + $this->db->table('reconnect_probe')->insert(['note' => 'before-timeout']); + $this->assertSame(1, $this->db->transDepth); + $this->assertTrue($this->db->transStatus()); + + $this->db->query('SET SESSION wait_timeout = 1'); + sleep(3); + + $this->assertFalse($this->db->ping(), 'Precondition: the server should have closed the idle socket.'); + + $this->db->reconnect(); + + $depthAfterReconnect = $this->db->transDepth; + $statusAfterReconnect = $this->db->transStatus(); + $completed = $this->db->transComplete(); + $rows = (int) $observer->table('reconnect_probe')->countAllResults(); + + $this->assertSame( + ['depth' => 0, 'status' => false, 'completed' => false, 'rows' => 0], + [ + 'depth' => $depthAfterReconnect, + 'status' => $statusAfterReconnect, + 'completed' => $completed, + 'rows' => $rows, + ], + ); + } finally { + if ($this->db->transDepth > 0) { + $this->db->transRollback(); + } + + $this->db->query('DROP TABLE IF EXISTS ' . $table); + $observer->close(); + $this->db->close(); + } + } + + public function testReconnectKeepsAnOpenTransactionWhenTheSocketIsAlive(): void + { + if ($this->db->DBDriver !== 'MySQLi') { + $this->markTestSkipped('Requires the MySQLi driver.'); + } + + $table = $this->db->escapeIdentifier($this->db->prefixTable('reconnect_probe')); + $this->db->query('DROP TABLE IF EXISTS ' . $table); + $this->db->query( + 'CREATE TABLE ' . $table . ' (' + . 'id INT NOT NULL AUTO_INCREMENT, ' + . 'note VARCHAR(32) NOT NULL, ' + . 'PRIMARY KEY (id)' + . ') ENGINE=InnoDB', + ); + + try { + $this->db->transStart(); + $this->db->table('reconnect_probe')->insert(['note' => 'still-open']); + $this->assertTrue($this->db->ping()); + + $this->db->reconnect(); + + $this->assertSame(1, $this->db->transDepth); + $this->assertTrue($this->db->transStatus()); + $this->assertTrue($this->db->transComplete()); + $this->assertSame(1, $this->db->table('reconnect_probe')->countAllResults()); + } finally { + if ($this->db->transDepth > 0) { + $this->db->transRollback(); + } + + $this->db->query('DROP TABLE IF EXISTS ' . $table); + $this->db->close(); + } + } +} diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index 8739e9546d4c..92ca13fc7330 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 ``reconnect()`` so a replaced socket does not keep the + transaction that died with the old connection. ``transDepth`` is cleared and + the transaction status is failed, so ``transComplete()`` does not report + success. See `#10648 `_. - **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/connecting.rst b/user_guide_src/source/database/connecting.rst index 65588dca4187..f3ec0258710a 100644 --- a/user_guide_src/source/database/connecting.rst +++ b/user_guide_src/source/database/connecting.rst @@ -99,7 +99,9 @@ which can gracefully keep the connection alive or re-establish it. The ``reconnect()`` method pings the server to check if the connection is still alive. If the connection has been lost, it will close and re-establish the -connection. +connection. An open transaction ends with the old connection. ``reconnect()`` +clears the transaction depth and marks the transaction failed, so +``transComplete()`` does not report success. .. literalinclude:: connecting/007.php :lines: 2- From 1a7668c7da063376891bfb5f3ce1e3a20e94d97a Mon Sep 17 00:00:00 2001 From: Kevin Lopez Date: Fri, 23 Oct 2026 06:45:39 -0700 Subject: [PATCH 2/2] fix: clear lost transaction state before reconnecting reconnect() cleared transDepth and transStatus only after initialize() returned. When initialize() threw, the old transaction state survived, and a later query() that reconnected on its own let transComplete() report success. The state is now cleared before initialize() runs. OCI8 keeps its commit mode in PHP. Clearing the depth skipped _transRollback(), so commitMode stayed OCI_NO_AUTO_COMMIT and later writes were never committed. OCI8 now restores OCI_COMMIT_ON_SUCCESS when the transaction is abandoned. The guide and changelog now say this applies only when ping() reports the connection lost. On PostgreSQL, pg_ping() re-establishes a broken connection itself and returns true, so reconnect() does not clear the state there. Refs #10648 Co-Authored-By: Claude Opus 5.5 --- system/Database/BaseConnection.php | 25 ++++++- system/Database/OCI8/Connection.php | 7 ++ .../Database/ReconnectLostTransactionTest.php | 75 +++++++++++++++++++ user_guide_src/source/changelogs/v4.7.6.rst | 8 +- user_guide_src/source/database/connecting.rst | 9 ++- 5 files changed, 116 insertions(+), 8 deletions(-) create mode 100644 tests/system/Database/ReconnectLostTransactionTest.php diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 7832f541768e..e71645e0f9c7 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -627,7 +627,9 @@ public function close() * * A replaced socket is not in the transaction that was open on the old * one. The server has already rolled that work back. Clear the depth and - * mark the transaction failed so transComplete() cannot report success. + * mark the transaction failed before opening the new connection, so a + * failed reconnect cannot still report success. OCI8 also returns its + * commit mode to autocommit. * * @return void */ @@ -637,15 +639,30 @@ public function reconnect() $lostTransaction = $this->transDepth > 0; $this->close(); - $this->initialize(); + // initialize() can throw. Clear the lost transaction first so a + // failed reconnect cannot still report the old transaction. if ($lostTransaction) { - $this->transDepth = 0; - $this->transStatus = false; + $this->abandonLostTransaction(); } + + $this->initialize(); } } + /** + * The socket that owned the transaction is already closed. + * + * Drivers that keep transaction state in PHP, rather than only on the + * server, must override this. OCI8 commit mode is one case: leaving it + * at OCI_NO_AUTO_COMMIT makes later writes uncommitted. + */ + protected function abandonLostTransaction(): void + { + $this->transDepth = 0; + $this->transStatus = false; + } + /** * Platform dependent way method for closing the connection. * diff --git a/system/Database/OCI8/Connection.php b/system/Database/OCI8/Connection.php index 0d700ef095cf..69f5d98e6416 100644 --- a/system/Database/OCI8/Connection.php +++ b/system/Database/OCI8/Connection.php @@ -722,6 +722,13 @@ protected function _transRollback(): bool return oci_rollback($this->connID); } + protected function abandonLostTransaction(): void + { + $this->commitMode = OCI_COMMIT_ON_SUCCESS; + + parent::abandonLostTransaction(); + } + /** * Returns the name of the current database being used. */ diff --git a/tests/system/Database/ReconnectLostTransactionTest.php b/tests/system/Database/ReconnectLostTransactionTest.php new file mode 100644 index 000000000000..eca053ef5b41 --- /dev/null +++ b/tests/system/Database/ReconnectLostTransactionTest.php @@ -0,0 +1,75 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Database; + +use CodeIgniter\Database\Exceptions\DatabaseException; +use CodeIgniter\Database\MySQLi\Connection as MySQLiConnection; +use CodeIgniter\Database\OCI8\Connection as OCI8Connection; +use CodeIgniter\Test\CIUnitTestCase; +use PHPUnit\Framework\Attributes\Group; + +/** + * @internal + */ +#[Group('Others')] +final class ReconnectLostTransactionTest extends CIUnitTestCase +{ + public function testReconnectClearsTransactionStateWhenInitializeFails(): void + { + $db = new class (['DBDriver' => 'MySQLi']) extends MySQLiConnection { + public function initialize(): never + { + throw new DatabaseException('database unavailable'); + } + }; + + $this->setPrivateProperty($db, 'transDepth', 1); + + try { + $db->reconnect(); + $this->fail('initialize() should have thrown.'); + } catch (DatabaseException $exception) { + $this->assertSame('database unavailable', $exception->getMessage()); + } + + $this->assertSame(0, $this->getPrivateProperty($db, 'transDepth')); + $this->assertFalse($db->transStatus()); + } + + public function testReconnectRestoresOci8CommitModeWhenTransactionIsLost(): void + { + if (! defined('OCI_COMMIT_ON_SUCCESS')) { + define('OCI_COMMIT_ON_SUCCESS', 32); + } + + if (! defined('OCI_NO_AUTO_COMMIT')) { + define('OCI_NO_AUTO_COMMIT', 0); + } + + $db = new class (['DBDriver' => 'OCI8']) extends OCI8Connection { + public function initialize(): void + { + } + }; + + $db->commitMode = OCI_NO_AUTO_COMMIT; + $this->setPrivateProperty($db, 'transDepth', 1); + + $db->reconnect(); + + $this->assertSame(OCI_COMMIT_ON_SUCCESS, $db->commitMode); + $this->assertSame(0, $this->getPrivateProperty($db, 'transDepth')); + $this->assertFalse($db->transStatus()); + } +} diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index 92ca13fc7330..aab6c79f5bfb 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -34,8 +34,12 @@ Bugs Fixed - **Database:** Fixed ``reconnect()`` so a replaced socket does not keep the transaction that died with the old connection. ``transDepth`` is cleared and - the transaction status is failed, so ``transComplete()`` does not report - success. See `#10648 `_. + the transaction status is failed before the new connection is opened, so a + failed reconnect cannot still report success. OCI8 commit mode returns to + autocommit. ``transComplete()`` does not report success. This applies when + ``ping()`` reports the connection lost; PostgreSQL's ``pg_ping()`` reconnects + a broken connection itself, so it is not covered. + See `#10648 `_. - **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/connecting.rst b/user_guide_src/source/database/connecting.rst index f3ec0258710a..6099c7abc2a1 100644 --- a/user_guide_src/source/database/connecting.rst +++ b/user_guide_src/source/database/connecting.rst @@ -100,8 +100,13 @@ which can gracefully keep the connection alive or re-establish it. The ``reconnect()`` method pings the server to check if the connection is still alive. If the connection has been lost, it will close and re-establish the connection. An open transaction ends with the old connection. ``reconnect()`` -clears the transaction depth and marks the transaction failed, so -``transComplete()`` does not report success. +clears the transaction depth and marks the transaction failed before it +opens the new connection, so ``transComplete()`` does not report success. +On OCI8, commit mode returns to autocommit. + +This only applies when ``ping()`` reports the connection lost. On PostgreSQL, +``ping()`` uses ``pg_ping()``, which re-establishes a broken connection itself +and returns true, so ``reconnect()`` does not clear the transaction state. .. literalinclude:: connecting/007.php :lines: 2-