[rc2] Fix concurrency issue in RelationalConnection ambient transaction handling

src/EFCore.Relational/Storage/RelationalConnection.cs

@@ -39,6 +39,7 @@ public abstract class RelationalConnection : IRelationalConnection, ITransaction
     private bool _openedInternally;
     private int? _commandTimeout;
     private readonly int? _defaultCommandTimeout;
+    private volatile bool _resetting;
     private readonly ConcurrentStack<Transaction> _ambientTransactions = new();
     private DbConnection? _connection;
     private readonly IRelationalCommandBuilder _relationalCommandBuilder;
@@ -684,16 +685,14 @@ private void ClearTransactions(bool clearAmbient)
     {
         CurrentTransaction = null;
         EnlistedTransaction = null;
-        if (clearAmbient && _ambientTransactions.Count > 0)
+        if (clearAmbient)
         {
-            while (_ambientTransactions.Any(t => t != null))
+            _resetting = true;
+            while (_ambientTransactions.TryPop(out var ambientTransaction))
             {
-                _ambientTransactions.TryPop(out var ambientTransaction);
-                if (ambientTransaction != null)
-                {
-                    ambientTransaction.TransactionCompleted -= HandleTransactionCompleted;
-                }
+                ambientTransaction.TransactionCompleted -= HandleTransactionCompleted;
             }
+            _resetting = false;
         }
 
         if (_openedCount < 0)
@@ -852,18 +851,18 @@ private void HandleAmbientTransactions()
     private void HandleTransactionCompleted(object? sender, TransactionEventArgs e)
     {
         // This could be invoked on a different thread at arbitrary time after the transaction completes
-        _ambientTransactions.TryPeek(out var ambientTransaction);
-        if (e.Transaction != ambientTransaction)
+        if (!_ambientTransactions.TryPop(out var ambientTransaction)
+            || _resetting)
         {
-            throw new InvalidOperationException(RelationalStrings.NestedAmbientTransactionError);
+            return;
         }
 
-        if (ambientTransaction != null)
+        if (e.Transaction != ambientTransaction)
         {
-            ambientTransaction.TransactionCompleted -= HandleTransactionCompleted;
+            throw new InvalidOperationException(RelationalStrings.NestedAmbientTransactionError);
         }
 
-        _ambientTransactions.TryPop(out _);
+        ambientTransaction.TransactionCompleted -= HandleTransactionCompleted;
     }
 
     /// <summary>

test/EFCore.Relational.Tests/RelationalConnectionTest.cs

@@ -2,7 +2,9 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Data;
+using System.Transactions;
 using Microsoft.EntityFrameworkCore.TestUtilities.FakeProvider;
+using IsolationLevel = System.Data.IsolationLevel;
 
 namespace Microsoft.EntityFrameworkCore;
 
@@ -1082,6 +1084,79 @@ public async Task Reports_command_diagnostic_on_cancellation()
             });
     }
 
+    [ConditionalFact]
+    public void HandleTransactionCompleted_with_concurrent_ClearTransactions_is_thread_safe()
+    {
+        // This test verifies the fix for the race condition where HandleTransactionCompleted
+        // could be called on a different thread while ClearTransactions is executing.
+        var exceptions = new List<Exception>();
+        for (var i = 0; i < Environment.ProcessorCount; i++)
+        {
+            var connection = new FakeRelationalConnection(
+                CreateOptions(new FakeRelationalOptionsExtension().WithConnectionString("Database=ConcurrencyTest")));
+
+            using var scope = new TransactionScope();
+            connection.Open();
+
+            var random = new Random();
+            var resetFirst = random.Next(0, 1) == 0;
+            var tasks = new Task[2];
+            tasks[0] = Task.Run(async () =>
+            {
+                try
+                {
+                    // Small delay to increase chance of race condition
+                    await Task.Yield();
+
+                    if (resetFirst)
+                    {
+                        ((IResettableService)connection).ResetState();
+                    }
+                    else
+                    {
+                        scope.Complete();
+                    }
+                }
+                catch (Exception ex)
+                {
+                    lock (exceptions)
+                    {
+                        exceptions.Add(ex);
+                    }
+                }
+            });
+
+            tasks[1] = Task.Run(async () =>
+            {
+                try
+                {
+                    // Small delay to increase chance of race condition
+                    await Task.Yield();
+
+                    if (resetFirst)
+                    {
+                        scope.Complete();
+                    }
+                    else
+                    {
+                        ((IResettableService)connection).ResetState();
+                    }
+                }
+                catch (Exception ex)
+                {
+                    lock (exceptions)
+                    {
+                        exceptions.Add(ex);
+                    }
+                }
+            });
+
+            Task.WaitAll(tasks, TimeSpan.FromSeconds(10));
+        }
+
+        Assert.Empty(exceptions);
+    }
+
     private static IDbContextOptions CreateOptions(params RelationalOptionsExtension[] optionsExtensions)
     {
         var optionsBuilder = new DbContextOptionsBuilder();

test/EFCore.Relational.Tests/TestUtilities/FakeProvider/FakeRelationalConnection.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Transactions;
 using Microsoft.EntityFrameworkCore.Diagnostics.Internal;
 using Microsoft.EntityFrameworkCore.Infrastructure.Internal;
 using Microsoft.EntityFrameworkCore.Storage.Internal;
@@ -70,6 +71,11 @@ public List<Tuple<string, object>> TransactionDiagnosticEvents
     public List<Tuple<string, object>> ConnectionDiagnosticEvents
         => ((ListDiagnosticSource)Dependencies.ConnectionLogger.DiagnosticSource).DiagnosticList;
 
+    protected override bool SupportsAmbientTransactions => true;
+
+    protected override void ConnectionEnlistTransaction(Transaction transaction)
+    { }
+
     protected override DbConnection CreateDbConnection()
     {
         var connection = new FakeDbConnection(ConnectionString);