TransactionTracer concurrency for Maui Transactions

[jamescrosswell]

Playing around with a concurrency solution for the three concurrency threads that keep getting re-flagged on #5138:

Thread	Status on #5138 head	Resolved here by
IsFinished / EndTimestamp torn view (sentry-bot, deferred to #5174)	Open	IsFinished reads a Volatile bool, not the nullable struct
OnIdleTimeout TOCTOU — Warden flagged 3× across 3 commits (b74a6c3, f5d222e, e13985b)	Open	Single TryBeginFinish critical section; PeekActive re-check inside the lock
_idleTimer.Cancel() treated as authoritative	Open	Cancellation is explicitly advisory; correctness comes from the in-lock re-check

Solution

Treat TransactionTracer as a one-shot state machine (Running → Finished). The transition, the EndTimestamp write, and the idle-timer disposal happen in one critical section, and IsFinished reads a separate bool flag so observers can never see an inconsistent intermediate view.

What changed

TransactionTracer

• IsFinished reads bool _hasFinished via Volatile.Read, not the Nullable<DateTimeOffset> EndTimestamp. EndTimestamp is gated on the same flag so lock-free readers see either null or the final stable value, never an in-progress write.
• TryBeginFinish(fromIdleTimer, out shouldDiscard) is the only path from Running → Finished. Inside one lock acquisition it: checks the one-shot guard, re-checks PeekActive() == null for idle-timer callers (the authoritative defense against a stale firing — Timer.Change(Infinite) cannot stop an in-flight callback), computes the trimmed EndTimestamp, disposes the timer, and flips _hasFinished last via Volatile.Write.
• Finish() and OnIdleTimeout() are thin wrappers around TryBeginFinish + CompleteCapture. Capture work (profiler stop, scope reset, CaptureTransaction, ReleaseSpans) runs outside the lock but only after the flag flip; concurrent AddChildSpan / ChildSpanFinished see _hasFinished and bail.
• ChildSpanFinished keeps its fast-path no-op for non-idle transactions — no lock acquisition in the common non-MAUI case.
• LastActiveSpanTracker: inner lock removed (all callers hold _lock); PeekActive is now non-destructive so SpanTracer.Unfinish() can resurrect a finished span without it falling out of the tracker.

SpanTracer

• Same Volatile-gated EndTimestamp / IsFinished pattern. The setter sequences writes in opposite orders for null vs non-null so the gate stays consistent through both Finish and Unfinish.

MauiEventsBinder

• StartUiTransaction and OnWindowOnStopped use the (scope, state) ConfigureScope overload with static lambdas — no closure allocations, no risk of the lambda reading a different CurrentUiTx than the caller intended.

Note

Although every internal access to _spans is now protected by the lock, we still need a ConcurrentBagLite for this since external consumers can fetch a read-only reference to the collection, perform some operations on the transaction that would mutate the underlying collection, and then check/expect those changes to be reflected on the reference that they obtained earlier. This behaviour needs to be preserved.

[codecov]

Codecov Report

❌ Patch coverage is 96.72131% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.10%. Comparing base (f5d222e) to head (beaa188).
⚠️ Report is 1 commits behind head on maui-trace-ui-5116.

Files with missing lines	Patch %	Lines
src/Sentry/TransactionTracer.cs	96.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                  Coverage Diff                   @@
##           maui-trace-ui-5116    #5248      +/-   ##
======================================================
- Coverage               74.11%   74.10%   -0.02%     
======================================================
  Files                     509      502       -7     
  Lines                   18433    18268     -165     
  Branches                 3624     3580      -44     
======================================================
- Hits                    13662    13537     -125     
+ Misses                   3886     3860      -26     
+ Partials                  885      871      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sentry]

Bug: Setting TransactionTracer.EndTimestamp directly no longer works as expected because the setter doesn't update the _hasFinished flag, causing the getter to return null.
_{Severity: MEDIUM}

Suggested Fix

Update the EndTimestamp setter in TransactionTracer.cs to also set the _hasFinished flag to true via Volatile.Write(ref _hasFinished, true). This will restore the previous behavior where setting the end timestamp also marks the transaction as finished, ensuring the getter returns the correct value.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/Sentry/TransactionTracer.cs#L84-L95

Potential issue: The `EndTimestamp` property setter in `TransactionTracer` was changed
to only update the internal `_endTimestamp` field without setting the `_hasFinished`
flag to `true`. The corresponding getter is conditional on `_hasFinished` and returns
`null` if it's false. Code that relies on setting `EndTimestamp` directly to mark a
transaction as finished, such as in tests, will now find that `EndTimestamp` returns
`null`. This causes issues when creating a `SentryTransaction` from such a tracer, as
the resulting transaction will incorrectly have a `null` timestamp and an `IsFinished`
status of `false`.

Also affects:

• src/Sentry.Testing/SentryClientTests.cs:1380
• src/Sentry.Testing/SentryClientTests.cs:1500

_{Did we get this right? 👍 / 👎 to inform future reviews.}

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit beaa188. Configure here.}

[cursor]

_activeSpanTracker.Clear() races with PeekActive() after lock removal

Medium Severity

The inner lock was removed from LastActiveSpanTracker with the rationale that "all callers hold _lock," but ReleaseSpans() calls _activeSpanTracker.Clear() without holding _lock. ReleaseSpans is invoked from CompleteCapture, which runs outside the lock. A concurrent call to GetLastActiveSpan() (which does hold _lock) iterates the underlying Stack<ISpan> via foreach in PeekActive(). If Clear() modifies the stack during that iteration, this can throw InvalidOperationException or produce undefined behavior. The old inner lock on LastActiveSpanTracker protected against exactly this scenario.

Additional Locations (2)

• src/Sentry/TransactionTracer.cs#L383-L405
• src/Sentry/TransactionTracer.cs#L408-L415

 

^{Reviewed by Cursor Bugbot for commit beaa188. Configure here.}

[cursor]

Commented-out getter left in production code

Low Severity

The line // get => _endTimestamp; is a commented-out alternative implementation left over from development. It serves no purpose and could confuse future readers about the intended behavior of the EndTimestamp getter.

 

^{Reviewed by Cursor Bugbot for commit beaa188. Configure here.}