Allow controller extensions to finalize after timeout

[Amaury Levé (Evangelink)]

Fixes #10791

Summary

• keep --timeout ownership exclusively in the test host, including process-isolated runs
• let the controller detect an aborted child exit and run controller lifetime handlers with a fresh bounded cleanup token
• bound controller data-consumer shutdown and avoid concurrent lifecycle callbacks or disposal for abandoned handlers
• preserve non-success exit semantics and recover completed TRX results with consistent aborted-run diagnostics

Validation

• targeted Debug builds plus Microsoft.Testing.Platform and TRX package generation with binary logs
• 26 focused stop-policy and controller-finalization unit tests
• 18 invalid-timeout acceptance cases (net462, net8.0, net10.0)
• 2 TRX data-row acceptance cases (net8.0, net10.0)
• 4 timeout lifetime-handler/TRX recovery acceptance cases (net8.0, net10.0)

[github-actions]

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

✅ 22/22 dimensions clean — no findings.

Review notes:

• Algorithmic Correctness: The timeout ownership move from CommonServices to BuildAsync (gated on host is not TestHostControllersTestHost && !HasTestHostController) correctly ensures only the in-process host applies the application-level cancellation, while the controller manages its own timeout via a separate CancellationTokenSource. Exit code logic correctly distinguishes timeout-aborted from crashed hosts.
• Threading & Concurrency: _skipLifetimeHandlerDisposal is written during RunTestHostProcessAsync and read during DisposeServicesAsync — these execute sequentially (run → dispose), so no synchronization is needed. All CancellationTokenSource instances are properly scoped with using.
• Resource Management: timeoutCancellationTokenSource, executionCancellationTokenSource, and finalizationCancellationTokenSource are all using-declared. The TestHostTerminationTimeout on WaitForExitAsync prevents an unbounded hang when a test host ignores termination.
• Defensive Coding: TryFinalizeControllerExtensionAsync applies double-cancellation (finalization(token) + .WithCancellationAsync(token)) — this is intentionally defensive against handlers that ignore the token internally.
• Test Completeness: Two unit tests cover TryFinalizeControllerExtensionAsync (uncanceled token, bounded finalization). Two acceptance tests cover end-to-end timeout finalization and TRX recovery with aborted-session diagnostics.
• Flakiness: Thread.Sleep(10000) appears in test asset code (simulating a long-running test host), not in test synchronization — acceptable for acceptance tests.
• Dimensions 3 (Security), 4 (Public API), 6 (Cross-TFM), 9 (Localization), 18 (Analyzer Quality), 19 (IPC Wire), 20 (Build Infrastructure), 22 (PowerShell) are N/A for this change set.

[github-actions]

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 94.8 AIC · ⌖ 1.13 AIC · ⊞ 16.9K · ◷

[Copilot]

Pull request overview

Moves process-isolated timeout ownership to the controller, enabling bounded post-timeout extension finalization and TRX recovery.

Changes:

• Separates execution timeout cancellation from controller cleanup.
• Preserves aborted exit semantics and recovers partial TRX results.
• Adds unit and acceptance coverage for timeout finalization.

Show a summary per file

File	Description
TestApplicationBuilderTests.cs	Tests bounded finalization tokens.
TrxTests.cs	Tests partial TRX timeout recovery.
TestHostProcessLifetimeHandlerTests.cs	Tests uncanceled cleanup tokens.
TestHostControllersTestHost.ProcessLifecycle.cs	Implements timeout and finalization flow.
TestHostControllersTestHost.Disposal.cs	Avoids disposing active handlers.
TestHostControllersTestHost.cs	Adds timeout configuration and state.
TestHostBuilder.cs	Assigns timeout ownership by host role.
TestHostBuilder.CommonServices.cs	Removes global timeout scheduling.
TrxProcessLifetimeHandler.cs	Adds aborted-run TRX diagnostics.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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

[Copilot]

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

src/Platform/Microsoft.Testing.Platform/Hosts/TestHostControllersTestHost.ProcessLifecycle.cs:130

• The execution timer starts after every OnTestHostProcessStartedAsync callback, but those callbacks do not block the child: after the PID handshake reply, the child continues building/running independently. A slow start callback can therefore let tests execute beyond --timeout before this line even arms the timer, extending the advertised global execution deadline. Start the controller-owned deadline when the child is launched or connected, while retaining a separate token only for finalization.

            timeoutCancellationTokenSource?.CancelAfter(executionTimeout!.Value);

src/Platform/Microsoft.Testing.Platform/Hosts/TestHostControllersTestHost.ProcessLifecycle.cs:214

