Scope.Transaction is now AsyncLocal

CHANGELOG.md

@@ -10,6 +10,8 @@
 - `Device.BatteryLevel` and `Device.ProcessorFrequency` are now stored as floats rather than ints, to align with the Cocoa and Java SDKs ([#3567](https://github.com/getsentry/sentry-dotnet/pull/3567))
 - `SentryOptions.EnableTracing` has been removed. Instead, tracing should be enabled or disabled by setting the `SentryOptions.TracesSampleRate` or by using `SentryOptions.TracesSampler` to configure a sampling function ([#3569](https://github.com/getsentry/sentry-dotnet/pull/3569))
 - The `FailedRequestTargets`, `TagFilters` and `TracePropagationTargets` options have all been changed from `SubstringOrRegexPattern` to `IList<StringOrRegex>` ([#3566](https://github.com/getsentry/sentry-dotnet/pull/3566))
+- `Scope.Transaction` is now always stored as an `AsyncLocal` also in [Global Mode](https://docs.sentry.io/platforms/dotnet/configuration/options/#is-global-mode-enabled), to prevent auto-instrumented spans from the UI ending up parented to transactions from a background task (or vice versa). ([#3596](https://github.com/getsentry/sentry-dotnet/pull/3596))
+
 ## Unreleased
 
 ### Fixes

src/Sentry/Scope.cs

@@ -1,3 +1,8 @@
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Threading;
 using Sentry.Extensibility;
 using Sentry.Internal;
 using Sentry.Internal.Extensions;
@@ -180,15 +185,52 @@ public string? TransactionName
         }
     }
 
-    private ITransactionTracer? _transaction;
+    /// <summary>
+    /// <para>
+    /// Most of the properties on the Scope should have the same affinity as the Scope... For example, when using a
+    /// GlobalScopeStackContainer, anything you store on the scope will be applied to all events that get sent to Sentry
+    /// (no matter which thread they are sent from).
+    /// </para>
+    /// <para>
+    /// Transactions are an exception, however. We don't want spans from threads created on the UI thread to be added as
+    /// children of Transactions/Spans that get created on the background thread, or vice versa. As such,
+    /// Scope.Transaction is always stored as an AsyncLocal, regardless of the ScopeStackContainer implementation.
+    /// </para>
+    /// <para>
+    /// See https://github.com/getsentry/sentry-dotnet/issues/3590 for more information.
+    /// </para>
+    /// </summary>
+    private readonly AsyncLocal<ITransactionTracer?> _transaction = new();
 
     /// <summary>
-    /// Transaction.
+    /// The current Transaction
     /// </summary>
     public ITransactionTracer? Transaction
     {
-        get => _transaction;
-        set => _transaction = value;
+        get
+        {
+            _transactionLock.EnterReadLock();
+            try
+            {
+                return _transaction.Value;
+            }
+            finally
+            {
+                _transactionLock.ExitReadLock();
+            }
+        }
+        set
+        {
+            _transactionLock.EnterWriteLock();
+            try
+            {
+                _transaction.Value = value;
+            }
+            finally
+            {
+                _transactionLock.ExitWriteLock();
+            }
+        }
     }
 
     internal SentryPropagationContext PropagationContext { get; set; }
@@ -738,6 +780,24 @@ public void AddAttachment(string filePath, AttachmentType type = AttachmentType.
                 Path.GetFileName(filePath),
                 contentType));
 
-    internal void ResetTransaction(ITransactionTracer? expectedCurrentTransaction) =>
-        Interlocked.CompareExchange(ref _transaction, null, expectedCurrentTransaction);
+    /// <summary>
+    /// We need this lock to prevent a potential race condition in <see cref="ResetTransaction"/>.
+    /// </summary>
+    private readonly ReaderWriterLockSlim _transactionLock = new();
+
+    internal void ResetTransaction(ITransactionTracer? expectedCurrentTransaction)
+    {
+        _transactionLock.EnterWriteLock();
+        try
+        {
+            if (ReferenceEquals(_transaction.Value, expectedCurrentTransaction))
+            {
+                _transaction.Value = null;
+            }
+        }
+        finally
+        {
+            _transactionLock.ExitWriteLock();
+        }
+    }
 }

test/Sentry.Tests/Internals/SentryScopeManagerTests.cs

@@ -368,6 +368,32 @@ public void GlobalMode_PushScope_SameScope()
         client1.Should().BeSameAs(client2);
     }
 
+    /// <summary>
+    /// See https://github.com/getsentry/sentry-dotnet/issues/3590
+    /// </summary>
+    [Fact]
+    public void GlobalMode_ScopeTransaction_NotGlobal()
+    {
+        // Arrange
+        _fixture.SentryOptions.IsGlobalModeEnabled = true;
+        var sut = _fixture.GetSut();
+
+        // Act
+        Task.Run(() =>
+        {
+            var (scope, _) = sut.GetCurrent();
+            scope.SetExtra("Foo", "Bar");
+            scope.Transaction = Substitute.For<ITransactionTracer>();
+        });
+
+        // Assert
+        Task.Run(() =>
+        {
+            var (scope, _) = sut.GetCurrent();
+            scope.Extra["Foo"].Should().Be("Bar");  // In global mode most scope data should be shared
+            scope.Transaction.Should().BeNull();            // But transaction should be isolated
+        });
+    }
 
     [Fact]
     public void GlobalMode_Disabled_Uses_AsyncLocalScopeStackContainer()