fix: start session activity before discovery so discovery spans parent correctly

[thomhurst]

Summary

• Fixes orphaned "test discovery" OpenTelemetry span reported in [Bug]: OpenTelemetry - Missing root span #5244
• Extracts session activity creation into an idempotent TryStartSessionActivity() method on HookExecutor
• Calls it after Before(TestDiscovery) hooks (for execution requests) so users who set up their TracerProvider in Before(TestDiscovery) get a single unified trace with discovery properly nested under the session

Root cause

The "test discovery" span tried to parent under the session activity, but the session activity didn't exist yet — it was only created after Before(TestSession) hooks, which run after discovery. So sessionActivity?.Context ?? default always evaluated to default, producing an orphaned root span.

How it works

TryStartSessionActivity() is called from two sites:

1. After Before(TestDiscovery) hooks — catches users who set up their TracerProvider early
2. After Before(TestSession) hooks — catches users who set up their TracerProvider in the traditional location

The method is idempotent: it checks sessionContext.Activity is not null and HasListeners() before creating anything, so whichever call site runs first with an active listener wins.

Files changed

File	Change
TUnit.Engine/Services/HookExecutor.cs	Extract TryStartSessionActivity(), replace inline creation
TUnit.Engine/TestExecutor.cs	Thin pass-through to HookExecutor
TUnit.Engine/TestDiscoveryService.cs	Call TryStartSessionActivity() after discovery hooks
TUnit.UnitTests/SessionActivityLifecycleTests.cs	10 new tests for activity lifecycle

Test plan

• All 177 unit tests pass (TUnit.UnitTests)
• All 13 ASP.NET Core OTel correlation tests pass (TUnit.AspNetCore.Tests)
• Builds on both net8.0 and net10.0
• Manual verification with Jaeger/Grafana LGTM: user sets up TracerProvider in Before(TestDiscovery) and sees a single unified trace

Closes #5244

[claude]

Code Review

Overall this is a well-targeted, clean fix for the orphaned discovery span issue. The root cause analysis in the PR description is accurate, the idempotency approach is solid, and the test suite is thorough. A few observations:

---

Architecture / Design

The two-phase idempotency pattern is the right call here.
Calling TryStartSessionActivity() from two sequential sites (after Before(TestDiscovery) and after Before(TestSession)) with an early-return guard is a simple, low-risk design. It doesn't require the caller to know which phase the listener was registered in—correct.

Thread-safety: low risk in practice, but worth noting.
TryStartSessionActivity has a classic TOCTOU shape:

if (sessionContext.Activity is not null) return;   // check
// ... gap ...
sessionContext.Activity = TUnitActivitySource.StartActivity(...); // act

In practice, both call sites are sequential within the TUnit session lifecycle so a real race is unlikely today. But if the method is ever called from parallel hook execution or a concurrent path in the future, two threads could both pass the null check and create duplicate activities (with the loser's activity leaking). A one-liner lock or Interlocked.CompareExchange would make the intent explicit and guard against future callers:

// Safer — makes no-double-create intent explicit
lock (_activityLock)
{
    if (sessionContext.Activity is not null) return;
    if (TUnitActivitySource.Source.HasListeners())
        sessionContext.Activity = TUnitActivitySource.StartActivity(...);
}

Not blocking the PR on this, but worth considering given how easy it is to protect.

isForExecution guard in TestDiscoveryService.
Correct — skipping activity creation during pure discovery (IDE listing, --list-tests) avoids creating a dangling session span that would never be finished. Good defensive coding.

---

Test File: SessionActivityLifecycleTests.cs

Good: The ActivityListenerScope RAII helper is clean, and the stubs are minimal (only implement what TryStartSessionActivity actually touches). The hierarchy tests in FullSpanHierarchy_SessionParentsAllChildren and DiscoveryAndAssembly_ShareSameTrace directly validate the fix for #5244.

Missing negative test — the silent failure mode.
There's no test for the case where HasListeners() is false when TryStartSessionActivity is called. The important failure mode here is: user sets up their TracerProvider after discovery hooks, TryStartSessionActivity runs, HasListeners() is false, activity stays null, and the discovery span silently orphans (same bug as before, different trigger). A test that calls TryStartSessionActivity without an ActivityListenerScope and asserts sessionContext.Activity is null would make this contract explicit. However, the comment in the test file warns that TUnit's HTML reporter may keep HasListeners() == true at all times during test execution, which would make that test unreliable. If that's the case, the comment should be promoted to a more prominent warning.

null! in test setup is a code smell.

var executor = new HookExecutor(hookDelegateBuilder, contextProvider, null!);

This works today because TryStartSessionActivity doesn't touch the third argument, but it's a maintenance trap — anyone who later adds a call to the orchestrator inside that method gets a NullReferenceException at runtime in a test rather than a compile-time signal. Consider extracting a StubEventReceiverOrchestrator (even if it only throw new NotSupportedException() like the other stubs), or adding an XML doc note on the constructor explaining which parameters are safe to null in tests.

Comment on HasListeners assumption.

// TUnit's own HTML reporter listener is active during test execution, so HasListeners() is always true here.

This assumption should be surfaced more prominently — ideally as a class-level doc comment — because it means these tests can never reliably test the "no listeners" path. It's not wrong, just worth calling out so future maintainers don't add a test for that path and wonder why it doesn't work.

---

Minor

• The #if NET guards are consistent with the rest of the file — good.
• The /// \<inheritdoc cref="HookExecutor.TryStartSessionActivity"/\> on the TestExecutor pass-through is a nice touch for discoverability.
• The XML doc on TryStartSessionActivity accurately describes the contract.

---

Summary: Fix is correct and well-tested. The main actionable items are (in priority order):

1. Add a StubEventReceiverOrchestrator or doc comment to remove the null! fragility.
2. Consider a lock or note for thread safety (low urgency, high future-proofing value).
3. Clarify the HasListeners test assumption at the class level.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 20 complexity

Metric	Results
Complexity	20

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}