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; + } +}