From b35b9abceb49d244caec66e5c6028736454f044c Mon Sep 17 00:00:00 2001 From: Kevin Lopez Date: Thu, 22 Oct 2026 16:35:34 -0700 Subject: [PATCH 1/3] 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 | 1 + user_guide_src/source/database/connecting.rst | 4 +- 4 files changed, 147 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 75baa337d7b0..c22d289d270e 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -36,6 +36,7 @@ Bugs Fixed - **CLI:** Fixed a bug where ``CLI::promptByMultipleKeys()`` accepted a key missing from the options when it was below the last key, returning an empty selection, rejected a key above the last one when the options were not in key order, and offered ``0`` as the default when the first key was not ``0``. - **Commands:** Fixed a bug where generator commands did not sort the imports of a template with CRLF line endings. +- **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 `_. - **Format:** Fixed ``XMLFormatter`` so that an array key that becomes empty after sanitization no longer triggers a ``ValueError``; the fallback tag name ``item`` is used instead. Values that cannot be converted to a string, such as objects not implementing ``Stringable``, now throw a ``FormatException`` instead of a fatal ``Error``. - **Helpers:** Fixed a regression in v4.7.5 where ``set_value()`` threw a ``TypeError`` when the default value was not a string or an array, such as an integer, a float, ``null``, or a ``Stringable`` object. Such values are now converted to a string. - **HTTP:** Fixed a bug in ``ContentSecurityPolicy::setDefaultSrc()`` where passing an array of URIs resulted in an invalid ``'Array'`` directive. 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 4a25d53072b780800de96cff71b6c5eb3116282f Mon Sep 17 00:00:00 2001 From: Kevin Lopez Date: Fri, 23 Oct 2026 06:45:39 -0700 Subject: [PATCH 2/3] 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 | 2 +- user_guide_src/source/database/connecting.rst | 9 ++- 5 files changed, 111 insertions(+), 7 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 c22d289d270e..375d772e932f 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -36,7 +36,7 @@ Bugs Fixed - **CLI:** Fixed a bug where ``CLI::promptByMultipleKeys()`` accepted a key missing from the options when it was below the last key, returning an empty selection, rejected a key above the last one when the options were not in key order, and offered ``0`` as the default when the first key was not ``0``. - **Commands:** Fixed a bug where generator commands did not sort the imports of a template with CRLF line endings. -- **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 `_. +- **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 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 `_. - **Format:** Fixed ``XMLFormatter`` so that an array key that becomes empty after sanitization no longer triggers a ``ValueError``; the fallback tag name ``item`` is used instead. Values that cannot be converted to a string, such as objects not implementing ``Stringable``, now throw a ``FormatException`` instead of a fatal ``Error``. - **Helpers:** Fixed a regression in v4.7.5 where ``set_value()`` threw a ``TypeError`` when the default value was not a string or an array, such as an integer, a float, ``null``, or a ``Stringable`` object. Such values are now converted to a string. - **HTTP:** Fixed a bug in ``ContentSecurityPolicy::setDefaultSrc()`` where passing an array of URIs resulted in an invalid ``'Array'`` directive. 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- From e8bfeca357d16582a4945c81a59b2149266c741c Mon Sep 17 00:00:00 2001 From: Kevin Lopez Date: Mon, 26 Oct 2026 09:19:07 -0700 Subject: [PATCH 3/3] docs: say reconnect() does not resume the lost transaction Apply the review suggestion. The transaction-state reset only happens when ping() returns false, and PostgreSQL's pg_ping() can re-establish a broken connection and return true. The lost transaction is not resumed, later writes may autocommit, and the code that owns the whole transaction has to handle the failure before retrying. Drop the issue link from the changelog entry to match the changelog style. Co-authored-by: Michal Sniatala <459185+michalsn@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- user_guide_src/source/changelogs/v4.7.6.rst | 2 +- user_guide_src/source/database/connecting.rst | 14 +++++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index 375d772e932f..90bfa5e4c1b7 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -36,7 +36,7 @@ Bugs Fixed - **CLI:** Fixed a bug where ``CLI::promptByMultipleKeys()`` accepted a key missing from the options when it was below the last key, returning an empty selection, rejected a key above the last one when the options were not in key order, and offered ``0`` as the default when the first key was not ``0``. - **Commands:** Fixed a bug where generator commands did not sort the imports of a template with CRLF line endings. -- **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 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 `_. +- **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 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. - **Format:** Fixed ``XMLFormatter`` so that an array key that becomes empty after sanitization no longer triggers a ``ValueError``; the fallback tag name ``item`` is used instead. Values that cannot be converted to a string, such as objects not implementing ``Stringable``, now throw a ``FormatException`` instead of a fatal ``Error``. - **Helpers:** Fixed a regression in v4.7.5 where ``set_value()`` threw a ``TypeError`` when the default value was not a string or an array, such as an integer, a float, ``null``, or a ``Stringable`` object. Such values are now converted to a string. - **HTTP:** Fixed a bug in ``ContentSecurityPolicy::setDefaultSrc()`` where passing an array of URIs resulted in an invalid ``'Array'`` directive. diff --git a/user_guide_src/source/database/connecting.rst b/user_guide_src/source/database/connecting.rst index 6099c7abc2a1..f74188ee0f94 100644 --- a/user_guide_src/source/database/connecting.rst +++ b/user_guide_src/source/database/connecting.rst @@ -104,9 +104,17 @@ 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. +The transaction-state reset described above only occurs when ``ping()`` +returns false. On PostgreSQL, ``ping()`` uses ``pg_ping()``, which can +re-establish a broken connection and return true. In that case, +``reconnect()`` does not clear the transaction state. + +The lost transaction is not resumed on the new connection. Subsequent +writes may be committed automatically and cannot be rolled back by +calling ``transComplete()``. If the operation must remain atomic, stop +executing its remaining queries. Handle the failure at the application +level that owns the entire transaction, including any nested transaction +groups, before deciding whether to retry the entire operation. .. literalinclude:: connecting/007.php :lines: 2-