• This wrapper bounds only the current await, not the message-bus shutdown itself. AsynchronousMessageBus.DisableCoreAsync uses the still-uncanceled application token and waits indefinitely; after this cleanup token expires, DisposeServicesAsync calls EnsureMessageBusDisabledAsync and re-awaits that same _disableTask without a timeout. An uncooperative consumer can therefore still hang a timed-out controller indefinitely. Transition the bus to its canceled/bounded shutdown mode, or make the mandatory disposal re-await honor the cleanup deadline.

                    await messageBusProxy.DisableAsync().WithCancellationAsync(finalizationCancellationToken).ConfigureAwait(false);

src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxProcessLifetimeHandler.cs:209

• The aborted branch updates the TRX diagnostic, but the recovery warning emitted just below still says Test host crashed... for every recovered/empty sidecar. A normal --timeout run will therefore show contradictory termination and crash diagnoses. Reuse this aborted-vs-crashed distinction when constructing recoverySummary.

            string testHostExitInfo = testHostProcessInformation.ExitCode == (int)ExitCode.TestSessionAborted
                ? $"Test host process pid: {testHostProcessInformation.PID} was terminated because the test session was aborted."
                : $"Test host process pid: {testHostProcessInformation.PID} crashed.";

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

[Copilot]

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/Platform/Microsoft.Testing.Platform/Hosts/TestHostControllersTestHost.ProcessLifecycle.cs:283

• The timeout wrapper only starts after finalization(...) has returned a Task. An extension can block synchronously before its first await (or return), in which case this call never reaches WithCancellationAsync and --timeout remains unbounded. Invoke the callback on a worker before applying the cancellation wrapper; the bounded test should also cover a synchronously blocking delegate.

            await finalization(cancellationToken).WithCancellationAsync(cancellationToken).ConfigureAwait(false);

src/Platform/Microsoft.Testing.Platform/Hosts/TestHostControllersTestHost.ProcessLifecycle.cs:191

• The cleanup budget is created only when at least one lifetime handler exists, but AddDataConsumer alone can force a controller restart (TestHostControllersManager.cs:155). On timeout the application token remains uncanceled, so the no-handler path reaches DisposeServicesAsync and AsynchronousMessageBus.DisableCoreAsync takes its infinite graceful wait. A controller data consumer that does not finish can therefore still hang --timeout; apply the bounded drain/disable path independently of whether lifetime handlers are registered.

            using CancellationTokenSource? finalizationCancellationTokenSource = testExecutionCanceled
                ? new(ControllerExtensionFinalizationTimeout)
                : null;
            CancellationToken finalizationCancellationToken = finalizationCancellationTokenSource?.Token ?? applicationCancellationToken;

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

[Copilot]

Review details

Suppressed comments (1)

src/Platform/Microsoft.Testing.Platform/Hosts/TestHostControllersTestHost.ProcessLifecycle.cs:192

• The cleanup budget is created only when at least one lifetime handler exists. TestHostControllersManager also restarts the process for controller data consumers or environment-variable providers alone; on timeout in that configuration the application token remains uncanceled, and DisposeServicesAsync later awaits the message bus's infinite graceful-disable path. An uncooperative controller consumer can therefore still make --timeout hang forever. Apply the bounded shutdown/message-bus transition for every canceled controller run, independently of whether lifetime handlers are registered.

            using CancellationTokenSource? finalizationCancellationTokenSource = testExecutionCanceled
                ? new(ControllerExtensionFinalizationTimeout)
                : null;
            CancellationToken finalizationCancellationToken = finalizationCancellationTokenSource?.Token ?? applicationCancellationToken;

• Files reviewed: 13/13 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

Review details

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

[Copilot]

Review details

Suppressed comments (1)

src/Platform/Microsoft.Testing.Platform/Hosts/TestHostControllersTestHost.ProcessLifecycle.cs:320

