[10.x] After commit callback throwing an exception causes broken transactions afterwards

[oprypkhantc]

Hey again. It's another try for #50087.

The problem

The problem is that the transaction level is reset to 0 whenever an DB::afterCommit() callback fails. Normally, outside of tests that use DatabaseTransactions trait, this not causing any issues:

// Normal production code
//   transactionLevel === 0
DB::transaction(function () {
    //   transactionLevel === 1

    DB::afterCommit(function () {
        // Outside of tests, by the time this callback is executed, the transaction level
        // has already been reset to 0 because the commit just happened
        //   transactionLevel === 0
        
        throw new \Exception();
    });
});

// As expected, transaction level is 0, which matches an actual level of the connection. Nothing's broken.
//  transactionLevel === 0

// Works:
DB::beginTransaction();

Because the after commit callbacks are only executed after the commit, where transaction level is already 0, this isn't actually doing anything, as the transaction level cannot be decremented below 0.

However, when you try to do the same in a test that uses DatabaseTransactions trait, you will face an issue as the "default" transaction level is 1 and an actual database commit should never happen:

class SomeTest extends TestCase 
{
    use DatabaseTransactions;

    public function testSomething()
    {
        // transactionLevel === 1
    
        try {
            DB::transaction(function () {
                //   transactionLevel === 2
            
                DB::afterCommit(function () {
                    // In tests that use DatabaseTransactions, unlike the example above, this callback is executed without any
                    // COMMIT statement before, meaning the transaction level is still 1 here
                    //   transactionLevel === 1
                    
                    throw new \Exception();
                });
            });
        } catch (\Exception) {}
        
       // Outside of the transaction above, the transaction level should have stayed 1, but it's actually 0:
       //   transactionLevel === 0
       
       // Now if we try to start a new transaction, we get an error:

        // Instead of creating a savepoint, this attempts to create a transaction, which fails with a PDO exception:
        //   PDOException: There is already an active transaction
        DB::beginTransaction();
    }
}

Why transaction level is important

As demonstrated above, having a transaction level differ from an actual connection's transaction level leads to broken transactions. If Laravel thinks the transaction level is 0 when it's actually 1, it will attempt to send a BEGIN TRANSACTION; query instead of the CREATE SAVEPOINT trans2; query, which will cause an error on the DBMS side.

Why no reports of this bug

This is only reproducible under these 4 conditions:

• in a test
• test uses DatabaseTransactions trait
• test has two transactions
• first transaction throws an exception from an afterCommit callback

It's highly unlikely for anyone to encounter this.

Why fixing this is important

The test example above is simplified. Obviously, we don't have any tests like the above. However, we do have integration tests that call multiple endpoints in a single test, which is how we found out about this problem at all:

class SomeTest extends TestCase 
{
    use DatabaseTransactions;

    public function testSomething()
    {
        // transactionLevel === 1
        $this->getJson('some_endpoint_with_after_commit_error')
            ->assertUnprocessable()
            ->assertJson(['code' => 'something']);
            
        // transactionLevel === 0
        // Fails immediately due to `PDOException: There is already an active transaction`
        $this->getJson('some_endpoint_with_any_transaction')
            ->assertOk();
    }
}

Context

We're a large project that has 20k tests, nearly all of which use DatabaseTransactions trait. We've had our own implementation of "after commit" jobs, notifications and events/listeners long before Laravel first introduced it and have tackled a lot of the nuances that come with it. We're now trying to move away from our custom implementation in favour of Laravel built-in methods. We did not encounter any issues with this in our own implementation because we relied on the committed event which is outside of the try-catch block, so it works as expected even if something throws.

After switching to Laravel's implementation, this bug caused a few of "negative" integration tests to fail.

[taylorotwell]

I'll give it a shot. Thanks.