fix(aspnetcore): guard hosted-service stop against concurrent double Host.StopAsync

[thomhurst]

Root cause (issue #6339, post-1.58.0 report)

The reporter's remaining ObjectDisposedException in the After(Test) hook is not the timeout-CTS bug fixed in #6340/#6349 — their new inner stack shows it thrown inside Rebus' bus-dispose chain during WebApplicationFactory.DisposeAsync.

Mechanism (confirmed with an instrumented probe, no Rebus needed):

1. Minimal-hosting SUTs park their own app.Run() inside WaitForShutdownAsync, which calls Host.StopAsync when ApplicationStopping fires.
2. After(Test) → WebApplicationFactory.DisposeAsync → DeferredHost.StopAsync → Host.StopAsync — which fires ApplicationStopping, waking the parked shutdown path while its own stop is still in flight.
3. Host.StopAsync has no concurrency guard, so every IHostedService.StopAsync runs twice, concurrently. Services with non-thread-safe shutdown break — Rebus' RebusBus.Dispose/InMemErrorTracker/TplAsyncTask guards are plain bools with check-then-act windows, producing exactly the reported ODE.

Probe stacks captured pre-fix: one stop from HostingAbstractionsHostExtensions.WaitForShutdownAsync, the other from WebApplicationFactory.DisposeAsync via the After hook — overlapping on the same host. 4–8 failures per 50 tests.

Fix

FlowSuppressingHostedService already wraps every hosted service in the SUT, so its stop-phase methods (StopAsync, StoppingAsync, StoppedAsync) now run the inner stop exactly once — duplicate and concurrent callers observe the same underlying task. Synchronous throws are cached as faulted tasks so a duplicate caller can't re-invoke.

This is fundamentally an aspnetcore Mvc.Testing design gap (nothing synchronizes DeferredHost disposal with the app's own RunAsync shutdown), but the wrapper is the right interception point to shield TUnit users today.

Tests

• HostedServiceStopOnceTests — deterministic unit tests: 16-way concurrent stop → inner sees 1; sequential re-stop no-ops; same-task identity; sync-throw cached; lifecycle hooks once-guarded.
• HostStopExactlyOnceProbeTests — end-to-end: 50 parallel factory lifecycles, probe hosted service throws with both stacks on any second StopAsync entry. Failed 4/51 and 2/51 pre-fix; clean across 6 consecutive runs post-fix.
• Full TUnit.AspNetCore.Tests suite: 92/92 (net10.0); all TFMs build.

Fixes #6339

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 51 complexity

Metric	Results
Complexity	51

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.}

[claude]

Review

What it does: Fixes issue #6339 by guarding FlowSuppressingHostedService's StopAsync/StoppingAsync/StoppedAsync so the wrapped hosted service's stop-phase methods run exactly once, even when Host.StopAsync is invoked concurrently twice (which happens when WebApplicationFactory.DisposeAsync races a minimal-hosting SUT's own WaitForShutdownAsync path). Previously unguarded stop calls could run the inner service's shutdown logic twice in parallel, breaking non-thread-safe shutdown code (e.g. Rebus) with ObjectDisposedException.

Correctness

• The lock (_stopGate) { return _field ??= InvokeOnce(...); } pattern is a standard "cache the started task" idiom (akin to AsyncLazy) — correct and race-free: the lock is only held for the synchronous portion of kicking off the call, and all callers (concurrent or sequential) subsequently await the same cached Task.
• InvokeOnce correctly captures a synchronous throw as a faulted Task so it gets cached too — without this, a sync-throwing inner StopAsync would re-execute on every duplicate call, defeating the once-guard. Verified by Synchronously_Throwing_Stop_Is_Cached_Not_Reinvoked.
• Sharing one _stopGate lock object across three independent cached-task fields (_stopTask, _stoppingTask, _stoppedTask) is fine — no cross-field interaction or reentrancy risk since InvokeOnce never calls back into the wrapper.
• The is not IHostedLifecycleService fast path in StoppingAsync/StoppedAsync correctly returns before touching the lock, matching prior behavior for non-lifecycle inner services.
• One implicit design tradeoff worth flagging in case it's not intentional: the cached task is never reset, so if a host were ever restarted after being stopped (start → stop → start → stop), the second StopAsync would return the already-completed first task without re-invoking the inner service. This matches the PR's stated scope (WebApplicationFactory disposal, not host restart), but might be worth a one-line remark in the class doc if restart-after-stop is a scenario TUnit's hosting wrapper needs to support elsewhere.

Test coverage

• HostedServiceStopOnceTests is a solid, fast unit-level suite: concurrent (16-way) and sequential double-stop, task-identity check, sync-throw caching, and lifecycle-hook guarding. Good use of a TaskCompletionSource gate to force real overlap rather than relying on timing.
• HostStopExactlyOnceProbeTests is a good end-to-end regression test — it reproduces the real race via WebApplicationFactory + minimal hosting rather than only unit-testing the wrapper in isolation, and the probe throws with both stack traces on a double-entry, making a future regression easy to diagnose from CI output alone. [Repeat(50)] plus a random jitter is a reasonable way to reliably surface a race that was previously only observed in 4-8/50 runs.
• Both new test files follow existing conventions in TUnit.AspNetCore.Tests (assertion style, WebApplicationTest<TestWebAppFactory, Program> base usage, InternalsVisibleTo access to the internal wrapper class).

Minor / non-blocking

• StopProbeHostedService.StopAsync builds the exception message (with Environment.StackTrace) even in the non-throwing path is avoided (good — it's only built on entry > 1), so no perf concern there.
• No architectural concerns — this is a narrowly-scoped, well-isolated fix at the correct interception point given the upstream Mvc.Testing gap described in the PR body, and doesn't change behavior for the (presumably far more common) non-racing shutdown path.

Overall: the fix is correct, minimal, and well-tested. No changes requested.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a37fe9c3c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve each StopAsync caller's cancellation token

When a second Host.StopAsync arrives while the first stop is still running, this returns the cached task that was created with the first caller's token, so the duplicate caller's token is never observed. In contexts that pass a shorter or already-canceling shutdown token, that caller can now wait for the original stop task and run past its own timeout instead of canceling; share the single inner invocation, but have duplicate waits respect their own cancellationToken.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1ac3c735c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Avoid invoking StopAsync while holding the gate

When the first hosted service performs slow synchronous cleanup before returning its Task, a concurrent Host.StopAsync with a shorter or already-canceled token will block on _stopGate until this call returns, so it never reaches WaitWithCancellation and can run past its own shutdown timeout. This is fresh evidence beyond the earlier cached-task review comment: the inner stop is now still invoked while the monitor is held, before duplicate callers can obtain a cancellable wait on the shared task. Publish a placeholder under the lock and invoke the inner stop outside the monitor so duplicate waits can respect their own token immediately.

Useful? React with 👍 / 👎.

[claude]

Review update (re-review after new commit)

My previous review covered the first commit (6a37fe9) and found it correct with no changes requested. Since then, commit f1ac3c7 ("preserve stop cancellation") was pushed — reviewing that delta:

What it changes

StopAsync/StoppingAsync/StoppedAsync now wrap the shared cached task with task.WaitAsync(cancellationToken) before returning to the caller, instead of returning the shared task directly.

Why it's correct

• Previously, only the first caller's CancellationToken was ever used (passed into InvokeOnce) — duplicate callers awaited the same shared task and had no way to bail out via their own token if the shared operation ran long. WaitWithCancellation fixes this: each caller now observes cancellation on their own token without cancelling the underlying shared stop operation for other callers (correct, since the real inner StopAsync is intentionally invoked once and shared).
• cancellationToken.CanBeCanceled ? task.WaitAsync(cancellationToken) : task avoids allocating a wrapper task for the extremely common CancellationToken.None case.
• Task.WaitAsync(CancellationToken) internally still attaches a continuation to the original task, so the original task's exception/completion is observed even if a caller's wait is cancelled early — no unobserved-task-exception risk.
• New test Duplicate_Stop_Caller_Observes_Its_Own_Cancellation_Token (in HostedServiceStopOnceTests.cs) directly exercises this: a blocked first caller keeps the shared stop pending, a second caller cancels its own token and correctly observes OperationCanceledException while inner.StopCalls stays at 1 (proving the inner op wasn't re-invoked or aborted for the first caller).

CLAUDE.md compliance

No violations — no blocking on async (.Result/.GetAwaiter().GetResult()), no VSTest usage, no source-gen output changes requiring snapshot updates, and no reflection requiring AOT annotations.

No issues found in this delta. Fix remains correct, minimal, and well-tested.

[claude]

Review update (re-review after latest commit)

Prior reviews covered up through f1ac3c7 (preserve stop cancellation). This pass reviews the delta from the newest commit, dfd6df6 ("start stop outside lock"), which addresses the still-open Codex P2 comment about invoking StopAsync while holding _stopGate.

What it changes

InvokeOnce no longer calls the wrapped operation synchronously inside lock (_stopGate). It now only publishes a TaskCompletionSource placeholder under the lock, then kicks off ExecuteAndCompleteAsync (which awaits the real operation and completes the TCS) after releasing the lock. StopAsync/StoppingAsync/StoppedAsync were simplified to call InvokeOnce(ref field, ...) directly instead of duplicating the lock/WaitWithCancellation boilerplate three times.

Why it's correct

• The lock's only job now is the fast, non-blocking check-and-publish of the cached Task — exactly the invariant needed to guarantee the inner operation still runs once. A duplicate caller no longer has to wait for a slow synchronous inner StopAsync (e.g. one that blocks on a ManualResetEventSlim) to return before it can even reach WaitWithCancellation and honor its own (possibly already-canceled) token. This was precisely the race Codex flagged.
• Exactly-once semantics are preserved: completionSource is only non-null for the single caller that wins the cachedTask is null check inside the lock, so ExecuteAndCompleteAsync/operation still can't run twice.
• Synchronous throws from operation are still captured correctly — since ExecuteAndCompleteAsync has no await before invoking operation, a sync throw happens inline within the method's try/catch and completes the TCS synchronously, preserving the previous "cache the sync throw" behavior (still covered by Synchronously_Throwing_Stop_Is_Cached_Not_Reinvoked).
• catch (OperationCanceledException) → SetCanceled(exception.CancellationToken) correctly re-surfaces the original cancellation token on the shared task rather than collapsing to a generic canceled state.
• Duplicate_Stop_Caller_Does_Not_Wait_For_Synchronous_Startup (already in the test file) directly exercises the fixed scenario: a first caller blocked inside a synchronous StopAsync, and a second caller with an already-canceled token that must return OperationCanceledException promptly instead of blocking on the gate.

Verification

Ran the new tests on this branch directly (HostedServiceStopOnceTests + HostStopExactlyOnceProbeTests, net10.0): 7/7 passed, including the 50-iteration end-to-end probe.

CLAUDE.md compliance

No violations — no blocking on async, no VSTest usage, no source-gen/snapshot changes, no reflection requiring AOT annotations.

No issues found. This delta correctly resolves the remaining Codex P2 concern without reintroducing any of the earlier ones. Fix is correct, minimal, and well-tested.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dfd6df6b7f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Avoid canceling the owning stop wait

When this is the first caller, task is the placeholder for the actual inner stop, but returning it through WaitWithCancellation means Host.StopAsync's shutdown-timeout token can cancel the wrapper while ExecuteAndCompleteAsync continues running the inner StopAsync in the background. For services that ignore the token or take longer than the timeout, the factory/host can proceed to dispose the service provider concurrently with stop cleanup, whereas hosted-service shutdown normally still awaits the returned tasks after cancellation is requested (docs). Apply the per-caller WaitAsync only to duplicate callers, and let the owning invocation return the cached stop task.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[claude]

Review: fix/6339-deferred-host-double-stop (#6397)

Overview

Fixes #6339: Host.StopAsync has no concurrency guard, so a minimal-hosting SUT's own WaitForShutdownAsync shutdown path and WebApplicationFactory.DisposeAsync's After(Test) teardown can both invoke Host.StopAsync concurrently, causing every wrapped IHostedService.StopAsync/StoppingAsync/StoppedAsync to run twice in parallel. FlowSuppressingHostedService now funnels all three stop-phase methods through a shared InvokeOnce helper backed by a TaskCompletionSource, so the inner service's stop logic runs exactly once no matter how many callers (concurrent or sequential) invoke it.

Correctness — delta since last automated review (ccaf8419, "preserve owning stop wait")

This commit hadn't been covered by the prior Codex passes yet, so I traced it independently:

• Before: every caller's returned task was WaitWithCancellation(task, cancellationToken), including the "owning" caller whose token was the one actually passed into the real operation(...) invocation.
• After: only non-owning (duplicate) callers get WaitWithCancellation; the owning caller gets the raw shared task back.
• This matters because for the owning caller, wrapping with task.WaitAsync(cancellationToken) was redundant at best and misleading at worst: if the inner service doesn't observe cancellation promptly, the wrapper could report StopAsync as canceled/complete to its caller while the real inner stop was still running unobserved in the background — a footgun for any code that treats a returned StopAsync as "the service has actually stopped." The new behavior restores parity with plain inner.StopAsync(cancellationToken) semantics for the single-caller (non-racing) case, while still giving duplicate callers a way to bail out on their own token without blocking on (or cancelling) the shared operation.
• Verified ownsInvocation is set correctly (true only for the caller that wins the cachedTask is null check under _stopGate), and that the new Owning_Stop_Caller_Awaits_Inner_After_Token_Cancellation test exercises exactly this: a token cancellation on the owning caller does not prematurely complete the returned task while the inner service is still blocked.
• No regression to the previously-fixed duplicate-caller path (Duplicate_Stop_Caller_Observes_Its_Own_Cancellation_Token still passes conceptually — that test's owning caller uses CancellationToken.None, so WaitWithCancellation was already a no-op there).

Other observations (non-blocking)

• _stopGate is shared across _stopTask, _stoppingTask, and _stoppedTask, so publishing a placeholder for one serializes briefly against the others. The critical section is a couple of field reads/writes, not the actual stop work, so this is not a real contention risk — just noting it in case a future change makes the critical section heavier.
• ExecuteAndCompleteAsync is fire-and-forget (_ = ExecuteAndCompleteAsync(...)), but all exception paths are funneled into the TaskCompletionSource inside the same method's try/catch, so there's no unobserved-task-exception risk.
• Good use of TaskCreationOptions.RunContinuationsAsynchronously on the TaskCompletionSource to avoid duplicate/owning callers running continuations synchronously on whichever thread completes the inner operation.

Test coverage

Strong: HostedServiceStopOnceTests isolates the wrapper's concurrency/cancellation semantics with deterministic unit tests (concurrent stop, sequential re-stop, task identity, sync-throw caching, lifecycle-hook guarding, and now the owning-caller-cancellation case), and HostStopExactlyOnceProbeTests adds an end-to-end regression probe against the actual double-Host.StopAsync race with a [Repeat(50)] end-to-end test. This is a good pairing of a fast/deterministic suite with a slower probabilistic regression probe.

CLAUDE.md compliance

No violations found — no blocking on async, no VSTest usage, no source-gen/snapshot changes, no reflection requiring AOT annotations. Dual-mode rule doesn't apply (this is ASP.NET Core hosting infrastructure, not core metadata collection).

Verdict

No correctness issues found, including in the most recent unreviewed commit. The fix is well-scoped to the actual race, and the owning/duplicate caller distinction added in the latest commit is a meaningful correctness improvement over the prior revision, not just a style change. Nice work isolating the root cause down to the Host.StopAsync concurrency gap with an instrumented probe before writing the fix.