• This diagnostic also runs through ProxyOutputDevice, so timing out while its server-mode branch is active must protect the proxy itself. Skipping only the original device still allows process-shutdown disposal to call ProxyOutputDevice.Dispose() and dispose the nested ServerModePerCallOutputDevice underneath the abandoned callback.

                    if (!_servicesStillRunning.Contains(outputDevice.OriginalOutputDevice))
                    {
                        _servicesStillRunning.Add(outputDevice.OriginalOutputDevice);

• Files reviewed: 20/20 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

Review details

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

[Copilot]

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/Platform/Microsoft.Testing.Platform/Hosts/TestHostControllersTestHost.ProcessLifecycle.cs:257

• When application cancellation arrives while an exit handler is running, finalized == false only means the old application token was canceled; the registration above has just created a fresh, uncanceled cleanup source. This branch nevertheless marks the fresh budget as timed out and stops the loop, so later handlers (including TRX recovery) are never invoked and a misleading timeout warning is emitted. After transitioning tokens, abandon only the current handler and continue with the fresh token unless that cleanup token itself has expired.

                    if (!finalized)
                    {
                        _servicesStillRunning.Add(lifetimeHandler);
                        TransitionToBoundedFinalizationIfCanceled();
                        _controllerFinalizationTimedOut = true;

src/Platform/Microsoft.Testing.Platform/Hosts/TestHostControllersTestHost.Disposal.cs:92

• A cleanup that starts before late application cancellation captures CancellationToken.None, so cancellation arriving while EnsureMessageBusDisabledAsync or a disposer is blocked cannot bound that operation. The transition registration in RunTestHostProcessAsync has already been disposed before DisposeServicesAsync runs, and even creating a new source would not replace this captured token. Use a stable controller-cleanup token whose deadline is armed on abort, or otherwise race in-flight cleanup against the late-created deadline, so cancellation during disposal cannot hang indefinitely.

    private async Task<bool> TryRunControllerCleanupAsync(Func<Task> cleanup)
    {
        CancellationToken cancellationToken = _controllerFinalizationCancellationTokenSource?.Token ?? CancellationToken.None;
        return !cancellationToken.IsCancellationRequested
            && await TryRunControllerExtensionAsync(_ => cleanup(), cancellationToken).ConfigureAwait(false);

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

[github-actions]

🧵 Parallel-safety audit — PR #10797

Parallelization — one row per test assembly audited:

Test assembly	Scope	Workers	Analyzer coverage
Microsoft.Testing.Platform.UnitTests	MethodLevel	CPU count (Workers = 0)	coverable once MSTEST0074-0077 ship (assembly attribute in Program.cs)
Microsoft.Testing.Platform.Acceptance.IntegrationTests	MethodLevel	CPU count (Workers = 0)	coverable once MSTEST0074-0077 ship (assembly attribute in Program.cs)

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0.

Audited all 5 changed test files:

• test/UnitTests/Microsoft.Testing.Platform.UnitTests/Hosts/CommonHostTests.cs — 2 new test methods, entirely local ServiceProvider/Mock<IDisposable> instances, no shared state.
• test/UnitTests/Microsoft.Testing.Platform.UnitTests/Messages/AsynchronousMessageBusTests.cs — 1 new test method using Mock<IEnvironment> (mocked, not real Environment.SetEnvironmentVariable/GetEnvironmentVariable), so no process-global mutation.
• test/UnitTests/Microsoft.Testing.Platform.UnitTests/Services/StopPoliciesServiceTests.cs — 2 new test methods plus one assertion-value change, all against a locally-constructed StopPoliciesService instance with local TaskCompletionSources; no statics.
• test/UnitTests/Microsoft.Testing.Platform.UnitTests/TestApplicationBuilderTests.cs — 8 new test methods, all against locally-constructed hosts/mocks (Mock<IEnvironment>, Mock<IProcess>, Mock<ITestHostHandle>) or reflection calls on local instances; no shared/static state touched.
• test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/TestHostProcessLifetimeHandlerTests.cs and TrxTests.cs — new acceptance tests spawn an out-of-process test host per test (TestInfrastructure.TestHost.ExecuteAsync), passing environment variables that are only observed inside that child process, not the current test-host process, so they do not mutate this process's environment (category A does not apply). New scratch files/directories use Guid.NewGuid()-based unique names (finalizationFile, disposalFile, testResultsPath, etc.), so no cross-test path collisions (category B). No [ResourceLock] / [DoNotParallelize] declarations were added, removed, or need to be (category C).

No changed .runsettings, testconfig.json, .csproj/.props/.targets, or assembly-level [Parallelize]/[DoNotParallelize] attributes were touched by this PR (parallelization state unchanged for both assemblies).

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 133.9 AIC · ⌖ 1.89 AIC · ⊞ 24.8K · [◷]( · ◷)

[Copilot]

Review details

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

[github-actions]

🧪 Expert test review — PR #10797

Grade	Test	Mutation	Notes	How to improve
C (70–79)	new TestApplicationBuilderTests. TryRunControllerExtensionAsync (helper)	N/A	7 new tests reflect into private static/instance methods via MethodInfo.GetMethod, coupling tests to implementation details rather than the public surface.	Expose an internal test seam (e.g. InternalsVisibleTo + internal methods) instead of reflecting on private members.
C (70–79)	new TestApplicationBuilderTests. TestHostControllerFinalization_ LateCancellationArmsSharedBoundedTokenSource	N/A	Uses reflection (EnsureControllerFinalizationCancellationTokenSource, ArmControllerFinalizationTimeout) on private instance methods; behavior itself is correctly asserted.	Replace reflection calls with an internal test-only accessor guarded by InternalsVisibleTo.
C (70–79)	new TestApplicationBuilderTests. TestHostControllerFinalization_ CancellationDuringCleanupArmsStableToken	N/A	Correct concurrency assertions, but couples to private members via reflection (RegisterControllerFinalizationTransition, TryRunControllerCleanupAsync).	Replace reflection with an internal accessor exposed for tests.
C (70–79)	new TestApplicationBuilderTests. TestHostControllerProcessTermination_ CustomHandleDisposalWaitsForExit	N/A	Reflects on private static WaitForExitAfterTerminationAsync; assertions on Dispose timing are otherwise sound.	Expose the method as internal and use InternalsVisibleTo instead of reflection.
C (70–79)	new TestApplicationBuilderTests. TestHostControllerProcessTermination_ WaitIsBounded	N/A	Reflects on private static WaitForExitAfterTerminationAsync; timing bound assertion (5s) is loose but acceptable for this scope.	Expose the method as internal and use InternalsVisibleTo instead of reflection.
C (70–79)	new TestApplicationBuilderTests. TestHostControllerOutputFinalization_ AbandonmentTracksProxyAndOriginalDevice	N/A	Reflects on private static MarkOutputDeviceStillRunning; assertion correctly checks both proxy and wrapped device are tracked.	Expose the method as internal and use InternalsVisibleTo instead of reflection.
B (80–89)	new TestHostProcessLifetimeHandlerTests. Timeout_ BoundsBlockingFinalizationWithoutDisposingRunningHandler	3/3 killed	Verifies finalization started, disposal skipped, and an 8s wall-clock bound; the generous bound risks flakiness under load.	Use a tighter, environment-scaled bound or assert relative to the configured 0.5s timeout instead of a fixed 8s ceiling.
B (80–89)	new TestHostProcessLifetimeHandlerTests. Timeout_ BoundsBlockingDisposalWithoutRetryingIt	3/3 killed	Confirms single dispose attempt and exit within an 8s wall-clock bound; same loose-bound risk as the sibling test.	Scale the assertion bound relative to the 0.5s finalization timeout rather than a fixed constant.
A (90–100)	new TestHostProcessLifetimeHandlerTests. Timeout_ FinalizesProcessLifetimeHandlerWithUncanceledToken	2/2 killed	Exit-code and cancellation-flag assertions directly protect the claimed finalization contract.	—
A (90–100)	new TrxTests. Trx_ WhenTimeoutTerminatesTestHost_ RecoversCompletedResults	3/3 killed	Parses the TRX XML and asserts exact outcome/testName rather than substring checks; strong end-to-end contract coverage.	—
A (90–100)	new CommonHostTests. RunAsync_ WhenInternalRunSkipsService_ DoesNotDisposeItDuringProcessShutdown	2/2 killed	Clear AAA; Verify(Times.Never) directly checks the alreadyDisposed contract added in this PR.	—
A (90–100)	new CommonHostTests. DisposeServiceProviderAsync_ WhenMessageBusIsAlreadyAbandoned_ DoesNotDisableItAgain	2/2 killed	Directly asserts DisableAsync is skipped for a bus already recorded in alreadyDisposed.	—
A (90–100)	new StopPoliciesServiceTests. ExecuteAbortCallbacksAsync_ WhenCalledTwice_ InvokesRegisteredCallbackOnce	2/2 killed	Precisely verifies idempotent execution of abort callbacks, matching the production once-only guard.	—
A (90–100)	new StopPoliciesServiceTests. ExecuteAbortCallbacksAsync_ WhenCalledConcurrently_ ReturnsSharedTask	3/3 killed	Uses TCS-based synchronization (no sleeps) to deterministically prove the shared in-flight task is returned.	—
A (90–100)	mod StopPoliciesServiceTests. RegisterOnAbortCallbackAsync_ ImmediatelyInvokesCallbackIfAlreadyTriggered	1/1 killed	Expected-invocation count corrected from 2 to 1, aligning with the once-only abort-callback semantics verified in production.	—
A (90–100)	new ShutdownTimeoutsTests. GetControllerFinalization_ WhenBothOverridesAreSet_ ShouldUseDedicatedValue	2/2 killed	Distinct env-var mocks and separate assertions for the two override values, avoiding cross-property coupling.	—

_{This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Suggestions on the Files changed tab can be applied with one click. Re-run with /review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 120.2 AIC · ⌖ 2.06 AIC · ⊞ 16.9K · [◷]( · ◷)