Always complete the message bus shutdown handshake

[Evangelink]

Fixes #10357.

The bug

CommonHost.ExecuteRequestAsync wraps the session body in catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) and swallows it, so NotifyTestSessionEndAsync is skipped on the cancellation path. That method is the only caller of baseMessageBus.DrainDataAsync() + DisableAsync(), and DisableAsync is the only path that awaits the consumer loops. CommonHost.RunAsync's finally then calls DisposeServiceProviderAsync(..., isProcessShutdown: true), which disposes the bus and every IDataConsumer.

So on cancellation nothing waited for the in-flight consumer loop and disposal raced it, and everything still queued was dropped. This affects every IDataConsumer — TRX, coverage, the report extensions, terminal output.

The fix

Close the handshake for every outcome of the session.

• ExecuteRequestAsync disables the bus in a finally, and DisposeServiceProviderAsync disables a BaseMessageBus before disposing it and its consumers — a backstop that also covers hosts and failure paths that never reach the request. Both go through a best-effort helper that logs and swallows, so it never replaces an exception that is already propagating.
• TestHostControllersTestHost.DisposeServicesAsync disables the bus before its own manual dispose loops. A CompositeExtensionFactory builds one object per registration, so a lifetime handler or env-var provider disposed there can be the very same instance the bus holds as an IDataConsumer — and the in-run disable is both gated on LifetimeHandlers.Length > 0 and unreachable on every early-out path.

Stop dropping queued payloads. The consumer pump no longer reads through the run's cancellation token, so completing the channel drains what was already queued instead of abandoning it. Consumers still receive the run token, so a cooperative consumer can bail out of its own work immediately.

Bound the shutdown, but only when aborting. The new finally means the cancellation path awaits consumer loops for the first time, so an unbounded wait would hang the cancellations that have no second escape hatch (--timeout, --maximum-failed-tests) — unlike an interactive Ctrl+C, which can be pressed again to force the exit. The wait is therefore bounded once the run is cancelled (ShutdownTimeouts, overridable via TESTINGPLATFORM_MESSAGEBUS_CANCELED_SHUTDOWN_TIMEOUT_SECONDS), including when the cancellation arrives while an unbounded wait is already in flight. The normal path stays unbounded so a slow final flush (a large TRX or coverage report) is never cut off.

Never dispose a consumer that outlived the budget. Doing so would recreate the exact race. Such a consumer is reported through BaseMessageBus.ConsumersStillRunning and deliberately leaked to the process exit. That verdict is latched at disposal and recorded in the caller's alreadyDisposed list, so it survives the consumer also being registered as a plain service and the provider being walked more than once.

Assorted hardening on the bus. DisableAsync is idempotent, every caller awaits the same completion rather than just observing that a disable started, and it completes every processor even if one faults. A failing InitAsync tears down the pumps it had already started. PublishAsync checks cancellation before the disabled guard so teardown-time publishes during an abort are a silent no-op instead of masking the cancellation. MessageBusProxy tolerates a bus that was never built.

Also replaces the stale TODO that pointed at #8086 (an unrelated analyzer issue).

Testing

16 new tests in AsynchronousMessageBusTests and CommonHostTests. Every behavioural fix is mutation-verified: reverting each one individually makes exactly the corresponding test fail, and restoring it makes it pass.

• build.cmd -test green on all TFMs (net462/net48/net8.0/net9.0), 0 warnings.
• Full MTP acceptance suite after -pack: 779 tests, 763 passed. The 3 failures are pre-existing and unrelated — Trx_WhenReportTrxAndResultsDirectoryAreSpecifiedWithArtifact... (net462) reproduces on a clean stash of the branch base, and the two NativeAotTests fail on a local vswhere.exe is not recognized linker error.

Not in scope

A cooperative consumer that throws OperationCanceledException on the first item after the abort still opts out of the remaining drain — that is deliberate, since forcing the rest of the backlog through a consumer that asked to stop is what would hang the abort.

[Copilot]

🟡 Not ready to approve

Shutdown still has disposal and publication races, and the shared constant lacks required API tracking.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Completes message-bus shutdown before disposing consumers, including cancellation and failure paths.

Changes:

