fix: open real transactions when no boundary exists

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,19 +61,21 @@ 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 );
 		}
 
 		// phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.PreparedSQL.NotPrepared
 		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;
 		}
 	}
 }

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<int, string> */
+	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;
+	}
+}