fix: auto-suppress ExecutionContext flow for hosted services (#5589)#5598
fix: auto-suppress ExecutionContext flow for hosted services (#5589)#5598
Conversation
`TestWebApplicationFactory<T>` now wraps every registered `IHostedService` so its `StartAsync` runs under `ExecutionContext.SuppressFlow`. Background tasks spawned inside `StartAsync` capture a clean execution context, preventing spans from hosted-service work in test B from being attributed to test A's `TraceId`. The wrapper also implements `IHostedLifecycleService` so the Host's `StartingAsync`/`StartedAsync`/`StoppingAsync`/`StoppedAsync` hooks keep firing for inner services that implement it (the Host uses an `is` check against the registered instance). Override `SuppressHostedServiceExecutionContextFlow` and return `false` to preserve ambient context flow.
There was a problem hiding this comment.
Code Review
Overall this is a well-targeted fix with clean implementation and solid test coverage. The IHostedLifecycleService passthrough is a thoughtful detail that prevents silent hook drops. A few points worth considering:
Significant: Suppressed() only covers the synchronous portion of StartAsync
private static Task Suppressed(Func<CancellationToken, Task> op, CancellationToken ct)
{
using var _ = ExecutionContext.SuppressFlow();
return op(ct); // ← using block ends here, before the returned Task completes
}The using disposes before the returned Task completes. This means suppression only applies while op(ct) runs synchronously (up to its first await). The fix works for the common Task.Run(...) pattern, but fails for services that await before spawning:
public async Task StartAsync(CancellationToken ct)
{
await _dbContext.EnsureCreatedAsync(ct); // returns after this await
_backgroundTask = Task.Run(ProcessQueue); // spawned here — NOT under suppression!
}This is a real-world pattern (initialize something, then start a background loop). The current behavior is that suppression silently does nothing in this case, which could be confusing since users may assume the fix applies uniformly.
Suggestion: Document this as a known limitation in the XML doc on SuppressHostedServiceExecutionContextFlow. Something like:
Note: suppression applies to background work spawned synchronously inside
StartAsync(e.g.Task.Runbefore anyawait). Work spawned after anawaitpoint is not covered; manually wrap such spawns withExecutionContext.SuppressFlow().
Minor: Asymmetric suppression across lifecycle methods
StartingAsync and StartedAsync use Suppressed(), but StoppingAsync and StoppedAsync do not. This is probably intentional — stop methods rarely spawn long-running background tasks that need test isolation — but it is implicit. Worth a one-line comment in the class to signal the intent was deliberate.
Minor: Tests check internal implementation type rather than behavior
await Assert.That(hostedServices.Any(h => h is FlowSuppressingHostedService)).IsTrue();This couples the test to the class name of the wrapper. If the wrapping strategy changes (e.g. a different decorator), the behavioral tests will pass but the structural test will fail for no real reason. Since InternalsVisibleTo makes this compile, it's not a blocker — but a behavioral assertion (e.g. verifying that Activity.Current is null in a spawned task) would be more resilient. The StartAsync_SuppressesFlow_IntoSpawnedTasks test already does this correctly.
Positive notes
- The
IHostedLifecycleServicepassthrough is exactly right — omitting it would silently break services with those hooks and be very hard to diagnose. WrapHostedServiceDescriptorcorrectly handles all three descriptor shapes (instance / factory / type).- The opt-out via
protected virtual boolis the right API shape — non-breaking, discoverable, and the safe default is the right choice. - Test coverage is comprehensive including the isolated-factory path.
- Docs updates are clear and accurate.
The async-limitation point is the only one that could genuinely surprise users, and a doc note would be sufficient. The rest are polish.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 20 |
TIP This summary will be updated as you push new changes. Give us feedback
`using var _ = SuppressFlow(); return inner.StartAsync(ct);` only suppresses context capture during the synchronous portion of StartAsync — `Task.Run` after a prior `await` re-captures the test's Activity.Current. Combine SuppressFlow with `Task.Run(() => inner.StartAsync(ct), ct)` so the inner hosted service runs on a thread-pool worker whose ExecutionContext was captured under suppression. Activity.Current starts null and stays null through every await point, so background tasks spawned anywhere inside StartAsync inherit a clean context. Also drops the Limitation xmldoc since it no longer applies, and adds a deep-async test (StartAsync_SuppressesFlow_WhenSpawnIsAfterAwait) proving the fix holds past the first await.
|
Addressed in 81309cb: #1 (sync-only suppression): Pushed further than the doc-note suggestion. Swapped the wrapper to combine #2 (asymmetric suppression): Added a comment on the stop-lifecycle methods explaining the intent — stop methods typically signal cancellation and await shutdown rather than spawning long-running background work, so Stop isn't the span-leak vector that Start is. #3 (structural tests): Kept. The behavioral test ( |
There was a problem hiding this comment.
Follow-up Review
Previous review issues have all been addressed in commit 81309cb. Here's the updated assessment:
Previous Issues — Status
#1 (sync-only suppression) — Fully resolved
The fix went beyond the doc-note suggestion: RunOnCleanContext now uses Task.Run under SuppressFlow():
private static Task RunOnCleanContext(Func<CancellationToken, Task> op, CancellationToken ct)
{
using var _ = ExecutionContext.SuppressFlow();
return Task.Run(() => op(ct), ct);
}This is the correct approach. Task.Run captures the execution context at scheduling time (when flow is suppressed), so the lambda and all its async continuations run with a clean context — covering the await ...; Task.Run(...) pattern that the previous version missed. The new StartAsync_SuppressesFlow_WhenSpawnIsAfterAwait test validates this.
#2 (asymmetric suppression) — Resolved
The comment on StoppingAsync/StoppedAsync is clear and states the intent explicitly.
#3 (structural tests) — Kept intentionally, reasoning is sound
The author's explanation is valid: the structural assertions verify wrapping actually happened in the isolated-factory path where there's no outer Activity to assert against behaviorally. Both test styles serve distinct purposes.
One New Minor Observation
FlowSuppressTestFactory and DeepAsyncFlowSuppressTestFactory have nearly identical ConfigureWebHost overrides — the only difference is the probe type. This is test-code duplication that has no real consequence, but if a third probe pattern is added later, it would be worth consolidating. Not a blocker.
Overall
The implementation is solid. The Task.Run-under-SuppressFlow approach is idiomatic and correctly handles all StartAsync shapes. The thread-pool hop for hosted service startup is the right trade-off in a test host: negligible cost, and hosted services with thread-affinity requirements are essentially nonexistent in practice. IHostedLifecycleService passthrough, the three descriptor-shape handling in WrapHostedServiceDescriptor, the opt-out mechanism, and the docs updates are all well-done. Ready to merge.
Summary
TestWebApplicationFactory<T>now decorates every registeredIHostedServiceso itsStartAsyncruns underExecutionContext.SuppressFlow(). Background tasks spawned insideStartAsynccapture a clean execution context, preventing spans from hosted-service work in test B being attributed to test A'sTraceId.FlowSuppressingHostedService) also implementsIHostedLifecycleServicesoStartingAsync/StartedAsync/StoppingAsync/StoppedAsynchooks keep firing for inner services that implement it — the Host uses anischeck against the registered instance, and a plainIHostedServicewrapper would silently drop those hooks.protected virtual bool SuppressHostedServiceExecutionContextFlow => true(override to returnfalse).How
Decoration runs in an override of
CreateHost(IHostBuilder), which fires after allConfigureWebHost/WithWebHostBuilder/ConfigureTestServicescallbacks — so it catches hosted services registered via the isolated-factory path as well as the base factory.Resolves #5589.
Test plan
StartAsync_SuppressesFlow_IntoSpawnedTasks— spawnedTask.RuninsideStartAsyncseesActivity.Current == nulleven when an outer Activity is active at factory-build time.OptOut_PreservesFlow_IntoSpawnedTasks— when override returnsfalse, spawned task inherits the outer Activity.RegisteredHostedServices_AreWrapped— resolvedIHostedServiceinstances are of the wrapper type.IsolatedFactory_HostedServices_AreWrapped— services added viaGetIsolatedFactory/ConfigureTestServicesare also wrapped.TUnit.AspNetCore.Testssuite (20 tests) green on net10.0.Docs
docs/docs/examples/opentelemetry.md— "Spans from test A are showing up under test B" section updated to note the auto-fix.docs/docs/guides/distributed-tracing.md— StaticActivitySourcelimitation updated.