• Makes bus shutdown idempotent and bounded during cancellation.
• Tracks consumers that remain active to avoid unsafe disposal.
• Adds shutdown and cancellation tests plus internal API declarations.

File summaries

File	Description
AsynchronousMessageBusTests.cs	Tests shutdown, draining, and timeout behavior.
CommonHostTests.cs	Tests host teardown and consumer disposal.
ShutdownTimeouts.cs	Defines canceled-shutdown timeout handling.
MessageBusProxy.cs	Supports safe pre-build shutdown.
IAsyncConsumerDataProcessor.cs	Exposes consumer-running state.
BlockingConsumerDataProcessor.cs	Adds bounded canceled shutdown.
BaseMessageBus.cs	Exposes remaining active consumers.
AsynchronousMessageBus.cs	Implements coordinated, idempotent disablement.
AsyncConsumerDataProcessor.netstandard.cs	Drains queued payloads on .NET Standard.
AsyncConsumerDataProcessor.net.cs	Drains queued payloads on modern .NET.
InternalAPI/net/InternalAPI.Unshipped.txt	Tracks framework-specific internal API.
InternalAPI/InternalAPI.Unshipped.txt	Tracks shared internal API additions.
TestHostControllersTestHost.cs	Disables the bus before controller disposal.
CommonTestHost.cs	Adds shutdown backstops and disposal protection.
EnvironmentVariableConstants.cs	Adds timeout configuration key.

Review details

• Files reviewed: 15/15 changed files
• Comments generated: 12
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[github-actions]

🤖 Automated content by GitHub Copilot. Generated by the Grade Tests on PR (on open / sync) workflow. · sonnet46 170.3 AIC · ⌖ 4.11 AIC · ⊞ 11.8K · ◷

[Copilot]

🟡 Not ready to approve

Internal API tracking is incomplete, and sequential per-consumer waits can multiply the configured shutdown timeout.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (4)

src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs:19

• This shared source adds a tracked internal API, but no matching API declarations were added. EnvironmentVariableConstants.cs is compiled into the platform plus Retry, TrxReport, HotReload, MSBuild, HangDump, CtrfReport, and Platform.MSBuild; the existing TESTINGPLATFORM_MESSAGEBUS_DRAINDATA_ATTEMPTS entries show that these constants are tracked. PublicApiAnalyzers will therefore report RS0016 in every tracking consumer (including Microsoft.Testing.Platform itself) until the corresponding InternalAPI.Unshipped.txt files are updated.

    public const string TESTINGPLATFORM_MESSAGEBUS_CANCELED_SHUTDOWN_TIMEOUT_SECONDS = nameof(TESTINGPLATFORM_MESSAGEBUS_CANCELED_SHUTDOWN_TIMEOUT_SECONDS);

src/Platform/Microsoft.Testing.Platform/Messages/AsynchronousMessageBus.cs:302

• The canceled-shutdown timeout is applied once per processor, but this loop awaits processors sequentially. With N consumers that ignore cancellation, teardown can therefore take N × the configured timeout (five consumers make the 30-second “budget” 2.5 minutes), so the new setting does not actually bound message-bus shutdown as a whole. Start/completion-signal all processors before waiting and use a shared deadline (or await their bounded completions concurrently).

                    await processor.CompleteAddingAsync().ConfigureAwait(false);

src/Platform/Microsoft.Testing.Platform/Messages/AsynchronousMessageBus.cs:100

