From ca0ee723dedba0246e50285200d68cf7886f888c Mon Sep 17 00:00:00 2001 From: "homeboy-ci[bot]" <266378653+homeboy-ci[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 23:13:45 +0000 Subject: [PATCH] fix: open real transactions when no boundary exists TransactionScope forced the savepoint path on SQLite and treated an unsupported @@autocommit as autocommit disabled, so it issued a bare SAVEPOINT with no enclosing transaction. Engines that do not implement @@autocommit report NULL, which casts to 0 and was read as "a caller owns the outer transaction". Only a non-NULL zero now counts, and the driver-specific shortcut is gone, so a scope opens a real transaction when nothing encloses it. A bare SAVEPOINT is invalid on MySQL: it opens no transaction, and releasing it fails with error 1305. Upstream sqlite-database-integration is aligning with those semantics, which turns the old path into failed commits and rollbacks that silently leave writes applied. Nesting can no longer be inferred from server variables, because a plain START TRANSACTION is invisible to them on SQLite and a second one is an implicit commit of the first. Scope depth is tracked in process instead, and unresolved scopes release their depth so later scopes still open a real boundary. Refs #3436 --- inc/Core/Database/TransactionScope.php | 55 ++++++- .../Core/Database/TransactionScopeTest.php | 138 ++++++++++++++++++ 2 files changed, 186 insertions(+), 7 deletions(-) diff --git a/inc/Core/Database/TransactionScope.php b/inc/Core/Database/TransactionScope.php index 69c7840be..15a1b3e85 100644 --- a/inc/Core/Database/TransactionScope.php +++ b/inc/Core/Database/TransactionScope.php @@ -12,6 +12,7 @@ final class TransactionScope { private static int $savepoint_sequence = 0; + private static int $open_scopes = 0; private bool $active = true; private function __construct( @@ -20,13 +21,38 @@ private function __construct( private ?string $name = null ) {} + /** + * Release the nesting depth held by a scope that was never resolved. + * + * A caller that returns or throws without committing or rolling back would + * otherwise leave the depth raised for the rest of the request, and every + * later scope would nest inside a boundary that no longer exists. + */ + public function __destruct() { + if ( $this->active ) { + $this->release_depth(); + } + } + /** Open a transaction or a savepoint when the connection already has a boundary. */ public static function begin( $wpdb ): ?self { - $in_transaction = BaseRepository::is_sqlite(); + // An enclosing scope from this class already owns a transaction. Server + // variables cannot report this on every engine, so track it in process. + $in_transaction = self::$open_scopes > 0; + if ( ! $in_transaction ) { - // A caller that disabled autocommit owns the outer transaction. + /* + * A caller that disabled autocommit owns the outer transaction. + * + * Engines that do not implement these MySQL server variables report + * NULL rather than a value. NULL casts to 0, which would otherwise + * be read as "autocommit disabled" and wrongly claim an enclosing + * transaction, so only a non-NULL zero counts. + */ // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.PreparedSQL.NotPrepared - $in_transaction = 0 === (int) $wpdb->get_var( 'SELECT @@autocommit' ); + $autocommit = $wpdb->get_var( 'SELECT @@autocommit' ); + $in_transaction = null !== $autocommit && 0 === (int) $autocommit; + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.PreparedSQL.NotPrepared $has_transaction_variable = $wpdb->get_var( "SHOW VARIABLES LIKE 'in_transaction'" ); if ( ! $in_transaction && 'in_transaction' === strtolower( (string) $has_transaction_variable ) ) { @@ -35,12 +61,13 @@ public static function begin( $wpdb ): ?self { } } - if ( $in_transaction || BaseRepository::is_sqlite() ) { + if ( $in_transaction ) { $name = 'datamachine_transaction_' . ( ++self::$savepoint_sequence ); // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.PreparedSQL.InterpolatedNotPrepared if ( false === $wpdb->query( "SAVEPOINT {$name}" ) ) { return null; } + ++self::$open_scopes; return new self( $wpdb, 'savepoint', $name ); } @@ -48,6 +75,7 @@ public static function begin( $wpdb ): ?self { if ( false === $wpdb->query( 'START TRANSACTION' ) ) { return null; } + ++self::$open_scopes; return new self( $wpdb, 'transaction' ); } @@ -66,7 +94,7 @@ public function commit(): bool { } if ( $committed ) { - $this->active = false; + $this->close(); } return $committed; } @@ -86,12 +114,25 @@ public function rollback(): void { if ( false === $this->wpdb->query( "RELEASE SAVEPOINT {$this->name}" ) ) { return; } - $this->active = false; + $this->close(); return; } // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.PreparedSQL.NotPrepared if ( false !== $this->wpdb->query( 'ROLLBACK' ) ) { - $this->active = false; + $this->close(); + } + } + + /** Mark this scope closed and release its nesting depth. */ + private function close(): void { + $this->active = false; + $this->release_depth(); + } + + /** Give back the nesting depth this scope claimed when it opened. */ + private function release_depth(): void { + if ( self::$open_scopes > 0 ) { + --self::$open_scopes; } } } diff --git a/tests/Unit/Core/Database/TransactionScopeTest.php b/tests/Unit/Core/Database/TransactionScopeTest.php index 56e4b5bef..8353dc868 100644 --- a/tests/Unit/Core/Database/TransactionScopeTest.php +++ b/tests/Unit/Core/Database/TransactionScopeTest.php @@ -64,6 +64,107 @@ public function test_sqlite_rolled_back_scope_cannot_commit_as_stale_owner(): vo $this->assertSame( JobStatus::PENDING, $this->jobs->get_job( $job_id )['status'] ); } + /** + * Autocommit detection must not treat an unsupported variable as enabled. + * + * Engines that do not implement "@@autocommit" report NULL, which casts to + * 0 and would otherwise be read as "autocommit disabled". That would claim + * an enclosing transaction that does not exist and open a bare savepoint, + * which cannot be committed or rolled back on its own. + */ + public function test_absent_autocommit_variable_is_not_read_as_open_transaction(): void { + $wpdb = new TransactionScopeProbeWpdb( null ); + $scope = TransactionScope::begin( $wpdb ); + + $this->assertInstanceOf( TransactionScope::class, $scope ); + $this->assertSame( array( 'START TRANSACTION' ), $wpdb->executed ); + + $this->assertTrue( $scope->commit() ); + $this->assertSame( array( 'START TRANSACTION', 'COMMIT' ), $wpdb->executed ); + } + + /** A caller that disabled autocommit still nests inside its transaction. */ + public function test_disabled_autocommit_opens_savepoint_scope(): void { + $wpdb = new TransactionScopeProbeWpdb( '0' ); + $scope = TransactionScope::begin( $wpdb ); + + $this->assertInstanceOf( TransactionScope::class, $scope ); + $this->assertCount( 1, $wpdb->executed ); + $this->assertStringStartsWith( 'SAVEPOINT datamachine_transaction_', $wpdb->executed[0] ); + } + + /** Autocommit enabled means this scope owns a real transaction. */ + public function test_enabled_autocommit_opens_transaction_scope(): void { + $wpdb = new TransactionScopeProbeWpdb( '1' ); + $scope = TransactionScope::begin( $wpdb ); + + $this->assertInstanceOf( TransactionScope::class, $scope ); + $this->assertSame( array( 'START TRANSACTION' ), $wpdb->executed ); + } + + /** + * An unresolved scope must not make later scopes nest into nothing. + * + * A caller that returns or throws without committing or rolling back would + * otherwise leave the tracked depth raised, and the next independent scope + * would open a savepoint with no enclosing transaction to hold it. + */ + public function test_abandoned_scope_does_not_strand_later_scopes(): void { + $abandoned = TransactionScope::begin( new TransactionScopeProbeWpdb( null ) ); + $this->assertInstanceOf( TransactionScope::class, $abandoned ); + unset( $abandoned ); + + $wpdb = new TransactionScopeProbeWpdb( null ); + $scope = TransactionScope::begin( $wpdb ); + + $this->assertInstanceOf( TransactionScope::class, $scope ); + $this->assertSame( array( 'START TRANSACTION' ), $wpdb->executed ); + } + + /** + * A nested scope must not discard the enclosing scope's work. + * + * Engines differ in what they report through server variables, and some + * treat a second START TRANSACTION as an implicit commit of the first. The + * inner scope must therefore always nest rather than open a new boundary. + */ + public function test_inner_scope_rollback_preserves_outer_scope_write(): void { + $outer_job = $this->createPendingJob( 'Outer scope write' ); + $inner_job = $this->createPendingJob( 'Inner scope write' ); + + $outer = TransactionScope::begin( $GLOBALS['wpdb'] ); + $this->assertInstanceOf( TransactionScope::class, $outer ); + $this->assertTrue( $this->jobs->start_job( $outer_job ) ); + + $inner = TransactionScope::begin( $GLOBALS['wpdb'] ); + $this->assertInstanceOf( TransactionScope::class, $inner ); + $this->assertTrue( $this->jobs->start_job( $inner_job ) ); + $inner->rollback(); + + $this->assertSame( JobStatus::PENDING, $this->jobs->get_job( $inner_job )['status'] ); + $this->assertSame( JobStatus::PROCESSING, $this->jobs->get_job( $outer_job )['status'] ); + + $this->assertTrue( $outer->commit() ); + $this->assertSame( JobStatus::PROCESSING, $this->jobs->get_job( $outer_job )['status'] ); + $this->assertSame( JobStatus::PENDING, $this->jobs->get_job( $inner_job )['status'] ); + } + + /** An outer rollback must discard work committed by an inner scope. */ + public function test_outer_scope_rollback_discards_committed_inner_scope(): void { + $job_id = $this->createPendingJob( 'Outer rollback discards inner' ); + + $outer = TransactionScope::begin( $GLOBALS['wpdb'] ); + $this->assertInstanceOf( TransactionScope::class, $outer ); + + $inner = TransactionScope::begin( $GLOBALS['wpdb'] ); + $this->assertInstanceOf( TransactionScope::class, $inner ); + $this->assertTrue( $this->jobs->start_job( $job_id ) ); + $this->assertTrue( $inner->commit() ); + + $outer->rollback(); + $this->assertSame( JobStatus::PENDING, $this->jobs->get_job( $job_id )['status'] ); + } + private function requireSqlite(): void { if ( ! BaseRepository::is_sqlite() ) { $this->markTestSkipped( 'SQLite-specific savepoint integration coverage.' ); @@ -77,3 +178,40 @@ private function createPendingJob( string $label ): int { return $job_id; } } + +/** + * Records the statements a scope issues for a given "@@autocommit" value. + * + * The live test connection always runs inside the WordPress test transaction, + * so the autocommit-enabled branch is unreachable through $GLOBALS['wpdb']. + */ +// phpcs:ignore Generic.Files.OneObjectStructurePerFile.MultipleFound +class TransactionScopeProbeWpdb { + + /** @var array */ + public array $executed = array(); + + /** @param string|null $autocommit Value reported for "@@autocommit". */ + public function __construct( private $autocommit ) {} + + /** + * @param string $query Query to inspect. + * @return string|null + */ + public function get_var( $query ) { + if ( str_contains( $query, '@@autocommit' ) ) { + return $this->autocommit; + } + // Report no support for the "in_transaction" variable. + return null; + } + + /** + * @param string $query Query to record. + * @return int + */ + public function query( $query ) { + $this->executed[] = $query; + return 1; + } +}