fix(asp-net): forward disposal in FlowSuppressingHostedService (#5651)

[JohnVerheij]

Description

FlowSuppressingHostedService implements IHostedLifecycleService but did not
implement IDisposable or IAsyncDisposable. When the DI container disposed the
host, the container only saw the non-disposable wrapper, so the inner service's
Dispose / DisposeAsync was never called, leaking unmanaged resources silently.

Add both IDisposable and IAsyncDisposable to the wrapper, forwarding to the
inner service. DisposeAsync prefers the async path on inner and falls back to
sync Dispose when inner is IDisposable only. Dispose forwards only when inner
implements IDisposable; async-only inner services are not disposed on the
synchronous path, matching the "never block on async" rule in CLAUDE.md.
Callers with async-only resources should use DisposeAsync (or await using on
the owning factory).

Follows the dual-interface forwarding precedent already in TracedWebApplicationFactory. That class can call _inner.Dispose()/DisposeAsync() directly because its inner type always implements both interfaces; this wrapper cannot make that assumption about IHostedService implementations, so it uses an is-check.

Related Issue

Fixes #5651

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Performance improvement
• Refactoring (no functional changes)

Checklist

Required

• I have read the Contributing Guidelines
• If this is a new feature, I started a discussion first and received agreement
• My code follows the project's code style (modern C# syntax, proper naming conventions)
• I have written tests that prove my fix is effective or my feature works

TUnit-Specific Requirements

• Dual-Mode Implementation: N/A. The fix is in TUnit.AspNetCore.Core, not TUnit.Core.SourceGenerator or TUnit.Engine. Disposal forwarding is not test-discovery/execution metadata.
• Snapshot Tests: N/A. No source generator output or public API change.
• Performance: change is disposal-time branching. Zero new allocations, no LINQ, no reflection. Not a hot path.
• AOT Compatibility: reflection-free. Verified under PublishAot=true.

Testing

• All existing tests pass
• I have added tests that cover my changes
• I have tested both source-generated and reflection modes (N/A — not a dual-mode concern)

Additional Notes

Regression tests: 7 pure-unit-tests in HostedServiceDisposalForwardingTests.cs covering:

Test	Scenario
DisposeAsync_Forwards_To_IAsyncDisposable_Inner	Async path on async-only inner
Dispose_Forwards_To_IDisposable_Inner	Sync path on sync-only inner
DisposeAsync_On_SyncOnly_Inner_Falls_Back_To_Dispose	Async wrapper method, sync inner
Dispose_On_AsyncOnly_Inner_Is_No_Op	Sync wrapper, async-only inner — documented no-op
DisposeAsync_On_DualInterface_Inner_Prefers_Async_Path	Preference ordering
Dispose_On_DualInterface_Inner_Prefers_Sync_Path	Preference ordering
Wrapper_Does_Not_Throw_When_Inner_Is_Not_Disposable	Non-disposable inner is safe

Tests construct the wrapper directly (no TestWebApplicationFactory, no host) to isolate the forwarding contract from TestWebApplicationFactory's disposal-ordering behaviour, which is outside this fix's scope. Verified stable over 10 consecutive runs across net8.0, net9.0, and net10.0 (70/70 assertion successes per run, 210/210 across all three TFMs).

The "never block on async" rule in CLAUDE.md is the reason the sync Dispose path does not attempt an async-to-sync bridge for async-only inners. Microsoft.Extensions.Hosting.Host.Dispose internally uses DisposeAsync().AsTask().GetAwaiter().GetResult(), but that pattern is deliberately avoided here per the repo's stated convention — the documented no-op is the tradeoff.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 9 complexity

Metric	Results
Complexity	9

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review

This is a clean, well-reasoned fix for a real resource-leak bug. The implementation is minimal, correct, and follows established patterns in the codebase. A few observations below.

---

What's Good

Disposal preference ordering is correct. DisposeAsync preferring IAsyncDisposable over IDisposable matches how Microsoft.Extensions.DependencyInjection's ServiceProvider behaves and what consumers expect.

The no-op on Dispose() for async-only inners is the right call. Blocking on DisposeAsync().AsTask().GetAwaiter().GetResult() inside Dispose() would violate the project's explicit "never block on async" rule and risks deadlocks. The tradeoff is clearly documented in the XML docs and PR description — exactly the right approach.

Consistent with TracedWebApplicationFactory. The PR description calls this out, and it checks out — TracedWebApplicationFactory uses the same dual-interface forwarding pattern (lines 75–78). This is good precedent to follow.

Test coverage is thorough. All seven disposal scenarios are covered, and the probe classes are clear and minimal. The tests construct the wrapper directly without a host or DI container, correctly isolating the forwarding contract.

InternalsVisibleTo is already in place. TUnit.AspNetCore.Core.csproj already includes TUnit.AspNetCore.Tests, so new FlowSuppressingHostedService(inner) in the tests compiles as expected.

---

Minor Observations

1. Wrapper_Does_Not_Throw_When_Inner_Is_Not_Disposable — assertion is indirect

await Assert.That(inner.StartedCount).IsEqualTo(0);

StartedCount is 0 because StartAsync was never called — not as a consequence of disposal being safe. The test name says "Does_Not_Throw" but the assertion doesn't directly prove that. The test body is already the implicit assertion (no exception means it passed), so the Assert.That line either isn't needed or could check something more intentional. Low severity — purely cosmetic — but worth noting for future readers.

2. No double-disposal guard

Neither Dispose() nor DisposeAsync() is idempotent. If a caller invokes both (e.g., Dispose() followed by DisposeAsync()), the inner service gets disposed twice. This is the same pattern as TracedWebApplicationFactory, which also has no guard, so it's consistent with the existing codebase convention. And in practice the DI container won't call both.

That said, since FlowSuppressingHostedService now explicitly implements both IDisposable and IAsyncDisposable, it's worth noting this is a mild deviation from Microsoft's guidance which recommends idempotent disposal when both interfaces are implemented. A simple _disposed flag would close this:

private int _disposed;

public async ValueTask DisposeAsync()
{
    if (Interlocked.Exchange(ref _disposed, 1) != 0) return;
    if (inner is IAsyncDisposable asyncDisposable)
        await asyncDisposable.DisposeAsync().ConfigureAwait(false);
    else if (inner is IDisposable disposable)
        disposable.Dispose();
}

public void Dispose()
{
    if (Interlocked.Exchange(ref _disposed, 1) != 0) return;
    if (inner is IDisposable disposable)
        disposable.Dispose();
}

Whether this is worth adding depends on how defensive you want to be. Given that TracedWebApplicationFactory omits it, omitting it here is defensible too — just flagging the inconsistency with Microsoft's pattern.

---

Summary

This is a solid fix. The core bug (disposal not forwarded through the wrapper) is correctly resolved, the async/sync fallback logic is right, the no-op decision on sync-path for async-only inners is well-justified and documented, and the tests cover all the relevant combinations. The two observations above are minor and neither blocks merging.

[JohnVerheij]

Thanks for the careful review.

Obs 1 (indirect assertion): addressed in the force-push to dc08af34a. Replaced the StartedCount == 0 check with explicit ThrowsNothing() assertions on the disposal calls and dropped the now-unused StartedCount tracking from the probe. Test name and test body now match semantically, and failure messages will point at the specific disposal call that threw.

Obs 2 (double-disposal guard): keeping the current shape to stay aligned with TracedWebApplicationFactory, which also omits the idempotency guard. The PR description cites that as precedent. If @thomhurst prefers the explicit Interlocked.Exchange guard, happy to add it here and push a matching update to TracedWebApplicationFactory in a follow-up. Otherwise staying consistent seems right.

[thomhurst]

Thanks @JohnVerheij ! Nice catch