• The new partial-initialization cleanup is not exercised by the expanded AsynchronousMessageBusTests: every test shown completes InitAsync, and no test makes a later consumer fail after an earlier processor pump has started. Please cover that failure path and verify the already-created pump is completed, since removing this catch would otherwise reintroduce the resource leak without any test failure.

        catch
        {
            // The pumps of the processors we built before the failure are already running, and a bus whose
            // InitAsync threw never reaches SetBuiltMessageBus, so nothing would ever complete their channels
            // and they would park forever, rooting their consumers and everything queued for them.
            foreach (IAsyncConsumerDataProcessor processor in _consumerProcessor.Values)
            {
                processor.Dispose();

src/Platform/Microsoft.Testing.Platform/Messages/BlockingConsumerDataProcessor.cs:86

• This comment points to ShutdownTimeouts.CanceledConsumerCompletion, but no such member exists; the timeout is provided by GetCanceledConsumerCompletion (with DefaultCanceledConsumerCompletion as its fallback). Update the reference so future readers can find the governing code.

        // See ShutdownTimeouts.CanceledConsumerCompletion: on an aborted run the wait is bounded so a consumer

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[github-actions]

🤖 Automated content by GitHub Copilot. Generated by the Grade Tests on PR (on open / sync) workflow. · sonnet46 130.6 AIC · ⌖ 4.36 AIC · ⊞ 11.8K · ◷

[Copilot]

🟡 Not ready to approve

Shutdown ordering and cancellation races can still produce incomplete output, hidden failures, or concurrent consumer disposal.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (5)

src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs:314

• On the cancellation path, this renders the final output before the finally disables the bus. Any test updates already queued are therefore consumed only after DisplayAfterSessionEndRunAsync has built the terminal summary, so the cancellation summary can still omit partial results even though the new pump eventually drains them. Close the bus handshake before rendering the final output.

            // We keep the display after session out of the OperationCanceledException catch because we want to notify the IPlatformOutputDevice
            // also in case of cancellation. Most likely it needs to notify users that the session was canceled.
            await DisplayAfterSessionEndRunAsync(outputDevice, testSessionInfo).ConfigureAwait(false);

src/Platform/Microsoft.Testing.Platform/Messages/BlockingConsumerDataProcessor.cs:86

• This comment points to ShutdownTimeouts.CanceledConsumerCompletion, which does not exist; the timeout is obtained through ShutdownTimeouts.GetCanceledConsumerCompletion. Referencing the actual member keeps the shutdown rationale traceable.

        // See ShutdownTimeouts.CanceledConsumerCompletion: on an aborted run the wait is bounded so a consumer

src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs:555

• In per-request server cleanup, the filter excludes services cloned from the global provider before execution reaches this new running-consumer check. If such a shared consumer (for example, a factory-returned singleton registered in both roles) outlives the budget, the per-request bus is disposed and forgotten without persisting that verdict; a later global shutdown can then dispose the same instance while its old consume task is still running. Running consumers must be recorded across the owning/global provider even when this disposal pass does not own them.

                    if (Array.IndexOf(consumersStillRunning, dataConsumer) >= 0)
                    {
                        // Its ConsumeAsync outlived the shutdown budget of an aborted run, so it is ignoring the
                        // cancellation token. Disposing it now is precisely the race the handshake exists to
                        // prevent, so we leak it and let the process exit reclaim it instead. Recording it as
                        // already disposed is what makes that decision stick: the same consumer is usually also
                        // registered as a plain service, and a host can walk the provider more than once.
                        if (!alreadyDisposed.Contains(dataConsumer))

src/Platform/Microsoft.Testing.Platform/Messages/AsynchronousMessageBus.cs:101

• The new failed-initialization cleanup path is not covered by the added bus tests, despite nearby tests exercising the other shutdown branches. Add a test where one consumer's pump is created and a later consumer makes InitAsync fail, then verify the captured pump task terminates; otherwise removing this cleanup would leave the suite green while reintroducing the leak.

        catch
        {
            // The pumps of the processors we built before the failure are already running, and a bus whose
            // InitAsync threw never reaches SetBuiltMessageBus, so nothing would ever complete their channels
            // and they would park forever, rooting their consumers and everything queued for them.
            foreach (IAsyncConsumerDataProcessor processor in _consumerProcessor.Values)
            {
                processor.Dispose();
            }

src/Platform/Microsoft.Testing.Platform/Messages/ShutdownTimeouts.cs:42

• This validation does not cover the actual timeout range used by every processor. BlockingConsumerDataProcessor passes the value to SemaphoreSlim.WaitAsync(TimeSpan), whose millisecond timeout is limited to int.MaxValue; for example 3000000 seconds passes this check but makes shutdown throw instead of honoring the override. Conversely, a tiny positive value such as 1e-300 rounds to TimeSpan.Zero even though zero is meant to fall back. Validate the converted value against the tightest sink and require it to remain positive.

        // The upper bound is what Task.WaitAsync and CancellationTokenSource.CancelAfter accept. Without it a
        // syntactically valid but absurd value such as '1e300' would parse and then throw out of TimeSpan or the
        // wait itself, so an optional override could break the message bus instead of being ignored.
        return double.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture, out double seconds)
            && seconds > 0
            && seconds * 1000 <= Helpers.TaskExtensions.MaxSupportedTimeoutMs
            ? TimeSpan.FromSeconds(seconds)
            : DefaultCanceledConsumerCompletion;

• Files reviewed: 15/15 changed files
• Comments generated: 3
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

Sequential processor completion can incorrectly retain idle consumers, and timeout validation accepts values that round to zero.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

src/Platform/Microsoft.Testing.Platform/Messages/AsynchronousMessageBus.cs:355

• CompleteAllProcessorsAsync waits for each processor before it even completes the next one's channel. If an early consumer exhausts the canceled-run budget, the outer wait returns while later async pumps are still waiting on open channels; IsConsumerRunning then classifies those idle pumps as running, and Dispose permanently latches them into ConsumersStillRunning. This skips disposal of unrelated later consumers (notably across long-lived server requests), even though they were never executing ConsumeAsync. Complete all processor channels before awaiting their completion, or track actual consumer execution separately.

        foreach (IAsyncConsumerDataProcessor processor in _distinctProcessors)
        {
            try
            {
                using (_shutdownProgressReporter?.Track(processor.DataConsumer.Uid, processor.DataConsumer.DisplayName, nameof(IAsyncConsumerDataProcessor.CompleteAddingAsync)))
                {
                    await processor.CompleteAddingAsync().ConfigureAwait(false);

src/Platform/Microsoft.Testing.Platform/Messages/ShutdownTimeouts.cs:42

• A positive sub-tick value (for example 1e-10) passes these checks but TimeSpan.FromSeconds rounds it to TimeSpan.Zero. That bypasses the intended zero-value fallback and makes cancellation abandon the consumer wait immediately. Validate the constructed TimeSpan is nonzero before accepting the override.

        return double.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture, out double seconds)
            && seconds > 0
            && seconds * 1000 <= Helpers.TaskExtensions.MaxSupportedTimeoutMs
            ? TimeSpan.FromSeconds(seconds)
            : DefaultCanceledConsumerCompletion;

src/Platform/Microsoft.Testing.Platform/Messages/AsynchronousMessageBus.cs:101

• The new failed-initialization cleanup path is not exercised by the message-bus tests: all test consumers return true from IsEnabledAsync, and no test causes initialization to fail after a processor has been created. Add a test with one valid consumer followed by a disabled/throwing consumer and verify the first pump terminates, otherwise this leak-prevention behavior can regress unnoticed.

        try
        {
            await InitCoreAsync().ConfigureAwait(false);
        }
        catch
        {
            // The pumps of the processors we built before the failure are already running, and a bus whose
            // InitAsync threw never reaches SetBuiltMessageBus, so nothing would ever complete their channels
            // and they would park forever, rooting their consumers and everything queued for them.
            foreach (IAsyncConsumerDataProcessor processor in _consumerProcessor.Values)
            {
                processor.Dispose();

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

[github-actions]

🤖 Automated content by GitHub Copilot. Generated by the Grade Tests on PR (on open / sync) workflow. · sonnet46 187.6 AIC · ⌖ 4.12 AIC · ⊞ 11.8K · ◷

[Copilot]

🟡 Not ready to approve

Timeout validation and persistent server-mode consumer handling remain unsafe.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

src/Platform/Microsoft.Testing.Platform/Messages/ShutdownTimeouts.cs:43

• This upper bound is too large for the blocking-consumer path. BlockingConsumerDataProcessor.CompleteAddingAsync passes the value to SemaphoreSlim.WaitAsync(TimeSpan), whose maximum is Int32.MaxValue milliseconds, not TaskExtensions.MaxSupportedTimeoutMs (UInt32.MaxValue - 1). For example, an override of 3000000 seconds is accepted here but makes cancellation shutdown throw immediately instead of honoring the configured budget. Validate against the strictest downstream API.

        return double.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture, out double seconds)
            && seconds > 0
            && seconds * 1000 <= Helpers.TaskExtensions.MaxSupportedTimeoutMs
            ? TimeSpan.FromSeconds(seconds)

• Files reviewed: 15/15 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Human review recommended

Shutdown still has unresolved publisher, server-lifetime, and sequential-completion resource risks.

Review details

Suppressed comments (1)

src/Platform/Microsoft.Testing.Platform/Messages/AsynchronousMessageBus.cs:355

• CompleteAddingAsync both signals and waits, so this sequential loop can spend almost the entire global budget on the first uncooperative consumer before later processors are even told to stop. If the outer timeout wins while the loop is moving on, those later async pumps still report IsConsumerRunning merely because their channels remain open, causing otherwise idle/cooperative consumers to be skipped by disposal (and to accumulate across canceled server requests). Signal every processor before waiting, or start these completions concurrently and then aggregate their failures, so ConsumersStillRunning contains only consumers that actually outlive the shared budget.

        foreach (IAsyncConsumerDataProcessor processor in _distinctProcessors)
        {
            try
            {
                using (_shutdownProgressReporter?.Track(processor.DataConsumer.Uid, processor.DataConsumer.DisplayName, nameof(IAsyncConsumerDataProcessor.CompleteAddingAsync)))
                {
                    await processor.CompleteAddingAsync().ConfigureAwait(false);

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Human review recommended

The concurrency-sensitive teardown still has acknowledged deferred races, and timeout validation accepts positive values that collapse to zero.

Review details

Suppressed comments (1)

src/Platform/Microsoft.Testing.Platform/Messages/ShutdownTimeouts.cs:46

• A positive value below TimeSpan's resolution (for example 1e-300) passes these checks, but TimeSpan.FromSeconds rounds it to TimeSpan.Zero. That bypasses the intended rejection of zero and makes canceled shutdowns abandon consumers immediately. Validate the constructed timeout before returning it.

        return double.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture, out double seconds)
            && seconds > 0
            && seconds * 1000 <= int.MaxValue
            ? TimeSpan.FromSeconds(seconds)

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The timeout parser mishandles NaN, and one timing-sensitive test can be flaky under the project’s parallel net462 execution.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

src/Platform/Microsoft.Testing.Platform/Messages/ShutdownTimeouts.cs:44

• double.TryParse accepts "NaN", but every comparison with NaN is false, so this value reaches TimeSpan.FromSeconds and throws instead of falling back to the default. Reject NaN explicitly so a malformed optional override cannot break message-bus initialization.

        if (!double.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture, out double seconds)
            || seconds <= 0
            || seconds * 1000 > int.MaxValue)

test/UnitTests/Microsoft.Testing.Platform.UnitTests/Messages/AsynchronousMessageBusTests.cs:831

• This test assumes all idle ITask.Run pumps finish within 200 ms. The assembly runs methods in parallel (Program.cs:11), and this same file documents net462 thread-pool starvation for these pumps at lines 39-47; under that condition ConsumersStillRunning can still include an idle consumer and fail intermittently. Isolate this timing-sensitive test as the earlier pump test does.

    [TestMethod]
    public async Task DisableAsync_WhenOnlyOneCanceledConsumerIgnoresTheToken_ShouldNotReportTheCooperativeOnes()

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

Timeout validation still accepts sub-millisecond values that downstream waits effectively treat as zero.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

src/Platform/Microsoft.Testing.Platform/Messages/ShutdownTimeouts.cs:58

• Sub-millisecond positive values still pass this check (for example 0.0001 seconds produces a nonzero TimeSpan), but SemaphoreSlim.WaitAsync(TimeSpan) converts the timeout to whole milliseconds, so the blocking-consumer shutdown abandons the consumer immediately. This contradicts the fallback intended for values that effectively round to zero; require at least one millisecond before accepting the override.

        var timeout = TimeSpan.FromSeconds(seconds);
        return timeout > TimeSpan.Zero ? timeout : DefaultCanceledConsumerCompletion;

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Human review recommended

Known unresolved publisher-shutdown and server-scope lifetime races require final human review.

Review details

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[github-actions]

🤖 Automated content by GitHub Copilot. Generated by the Grade Tests on PR (on open / sync) workflow. · sonnet46 290.4 AIC · ⌖ 4.62 AIC · ⊞ 11.8K · ◷