From c4506dda0638298143b1862805f6e7b0899c3586 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 21 May 2026 11:45:03 +1200 Subject: [PATCH 1/8] spike: redesign TransactionTracer concurrency around an explicit state machine Replaces the implicit "IsFinished derived from EndTimestamp" model with a single int state field that transitions Running -> Finished atomically, inside the same critical section that writes EndTimestamp and disposes the idle timer. Closes the three open races flagged on PR #5138: 1. IsFinished/EndTimestamp torn view (deferred to #5174) 2. OnIdleTimeout TOCTOU between active-span check and Finish() call 3. _idleTimer.Cancel() being treated as authoritative Key design points: - IsFinished reads _state via Volatile.Read, not EndTimestamp. Anyone who observes IsFinished == true is guaranteed EndTimestamp is set and _spans is frozen. - One primitive: TryBeginFinish(fromIdleTimer, out shouldDiscard). All finish paths (Finish, OnIdleTimeout, Dispose) go through it. The Running -> Finished flip, the EndTimestamp write, and the idle-timer disposal happen inside one lock acquisition. - Idle-timer cancellation is now explicitly advisory. The authoritative defense against a stale firing is the PeekActive() re-check inside the lock: if a span arrived after the timer fired, the firing is rejected. - ChildSpanFinished keeps its fast-path no-op for non-idle transactions, so non-MAUI users pay no lock-acquisition cost on span finish. - _spans LINQ scan to compute trim-to-last-span EndTimestamp now happens inside the lock, so it can no longer race with span mutations. - External EndTimestamp setter routes through the lock for back-compat with tests and serialization round-trip. No tests regress (Sentry.Tests: 2363/2363, Sentry.Maui.Tests: 96/96). Co-Authored-By: Claude Sonnet 4.6 --- src/Sentry/TransactionTracer.cs | 196 +++++++++++++++++++++++--------- 1 file changed, 144 insertions(+), 52 deletions(-) diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs index 8f1f3954b2..5b14180b42 100644 --- a/src/Sentry/TransactionTracer.cs +++ b/src/Sentry/TransactionTracer.cs @@ -17,8 +17,23 @@ public sealed class TransactionTracer : IBaseTracer, IAutoTimeoutTracer, ITransa private readonly ISentryTimer? _idleTimer; private readonly TimeSpan? _idleTimeout; private readonly SentryStopwatch _stopwatch = SentryStopwatch.StartNew(); - private bool _hasFinished; - private readonly object _finishLock = new(); + + // Lifecycle state machine. + // + // The transaction is either Running or Finished. The Running -> Finished transition + // happens exactly once, inside `_lock`, atomically with assigning `_endTimestamp` and + // disposing the idle timer. After the transition, no further spans are accepted and + // observers see a consistent view: `IsFinished == true` AND `EndTimestamp != null` AND + // `_spans` is frozen. + // + // `IsFinished` reads `_state` (not `EndTimestamp`) so the two observables can never + // diverge. The public `EndTimestamp` setter exists for back-compat with the existing + // API surface (tests, serialization round-trip); it routes writes through `_lock` so + // external use cannot create a torn view. + private const int StateRunning = 0; + private const int StateFinished = 1; + private int _state = StateRunning; + private readonly object _lock = new(); private readonly Instrumenter _instrumenter = Instrumenter.Sentry; @@ -69,8 +84,23 @@ public SentryId TraceId /// public DateTimeOffset StartTimestamp { get; internal set; } + // Written exactly once during the Running -> Finished transition, inside `_lock`. + // External writes via the setter also go through `_lock`. Reads are lock-free because + // the field becomes monotonically stable (non-null) the moment `_state == Finished`. + private DateTimeOffset? _endTimestamp; + /// - public DateTimeOffset? EndTimestamp { get; internal set; } + public DateTimeOffset? EndTimestamp + { + get => _endTimestamp; + internal set + { + lock (_lock) + { + _endTimestamp = value; + } + } + } /// public string Operation @@ -183,7 +213,11 @@ public IReadOnlyList Fingerprint internal bool HasMetrics => _metricsSummary.IsValueCreated; /// - public bool IsFinished => EndTimestamp is not null; + // Derived from the lifecycle state field, not from `EndTimestamp`. This is the key + // invariant: anyone who observes `IsFinished == true` is guaranteed that `_state` has + // already transitioned, which means `_endTimestamp` was set inside the same critical + // section and `_spans` will no longer mutate. + public bool IsFinished => Volatile.Read(ref _state) != StateRunning; internal DynamicSamplingContext? DynamicSamplingContext { get; set; } @@ -252,6 +286,15 @@ internal TransactionTracer(IHub hub, ITransactionContext context, TimeSpan? idle } } + // The idle-timer callback. Runs on the ThreadPool. The whole "should we finish?" + // decision happens inside one critical section so that a concurrent AddChildSpan + // cannot slip a new span in between deciding-to-finish and actually-finishing. + // + // `Timer.Change(Infinite, ...)` (i.e. `_idleTimer.Cancel()`) cannot stop a callback + // that has already begun executing — so cancellation is advisory. The defense against + // a stale firing is the `PeekActive() != null` re-check inside the lock: if a span + // arrived after the timer fired but before we acquired the lock, we silently bail + // and let AddChildSpan's `Cancel()` (which already ran) keep the timer paused. private void OnIdleTimeout() { if (IsSentryRequest) @@ -260,21 +303,9 @@ private void OnIdleTimeout() return; } - // Discard if no child spans were ever started - bool shouldDiscard; - lock (_finishLock) + if (!TryBeginFinish(fromIdleTimer: true, out var shouldDiscard)) { - if (_spans.IsEmpty && !_hasFinished) - { - _hasFinished = true; - _idleTimer?.Dispose(); - EndTimestamp = _stopwatch.CurrentDateTimeOffset; - shouldDiscard = true; - } - else - { - shouldDiscard = false; - } + return; } if (shouldDiscard) @@ -284,7 +315,8 @@ private void OnIdleTimeout() return; } - Finish(Status ?? SpanStatus.Ok); + Status ??= SpanStatus.Ok; + CompleteCapture(); } /// @@ -322,26 +354,30 @@ internal ISpan StartChild(SpanId? spanId, SpanId parentSpanId, string operation, return span; } + // All decisions about whether to accept the span happen inside the same critical + // section as the actual mutation of `_spans`/`_activeSpanTracker`/`_idleTimer`. This + // closes both the span-limit TOCTOU and the "transaction finished between the check + // and the add" race. private void AddChildSpan(SpanTracer span) { - lock (_finishLock) + lock (_lock) { - if (_spans.Count >= SpanLimit) + if (_state != StateRunning) { - _options?.LogDebug("Discarding child span '{0}' due to {1} span limit", SpanId, SpanLimit); + _options?.LogDebug("Discarding child span '{0}' as the trace has already finished", SpanId); span.IsSampled = false; return; } - if (_hasFinished) + if (_spans.Count >= SpanLimit) { - _options?.LogDebug("Discarding child span '{0}' as the trace has already finished", SpanId); + _options?.LogDebug("Discarding child span '{0}' due to {1} span limit", SpanId, SpanLimit); span.IsSampled = false; return; } span.IsSampled = IsSampled; - _idleTimer?.Cancel(); // Pause the idle timer while a child span is in flight + _idleTimer?.Cancel(); // Pause the idle timer while a child span is in flight (advisory) _spans.Add(span); _activeSpanTracker.Push(span); } @@ -350,14 +386,16 @@ private void AddChildSpan(SpanTracer span) internal void ChildSpanFinished() { // Fast path: only idle-timeout transactions need to react to child finishes. - // _idleTimeout is readonly, so this check is safe outside the lock. + // `_idleTimeout` is readonly, so this check is safe outside the lock and avoids + // lock-acquisition overhead for the (overwhelming) majority of transactions that + // do not use an idle timer. if (!_idleTimeout.HasValue) { return; } - lock (_finishLock) + lock (_lock) { - if (_hasFinished) + if (_state != StateRunning) { return; } @@ -418,9 +456,9 @@ public void Clear() void IAutoTimeoutTracer.ResetIdleTimeout() { - lock (_finishLock) + lock (_lock) { - if (!_idleTimeout.HasValue || _hasFinished) + if (!_idleTimeout.HasValue || _state != StateRunning) { return; } @@ -428,17 +466,77 @@ void IAutoTimeoutTracer.ResetIdleTimeout() } } - private bool TryFinishOnce() + /// + /// The single atomic primitive for transitioning Running -> Finished. Returns true to + /// exactly one caller; all subsequent calls return false. The Running -> Finished flip, + /// the `EndTimestamp` write, and the timer disposal happen inside the same critical + /// section, so no observer can ever see an inconsistent intermediate state. + /// + /// + /// True when called from the idle-timer callback. In that case we also re-check whether + /// a child span is still active (it might have been added after the timer fired but + /// before we acquired the lock); if so, the firing is stale and we refuse to finish. + /// + /// + /// True if the transaction had no child spans and the caller (idle timer) should + /// discard rather than capture. + /// + private bool TryBeginFinish(bool fromIdleTimer, out bool shouldDiscard) { - lock (_finishLock) + shouldDiscard = false; + lock (_lock) { - if (_hasFinished) + if (_state != StateRunning) { return false; } - _hasFinished = true; + + if (fromIdleTimer) + { + // Defensive re-check: if a child span arrived between the timer firing and + // us acquiring the lock, AddChildSpan's `Cancel()` may have failed to stop + // the in-flight callback. The active-span check inside the lock is the + // authoritative answer. + if (_activeSpanTracker.PeekActive() != null) + { + return false; + } + + if (_spans.IsEmpty) + { + shouldDiscard = true; + } + } + + // Compute the end timestamp inside the lock. For idle transactions, trim to + // the latest finished child span when available. (Scanning `_spans` here is + // bounded by SpanLimit and only runs once per transaction.) + DateTimeOffset endTimestamp; + if (_idleTimeout.HasValue && !shouldDiscard) + { + DateTimeOffset latest = default; + foreach (var s in _spans) + { + if (s.IsFinished && s.EndTimestamp is { } et && et > latest) + { + latest = et; + } + } + endTimestamp = latest == default ? _stopwatch.CurrentDateTimeOffset : latest; + } + else + { + endTimestamp = _endTimestamp ?? _stopwatch.CurrentDateTimeOffset; + } + + _endTimestamp = endTimestamp; _idleTimer?.Cancel(); _idleTimer?.Dispose(); + + // State flip is the LAST write inside the lock. Use Volatile to publish the + // store; readers using `IsFinished` will, on observing `Finished`, also see + // the prior `_endTimestamp` write (release-store / acquire-load semantics). + Volatile.Write(ref _state, StateFinished); return true; } } @@ -446,7 +544,7 @@ private bool TryFinishOnce() /// public void Finish() { - if (!TryFinishOnce()) + if (!TryBeginFinish(fromIdleTimer: false, out _)) { return; } @@ -456,33 +554,27 @@ public void Finish() { // Normally we wouldn't start transactions for Sentry requests but when instrumenting with OpenTelemetry // we are only able to determine whether it's a sentry request or not when closing a span... we leave these - // to be garbage collected and we don't want idle timers triggering on them + // to be garbage collected. The idle timer has already been disposed inside TryBeginFinish. _options?.LogDebug("Transaction '{0}' is a Sentry Request. Don't complete.", SpanId); return; } - TransactionProfiler?.Finish(); Status ??= SpanStatus.Ok; + CompleteCapture(); + } - // For idle transactions, trim end time to the last finished child span - if (_idleTimeout.HasValue) - { - var latestSpanEnd = _spans - .Where(s => s.IsFinished) - .Select(s => s.EndTimestamp) - .DefaultIfEmpty() - .Max(); - EndTimestamp = latestSpanEnd ?? _stopwatch.CurrentDateTimeOffset; - } - else - { - EndTimestamp ??= _stopwatch.CurrentDateTimeOffset; - } + // The non-locked work that follows a successful Running -> Finished transition: stop + // the profiler, reset scope, capture the transaction, and release tracked spans. By + // the time we get here, `_state == Finished` and `_endTimestamp` is set, so any + // concurrent reader sees a coherent finished transaction. + private void CompleteCapture() + { + TransactionProfiler?.Finish(); _options?.LogDebug("Finished Transaction '{0}'.", SpanId); // Clear the transaction from the scope and regenerate the Propagation Context - // We do this so new events don't have a trace context that is "older" than the transaction that just finished + // so new events don't have a trace context that is "older" than the transaction that just finished _hub.ConfigureScope(static (scope, transactionTracer) => scope.ResetTransaction(transactionTracer), this); // Client decides whether to discard this transaction based on sampling From b4309ab36eab99dd6c7501b0165d33ea4f2f056d Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 21 May 2026 14:24:15 +1200 Subject: [PATCH 2/8] spike: simplify state machine to a single bool Two states don't need an int + named constants. `Volatile.Read`/`Write` work on bool in C#, so the memory-ordering property is preserved. Co-Authored-By: Claude Sonnet 4.6 --- src/Sentry/TransactionTracer.cs | 51 +++++++++++++++------------------ 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs index 5b14180b42..12a27a8a36 100644 --- a/src/Sentry/TransactionTracer.cs +++ b/src/Sentry/TransactionTracer.cs @@ -18,21 +18,16 @@ public sealed class TransactionTracer : IBaseTracer, IAutoTimeoutTracer, ITransa private readonly TimeSpan? _idleTimeout; private readonly SentryStopwatch _stopwatch = SentryStopwatch.StartNew(); - // Lifecycle state machine. + // The Running -> Finished transition happens exactly once, inside `_lock`, atomically + // with assigning `_endTimestamp` and disposing the idle timer. After the transition, no + // further spans are accepted and observers see a consistent view: `IsFinished == true` + // AND `EndTimestamp != null` AND `_spans` is frozen. // - // The transaction is either Running or Finished. The Running -> Finished transition - // happens exactly once, inside `_lock`, atomically with assigning `_endTimestamp` and - // disposing the idle timer. After the transition, no further spans are accepted and - // observers see a consistent view: `IsFinished == true` AND `EndTimestamp != null` AND - // `_spans` is frozen. - // - // `IsFinished` reads `_state` (not `EndTimestamp`) so the two observables can never - // diverge. The public `EndTimestamp` setter exists for back-compat with the existing - // API surface (tests, serialization round-trip); it routes writes through `_lock` so - // external use cannot create a torn view. - private const int StateRunning = 0; - private const int StateFinished = 1; - private int _state = StateRunning; + // `IsFinished` reads `_hasFinished` (not `EndTimestamp`) so the two observables can + // never diverge. The public `EndTimestamp` setter exists for back-compat with the + // existing API surface (tests, serialization round-trip); it routes writes through + // `_lock` so external use cannot create a torn view. + private bool _hasFinished; private readonly object _lock = new(); private readonly Instrumenter _instrumenter = Instrumenter.Sentry; @@ -86,7 +81,7 @@ public SentryId TraceId // Written exactly once during the Running -> Finished transition, inside `_lock`. // External writes via the setter also go through `_lock`. Reads are lock-free because - // the field becomes monotonically stable (non-null) the moment `_state == Finished`. + // the field becomes monotonically stable (non-null) the moment `_hasFinished` flips. private DateTimeOffset? _endTimestamp; /// @@ -213,11 +208,11 @@ public IReadOnlyList Fingerprint internal bool HasMetrics => _metricsSummary.IsValueCreated; /// - // Derived from the lifecycle state field, not from `EndTimestamp`. This is the key - // invariant: anyone who observes `IsFinished == true` is guaranteed that `_state` has - // already transitioned, which means `_endTimestamp` was set inside the same critical + // Derived from the lifecycle flag, not from `EndTimestamp`. This is the key invariant: + // anyone who observes `IsFinished == true` is guaranteed that the transition flip + // already happened, which means `_endTimestamp` was set inside the same critical // section and `_spans` will no longer mutate. - public bool IsFinished => Volatile.Read(ref _state) != StateRunning; + public bool IsFinished => Volatile.Read(ref _hasFinished); internal DynamicSamplingContext? DynamicSamplingContext { get; set; } @@ -362,7 +357,7 @@ private void AddChildSpan(SpanTracer span) { lock (_lock) { - if (_state != StateRunning) + if (_hasFinished) { _options?.LogDebug("Discarding child span '{0}' as the trace has already finished", SpanId); span.IsSampled = false; @@ -395,7 +390,7 @@ internal void ChildSpanFinished() } lock (_lock) { - if (_state != StateRunning) + if (_hasFinished) { return; } @@ -458,7 +453,7 @@ void IAutoTimeoutTracer.ResetIdleTimeout() { lock (_lock) { - if (!_idleTimeout.HasValue || _state != StateRunning) + if (!_idleTimeout.HasValue || _hasFinished) { return; } @@ -486,7 +481,7 @@ private bool TryBeginFinish(bool fromIdleTimer, out bool shouldDiscard) shouldDiscard = false; lock (_lock) { - if (_state != StateRunning) + if (_hasFinished) { return false; } @@ -533,10 +528,10 @@ private bool TryBeginFinish(bool fromIdleTimer, out bool shouldDiscard) _idleTimer?.Cancel(); _idleTimer?.Dispose(); - // State flip is the LAST write inside the lock. Use Volatile to publish the - // store; readers using `IsFinished` will, on observing `Finished`, also see - // the prior `_endTimestamp` write (release-store / acquire-load semantics). - Volatile.Write(ref _state, StateFinished); + // The flip is the LAST write inside the lock. Use Volatile to publish the + // store; readers using `IsFinished` will, on observing `true`, also see the + // prior `_endTimestamp` write (release-store / acquire-load semantics). + Volatile.Write(ref _hasFinished, true); return true; } } @@ -565,7 +560,7 @@ public void Finish() // The non-locked work that follows a successful Running -> Finished transition: stop // the profiler, reset scope, capture the transaction, and release tracked spans. By - // the time we get here, `_state == Finished` and `_endTimestamp` is set, so any + // the time we get here, `_hasFinished == true` and `_endTimestamp` is set, so any // concurrent reader sees a coherent finished transaction. private void CompleteCapture() { From b352f58e2bedef9820f675e161f3a16828f951dd Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 21 May 2026 15:33:40 +1200 Subject: [PATCH 3/8] Tidy up the comments --- src/Sentry/TransactionTracer.cs | 65 ++++++++++----------------------- 1 file changed, 20 insertions(+), 45 deletions(-) diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs index 12a27a8a36..026dad4eb4 100644 --- a/src/Sentry/TransactionTracer.cs +++ b/src/Sentry/TransactionTracer.cs @@ -18,17 +18,13 @@ public sealed class TransactionTracer : IBaseTracer, IAutoTimeoutTracer, ITransa private readonly TimeSpan? _idleTimeout; private readonly SentryStopwatch _stopwatch = SentryStopwatch.StartNew(); - // The Running -> Finished transition happens exactly once, inside `_lock`, atomically - // with assigning `_endTimestamp` and disposing the idle timer. After the transition, no - // further spans are accepted and observers see a consistent view: `IsFinished == true` - // AND `EndTimestamp != null` AND `_spans` is frozen. - // - // `IsFinished` reads `_hasFinished` (not `EndTimestamp`) so the two observables can - // never diverge. The public `EndTimestamp` setter exists for back-compat with the - // existing API surface (tests, serialization round-trip); it routes writes through - // `_lock` so external use cannot create a torn view. + /// + /// Set exactly once inside the `_lock` at the same time as setting `_endTimestamp` and disposing the idle timer. + /// `IsFinished` makes a volatile read of `_hasFinished` (no lock required as would be necessary for the more + /// complex `_endTimestamp` struct), which is essentially why we need this separate flag. + /// private bool _hasFinished; - private readonly object _lock = new(); + private readonly Lock _lock = new(); private readonly Instrumenter _instrumenter = Instrumenter.Sentry; @@ -79,15 +75,16 @@ public SentryId TraceId /// public DateTimeOffset StartTimestamp { get; internal set; } - // Written exactly once during the Running -> Finished transition, inside `_lock`. - // External writes via the setter also go through `_lock`. Reads are lock-free because - // the field becomes monotonically stable (non-null) the moment `_hasFinished` flips. + /// + /// Guard writes with `_lock` + /// private DateTimeOffset? _endTimestamp; /// public DateTimeOffset? EndTimestamp { - get => _endTimestamp; + // get => _endTimestamp; + get => Volatile.Read(ref _hasFinished) ? _endTimestamp : null; internal set { lock (_lock) @@ -208,10 +205,6 @@ public IReadOnlyList Fingerprint internal bool HasMetrics => _metricsSummary.IsValueCreated; /// - // Derived from the lifecycle flag, not from `EndTimestamp`. This is the key invariant: - // anyone who observes `IsFinished == true` is guaranteed that the transition flip - // already happened, which means `_endTimestamp` was set inside the same critical - // section and `_spans` will no longer mutate. public bool IsFinished => Volatile.Read(ref _hasFinished); internal DynamicSamplingContext? DynamicSamplingContext { get; set; } @@ -281,15 +274,6 @@ internal TransactionTracer(IHub hub, ITransactionContext context, TimeSpan? idle } } - // The idle-timer callback. Runs on the ThreadPool. The whole "should we finish?" - // decision happens inside one critical section so that a concurrent AddChildSpan - // cannot slip a new span in between deciding-to-finish and actually-finishing. - // - // `Timer.Change(Infinite, ...)` (i.e. `_idleTimer.Cancel()`) cannot stop a callback - // that has already begun executing — so cancellation is advisory. The defense against - // a stale firing is the `PeekActive() != null` re-check inside the lock: if a span - // arrived after the timer fired but before we acquired the lock, we silently bail - // and let AddChildSpan's `Cancel()` (which already ran) keep the timer paused. private void OnIdleTimeout() { if (IsSentryRequest) @@ -349,10 +333,6 @@ internal ISpan StartChild(SpanId? spanId, SpanId parentSpanId, string operation, return span; } - // All decisions about whether to accept the span happen inside the same critical - // section as the actual mutation of `_spans`/`_activeSpanTracker`/`_idleTimer`. This - // closes both the span-limit TOCTOU and the "transaction finished between the check - // and the add" race. private void AddChildSpan(SpanTracer span) { lock (_lock) @@ -372,7 +352,7 @@ private void AddChildSpan(SpanTracer span) } span.IsSampled = IsSampled; - _idleTimer?.Cancel(); // Pause the idle timer while a child span is in flight (advisory) + _idleTimer?.Cancel(); _spans.Add(span); _activeSpanTracker.Push(span); } @@ -381,9 +361,7 @@ private void AddChildSpan(SpanTracer span) internal void ChildSpanFinished() { // Fast path: only idle-timeout transactions need to react to child finishes. - // `_idleTimeout` is readonly, so this check is safe outside the lock and avoids - // lock-acquisition overhead for the (overwhelming) majority of transactions that - // do not use an idle timer. + // `_idleTimeout` is readonly, so this check is safe outside the lock if (!_idleTimeout.HasValue) { return; @@ -464,13 +442,13 @@ void IAutoTimeoutTracer.ResetIdleTimeout() /// /// The single atomic primitive for transitioning Running -> Finished. Returns true to /// exactly one caller; all subsequent calls return false. The Running -> Finished flip, - /// the `EndTimestamp` write, and the timer disposal happen inside the same critical + /// the `EndTimestamp` write, and the timer disposal all happen inside the same critical /// section, so no observer can ever see an inconsistent intermediate state. /// /// /// True when called from the idle-timer callback. In that case we also re-check whether /// a child span is still active (it might have been added after the timer fired but - /// before we acquired the lock); if so, the firing is stale and we refuse to finish. + /// before we acquired the lock); if so, the firing is stale, and we refuse to finish. /// /// /// True if the transaction had no child spans and the caller (idle timer) should @@ -528,9 +506,7 @@ private bool TryBeginFinish(bool fromIdleTimer, out bool shouldDiscard) _idleTimer?.Cancel(); _idleTimer?.Dispose(); - // The flip is the LAST write inside the lock. Use Volatile to publish the - // store; readers using `IsFinished` will, on observing `true`, also see the - // prior `_endTimestamp` write (release-store / acquire-load semantics). + // MUST be the last write inside the lock for all logic that depends on volatile reads to work Volatile.Write(ref _hasFinished, true); return true; } @@ -558,10 +534,9 @@ public void Finish() CompleteCapture(); } - // The non-locked work that follows a successful Running -> Finished transition: stop - // the profiler, reset scope, capture the transaction, and release tracked spans. By - // the time we get here, `_hasFinished == true` and `_endTimestamp` is set, so any - // concurrent reader sees a coherent finished transaction. + /// + /// Performs non-locked work that follows a successful Running -> Finished transition + /// private void CompleteCapture() { TransactionProfiler?.Finish(); @@ -569,7 +544,7 @@ private void CompleteCapture() _options?.LogDebug("Finished Transaction '{0}'.", SpanId); // Clear the transaction from the scope and regenerate the Propagation Context - // so new events don't have a trace context that is "older" than the transaction that just finished + // We do this so new events don't have a trace context that is "older" than the transaction that just finished _hub.ConfigureScope(static (scope, transactionTracer) => scope.ResetTransaction(transactionTracer), this); // Client decides whether to discard this transaction based on sampling From 608b5ba3fdad9bf0cd1a66c9f0085b6ab7f56eb5 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 21 May 2026 16:32:28 +1200 Subject: [PATCH 4/8] Remove redundant inner lock from LastActiveSpanTracker All internal callers (AddChildSpan, ChildSpanFinished, TryBeginFinish, ReleaseSpans) already hold the enclosing TransactionTracer._lock when they touch the tracker. The public GetLastActiveSpan acquires _lock explicitly so it gets the same guarantee. Co-Authored-By: Claude Sonnet 4.6 --- src/Sentry/TransactionTracer.cs | 46 ++++++++++++++------------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs index 026dad4eb4..cc6443aa2c 100644 --- a/src/Sentry/TransactionTracer.cs +++ b/src/Sentry/TransactionTracer.cs @@ -381,51 +381,43 @@ internal void ChildSpanFinished() } } + // All callers (`AddChildSpan`, `ChildSpanFinished`, `TryBeginFinish`, `ReleaseSpans`, + // and the public `GetLastActiveSpan` below) acquire the enclosing `TransactionTracer._lock` + // before touching this tracker, so it does not need its own lock. private class LastActiveSpanTracker { - private readonly Lock _lock = new(); - private readonly Lazy> _trackedSpans = new(); private Stack TrackedSpans => _trackedSpans.Value; - public void Push(ISpan span) - { - lock (_lock) - { - TrackedSpans.Push(span); - } - } + public void Push(ISpan span) => TrackedSpans.Push(span); public ISpan? PeekActive() { - lock (_lock) + while (TrackedSpans.Count > 0) { - while (TrackedSpans.Count > 0) + // Stop tracking inactive spans + var span = TrackedSpans.Peek(); + if (!span.IsFinished) { - // Stop tracking inactive spans - var span = TrackedSpans.Peek(); - if (!span.IsFinished) - { - return span; - } - TrackedSpans.Pop(); + return span; } - return null; + TrackedSpans.Pop(); } + return null; } - public void Clear() - { - lock (_lock) - { - TrackedSpans.Clear(); - } - } + public void Clear() => TrackedSpans.Clear(); } private readonly LastActiveSpanTracker _activeSpanTracker = new LastActiveSpanTracker(); /// - public ISpan? GetLastActiveSpan() => _activeSpanTracker.PeekActive(); + public ISpan? GetLastActiveSpan() + { + lock (_lock) + { + return _activeSpanTracker.PeekActive(); + } + } void IAutoTimeoutTracer.ResetIdleTimeout() { From 761c30e2afe81d51f2be1ed3d84fc3d3485603ad Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 21 May 2026 16:35:00 +1200 Subject: [PATCH 5/8] Remove unnecessary comment --- src/Sentry/TransactionTracer.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs index cc6443aa2c..85e055db07 100644 --- a/src/Sentry/TransactionTracer.cs +++ b/src/Sentry/TransactionTracer.cs @@ -381,9 +381,6 @@ internal void ChildSpanFinished() } } - // All callers (`AddChildSpan`, `ChildSpanFinished`, `TryBeginFinish`, `ReleaseSpans`, - // and the public `GetLastActiveSpan` below) acquire the enclosing `TransactionTracer._lock` - // before touching this tracker, so it does not need its own lock. private class LastActiveSpanTracker { private readonly Lazy> _trackedSpans = new(); From 464e847985946911b582a26f04409fb3a7d4b4d3 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 21 May 2026 16:58:24 +1200 Subject: [PATCH 6/8] Tidy StartUiTransaction --- src/Sentry.Maui/Internal/MauiEventsBinder.cs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Sentry.Maui/Internal/MauiEventsBinder.cs b/src/Sentry.Maui/Internal/MauiEventsBinder.cs index c796312968..90f5b50a8b 100644 --- a/src/Sentry.Maui/Internal/MauiEventsBinder.cs +++ b/src/Sentry.Maui/Internal/MauiEventsBinder.cs @@ -420,9 +420,8 @@ internal void StartUiTransaction(string name) if (CurrentUiTx is not null) { // Idle timer will clean up any previous UI transaction, but we don't want any more child spans on it - _hub.ConfigureScope(scope => scope.ResetTransaction(CurrentUiTx)); + _hub.ConfigureScope(static (scope, tx) => scope.ResetTransaction(tx), CurrentUiTx); } - CurrentUiTx = null; var context = new TransactionContext(name, UserInteractionClickOp) { @@ -430,11 +429,10 @@ internal void StartUiTransaction(string name) }; CurrentUiTx = _hub is IHubInternal internalHub ? internalHub.StartTransaction(context, _options.AutoTransactionIdleTimeout) - // never called in practice... all our hubs implement IHubInternal : _hub.StartTransaction(context); - // Only bind to scope if there is no other transaction already there (user-created or SDK-owned navigation). - _hub.ConfigureScope(scope => scope.Transaction ??= CurrentUiTx); + // Bind to scope only if no other transaction is already there (user-installed or SDK-owned navigation). + _hub.ConfigureScope(static (scope, tx) => scope.Transaction ??= tx, CurrentUiTx); } // Application Events @@ -472,13 +470,13 @@ private void OnWindowOnStopped(object? sender, EventArgs _) if (uiTx.Spans.Count > 0) { // Only finish UI transactions with child spans. - // Childless transactions will be discarded by the idle timeout. + // Childless transactions get discarded by the idle timeout. uiTx.Finish(SpanStatus.Ok); } else { - // No child spans, so just detach from scope and let idle timeout handle it. - _hub.ConfigureScope(scope => scope.ResetTransaction(CurrentUiTx)); + // No child spans, so just detach from scope and let idle timeout handle it + _hub.ConfigureScope(static (scope, tx) => scope.ResetTransaction(tx), uiTx); } } CurrentUiTx = null; From df89335511a9d08ab57b956bb018a3a047faa35b Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 21 May 2026 17:24:13 +1200 Subject: [PATCH 7/8] Address SpanTracer concurrency follow-up Two changes that share a root cause: - SpanTracer.EndTimestamp: gate the getter on a Volatile bool so lock-free readers see a consistent (timestamp, finished) pair. Mirror of the pattern already used on TransactionTracer; toggles via Unfinish so the setter sequences writes in opposite orders for null vs non-null values. - LastActiveSpanTracker.PeekActive: stop popping finished spans off the stack. The destructive pop combined with SpanTracer.Unfinish() created a hole where an unfinished span could no longer be tracked, letting the idle timer finish the transaction while a logically-live span was still in flight. Now non-destructive; an unfinished span stays in the tracker and is re-discoverable after Unfinish(). The 6-step reproduction is documented on the regression test (IdleTimeout_AfterUnfinishedChildSpan_DoesNotCaptureTransaction) so the implementation stays clean. Co-Authored-By: Claude Sonnet 4.6 --- src/Sentry/SpanTracer.cs | 25 ++++++++++++++-- src/Sentry/TransactionTracer.cs | 6 ++-- test/Sentry.Tests/TransactionTracerTests.cs | 33 +++++++++++++++++++++ 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/src/Sentry/SpanTracer.cs b/src/Sentry/SpanTracer.cs index 608e0b404c..d79cb1a41b 100644 --- a/src/Sentry/SpanTracer.cs +++ b/src/Sentry/SpanTracer.cs @@ -34,11 +34,32 @@ public sealed class SpanTracer : IBaseTracer, ISpan /// public DateTimeOffset StartTimestamp { get; internal set; } + private DateTimeOffset? _endTimestamp; + private bool _isFinished; + /// - public DateTimeOffset? EndTimestamp { get; internal set; } + public DateTimeOffset? EndTimestamp + { + get => Volatile.Read(ref _isFinished) ? _endTimestamp : null; + internal set + { + // Ordering is load-bearing: the gate-flip is release-ordered against the data write, + // so lock-free readers gating on `_isFinished` see a consistent (timestamp, finished) pair. + if (value is null) + { + Volatile.Write(ref _isFinished, false); + _endTimestamp = null; + } + else + { + _endTimestamp = value; + Volatile.Write(ref _isFinished, true); + } + } + } /// - public bool IsFinished => EndTimestamp is not null; + public bool IsFinished => Volatile.Read(ref _isFinished); // Not readonly because of deserialization internal Dictionary? InternalMeasurements { get; private set; } diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs index 85e055db07..b899c315cc 100644 --- a/src/Sentry/TransactionTracer.cs +++ b/src/Sentry/TransactionTracer.cs @@ -390,15 +390,13 @@ private class LastActiveSpanTracker public ISpan? PeekActive() { - while (TrackedSpans.Count > 0) + // Non-destructive: leave finished spans in place so SpanTracer.Unfinish() can resurrect them. + foreach (var span in TrackedSpans) { - // Stop tracking inactive spans - var span = TrackedSpans.Peek(); if (!span.IsFinished) { return span; } - TrackedSpans.Pop(); } return null; } diff --git a/test/Sentry.Tests/TransactionTracerTests.cs b/test/Sentry.Tests/TransactionTracerTests.cs index 6aaa408e19..dc859bcca1 100644 --- a/test/Sentry.Tests/TransactionTracerTests.cs +++ b/test/Sentry.Tests/TransactionTracerTests.cs @@ -216,4 +216,37 @@ public void LastSpan_Finish_ThenTimerFires_CapturesTransaction() // Then the transaction is captured hub.Received(1).CaptureTransaction(Arg.Any()); } + + [Fact] + public void IdleTimeout_AfterUnfinishedChildSpan_DoesNotCaptureTransaction() + { + // Regression: prior to the fix, LastActiveSpanTracker.PeekActive() destructively popped + // finished spans off the stack. That created a hole when combined with SpanTracer.Unfinish() + // (used by EF for connection-pool reuse): + // + // 1. SpanTracer.Finish() sets EndTimestamp and calls Transaction.ChildSpanFinished(). + // 2. ChildSpanFinished -> PeekActive() destructively pops the now-finished span. + // 3. Stack empty -> idle timer (re)starts. + // 4. SpanTracer.Unfinish() resets EndTimestamp = null on the span -- it is logically alive + // again, but no longer in _activeSpanTracker. + // 5. PeekActive() returns null -- the idle timer fires unopposed. + // 6. TryBeginFinish sees PeekActive() == null and _spans.Count > 0, captures the transaction + // while the unfinished span is still in-flight. + // + // Fix: PeekActive is non-destructive; an unfinished span stays in the tracker and is + // re-discoverable after Unfinish(). + + // Given an auto-generated UI event transaction with a child span that is finished then unfinished + var hub = Substitute.For(); + var (transaction, timer) = CreateIdleTransaction(hub); + var span = (SpanTracer)transaction.StartChild("child"); + span.Finish(); + span.Unfinish(); + + // When the idle timer fires + timer.Fire(); + + // Then the transaction is NOT captured because the unfinished span is still tracked as active + hub.DidNotReceive().CaptureTransaction(Arg.Any()); + } } From beaa1884d208834ba14831d53265321096f39abe Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 21 May 2026 19:57:40 +1200 Subject: [PATCH 8/8] Sync Net4_8 API approval snapshot with actual generated surface Commit 4b482a79 added OrgId and StrictTraceContinuation to the Net4_8 verified file, but the underlying properties live in source that exists on main and not on this branch. The Windows CI is now flagging the mismatch on every run; removing the two stale lines restores parity with what PublicApiGenerator actually produces for Sentry.dll on this branch. Co-Authored-By: Claude Sonnet 4.6 --- test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt index f9068edea7..1fc1e68f4b 100644 --- a/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt +++ b/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt @@ -812,7 +812,6 @@ namespace Sentry public int MaxCacheItems { get; set; } public int MaxQueueItems { get; set; } public Sentry.Extensibility.INetworkStatusListener? NetworkStatusListener { get; set; } - public string? OrgId { get; set; } public double? ProfilesSampleRate { get; set; } public bool PropagateTraceparent { get; set; } public string? Release { get; set; } @@ -828,7 +827,6 @@ namespace Sentry public System.TimeSpan ShutdownTimeout { get; set; } public string SpotlightUrl { get; set; } public Sentry.StackTraceMode StackTraceMode { get; set; } - public bool StrictTraceContinuation { get; set; } public System.Collections.Generic.IList TagFilters { get; set; } public System.Collections.Generic.IList TraceIgnoreStatusCodes { get; set; } public System.Collections.Generic.IList TracePropagationTargets { get; set; }