Drain crashed testhost stderr without blocking a thread-pool thread

[Jakub Jareš (nohwnd)]

RunTestsShouldThrowOnStackOverflowException is still flaky, even after #16128. The abort message comes back as just Process path: ...\testhost.exe with the Stack overflow. line missing.

When a testhost crashes, .NET writes Stack overflow. to its stderr, and we collect that asynchronously through ErrorDataReceived. Under load - many test hosts running in parallel on CI, thread-pool starvation - that callback can fire noticeably late, after the process has already exited. #16128 added a wait for the stderr stream to reach EOF before reading it, but two things kept it flaky:

• the wait shared a single 500ms budget with the process-exit wait, so by the time we got to draining there was often little of it left, and
• it blocked a thread-pool thread, competing for the very thread the pending ErrorDataReceived callback needs to deliver EOF.

So under starvation the wait could starve out the thing it was waiting for, time out, and read an empty buffer - dropping the callstack and truncating the abort message.

This decouples the stderr drain from the exit-wait budget and awaits the EOF signal instead of blocking on it. The wait returns the instant EOF arrives, so a process that drains promptly costs almost nothing.

Budgets, and why a crash is the only generous one

A long budget is only right for a genuine crash. When we tear a testhost down on purpose - aborting or cleaning up a run from an IDE - and a grandchild process (e.g. a browser driver) holds the stderr pipe open, EOF never comes, and the abort would sit there for the whole budget. So there are three:

situation	budget
crash - abnormal exit of a process we were not tearing down	5s
clean exit	500ms
we are tearing the process down	100ms

The teardown budget is short but not zero, because output already sitting in the pipe costs nothing to pick up, and dropping it would replace a real error with a blank one.

Teardown is an explicit per-process cancellation signal, raised by TerminateProcess before it kills. Not an inference from the exit code, which does not hold up:

• Both DefaultTestHostManager.CleanTestHostAsync and DotnetTestHostManager.CleanTestHostAsync call TerminateProcess and then dispose the Process immediately. After Dispose, HasExited and ExitCode throw InvalidOperationException, so there is no exit code left to read on the very path the check exists for.
• A budget picked once, at exit time, cannot shorten a drain that is already running. An abort that arrives during a crash drain has to be able to cut it short.
• A testhost that exits abnormally on its own while a grandchild holds the pipe - the Edge Driver case the surrounding code already describes - is not a kill we asked for, so no amount of kill tracking covers it.

Because the signal is a cancellation, TerminateProcess also raises it for a process that has already exited, which is what shortens an in-flight drain.

Measurements

Against a child whose grandchild holds the stderr pipe open, measuring the delay that TestRequestSender.GetAbortErrorMessage waits out - which is what an abort in Visual Studio ends up waiting for:

scenario	exit-code inference	teardown signal
abort arrives while the crash drain is in flight	5158 ms	313 ms
kill a running process	1176 ms	624 ms
crash with a late callstack	3192 ms, callstack captured	3348 ms, callstack captured

Also in here

• EOF is signalled after the last ErrorDataReceived callback rather than before it, so anyone observing it is guaranteed to see the complete output.
• Corrected the comment about WaitForExitAsync. It does wait for the redirected streams to reach EOF, it just stops waiting as soon as its token is cancelled - which is why a separate, longer drain is still needed.
• using on the exit-wait CancellationTokenSource, so its timer is released instead of leaking one per process exit.

ProcessHelperTests cover the drain primitive (returns on EOF, stays bounded when EOF never comes, no-op when there is nothing to drain, cut short by a teardown signalled up front and by one that arrives mid-wait, and still capturing output that lands inside the teardown budget) and the budget selection. The race doesn't reproduce deterministically here, so the real proof is the CI flake rate.

Not fixed here

Worth knowing, because it sits right next to this: when we do have to kill a testhost, the exit callback never runs at all. CleanTestHostAsync disposes the Process right after TerminateProcess, and Process.Dispose() unregisters the exit watcher - measured 0/20 with the immediate dispose, 20/20 without it. So _clientExited is never set and GetAbortErrorMessage waits out its full 10s. That is a bigger abort win than anything in this PR, but it is a separate change.

🤖

[Copilot]

Pull request overview

This PR hardens ProcessHelper’s crash diagnostics by awaiting stderr EOF asynchronously (with separate clean-exit vs crash budgets) so late-delivered crash output like Stack overflow. isn’t truncated under thread-pool starvation.

Changes:

• Replace the stderr “drain” primitive with an async, non-thread-blocking WaitForErrorStreamToDrainAsync and use longer timeout on abnormal exit.
• Update ProcessHelper’s stderr EOF signal from ManualResetEventSlim to TaskCompletionSource (with RunContinuationsAsynchronously).
• Update ProcessHelperTests to cover the new async drain helper.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs	Await stderr EOF without blocking a thread-pool thread; use separate drain timeouts for clean vs crashed exits.
test/vstest.console.UnitTests/ProcessHelperTests.cs	Convert drain helper tests to async and validate bounded / no-op behavior.

[Jakub Jareš (nohwnd)]

This is about tenth attempt at fixing this, so needs careful review, especially considering that the process needs to exit fast in IDE, especially where there is no crash.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

test/vstest.console.UnitTests/ProcessHelperTests.cs:82

• The bounded-wait test is currently too permissive: it would still pass even if the implementation waited far longer than the provided 500ms timeout (it only asserts < 5000ms) or returned much earlier than the intended timeout (it only asserts >= 150ms). Tightening these bounds makes the test actually validate the intended behavior and catch regressions.

        Assert.IsGreaterThanOrEqualTo(

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs:203

• new CancellationTokenSource(processExitTimeout) allocates a timer even on the !NET + Windows path, where the token isn’t used (Windows uses WaitForExit(processExitTimeout) instead). On CI with many testhosts, this adds avoidable per-exit timer allocations; consider allocating the timed CTS only in the #if NET and Unix !NET branches (or using an untimed CTS + CancelAfter only where needed).

                            // behavior using the parameterless 'WaitForExit()' combined with an awaited Task.Run call.
                            // 'using' so the timer the timeout allocates is released as soon as we are done waiting,
                            // instead of leaking one per process exit when many test hosts are spawned.
                            using var cts = new CancellationTokenSource(processExitTimeout);
#if NET

test/vstest.console.UnitTests/ProcessHelperTests.cs:85

• The lower-bound assertion is too lax compared to the stated intent (“wait roughly the timeout”). With BudgetMs = 500, allowing completion after only 150ms could let regressions slip through where the method returns significantly earlier than the requested timeout when EOF never arrives.

        Assert.IsGreaterThanOrEqualTo(
            150L,
            stopwatch.ElapsedMilliseconds,
            $"The method should wait roughly the timeout for the stream (waited only {stopwatch.ElapsedMilliseconds} ms).");

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.