Wait for testhost stderr to drain before reading its crash output - #16128
Merged
Jakub Jareš (nohwnd) merged 1 commit intoJun 17, 2026
Merged
Conversation
When a test host crashes (e.g. with a stack overflow) its stderr is collected asynchronously through ErrorDataReceived. The Process.Exited handler in ProcessHelper read that captured stderr as soon as the process had exited, but neither WaitForExit(timeout) nor WaitForExitAsync waits for the async ErrorDataReceived callbacks to finish. So the error output was sometimes still empty when it was read, and the abort message ended up as just "Test host process crashed" without the "Stack overflow." detail. That is what makes RunTestsShouldThrowOnStackOverflowException flaky. The redirected stderr stream raises one last ErrorDataReceived with null Data when it reaches EOF, after all the data lines. Wait (bounded) for that signal before invoking the exit callback. The wait shares the existing ~500ms budget, so it does not add latency in the common case where the stream is already drained, and it stays bounded so a grandchild that keeps the pipe open (the Selenium/Edge case) cannot make us hang. Add ProcessHelper.WaitForErrorStreamToDrain and unit tests for it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a real race in ProcessHelper where a crashed testhost’s redirected stderr can still be in-flight via ErrorDataReceived when the Exited handler runs, causing consumers (and a flaky end-to-end test) to sometimes miss crash details like Stack overflow..
Changes:
- Add a bounded wait for redirected stderr to reach EOF (null
ErrorDataReceived), sharing the existing ~500ms exit budget, before invoking the exit callback. - Introduce
ProcessHelper.WaitForErrorStreamToDrainto encapsulate the “wait-for-EOF within remaining budget” behavior. - Add unit tests validating drain-wait behavior (waits for EOF, bounded when EOF never arrives, and no-op when budget is exhausted or stderr isn’t redirected).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs |
Waits (bounded) for stderr EOF before running the exit callback so crash output is fully captured. |
test/vstest.console.UnitTests/ProcessHelperTests.cs |
Adds unit tests covering the bounded stderr-drain wait logic. |
Jakub Jareš (nohwnd)
marked this pull request as ready for review
June 17, 2026 09:32
Jakub Jareš (nohwnd)
enabled auto-merge (squash)
June 17, 2026 11:05
Amaury Levé (Evangelink)
approved these changes
Jun 17, 2026
This was referenced Jun 23, 2026
Jakub Jareš (nohwnd)
added a commit
to nohwnd/vstest
that referenced
this pull request
Jun 26, 2026
…rosoft#16128) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jakub Jareš (nohwnd)
added a commit
that referenced
this pull request
Jun 30, 2026
…tyMatrix] (#16174) * Unify acceptance test data sources into [TestMatrix] and [CompatibilityMatrix] The acceptance tests had nine data-source attributes that overlapped and read inconsistently - Runner vs VSTestConsole, exclusion bools you had to mentally invert. Collapse them into two that read positively: [TestMatrix(console, testHost, ...)] for the framework matrix and [CompatibilityMatrix(scenario)] for the version-compat matrix. You pin an axis instead of excluding one. Migrated all in-repo call sites (48 files, a literal name swap). The old attributes stay for now so the open PRs that still use them keep building; they come out in a follow-up once those have merged. Checked the new attributes against the old ones for every call shape - same frameworks, /InIsolation, VSIX rows and order all match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Re-run CI (flaky RunTestsShouldThrowOnStackOverflowException, see #16128) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Make vsix additive in [TestMatrix] so it always adds a VSIX run Previously the VSIX row was nested under the NetFx console/host axes, so combinations like console: Net or testHost: Net silently dropped it. Emit it whenever vsix: true (Windows-only), independent of the axes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Unify TestMatrix console/testHost axes into a single Target enum VSTestConsole and TestHost were structurally identical ({ Both, NetFx, Net }). Collapse them into one Target enum and global-using-static it in the two integration test projects, so call sites read [TestMatrix(console: NetFx, testHost: Net)] The console:/testHost: parameter names carry the axis the type used to. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 14, 2026
Bump Microsoft.NET.Test.Sdk from 18.6.0 to 18.9.0
Analogy-LogViewer/Analogy.LogViewer.JsonParser#264
Open
This was referenced Aug 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The flaky
RunTestsShouldThrowOnStackOverflowExceptionis a real race, not a test problem.When a testhost crashes, its stderr (the
Stack overflow.line) is collected asynchronously viaErrorDataReceived. TheProcess.Exitedhandler inProcessHelperread that buffer as soon as the process had exited, but neitherWaitForExit(timeout)norWaitForExitAsyncwaits for the asyncErrorDataReceivedcallbacks to finish. So the buffer was sometimes still empty and the abort message came out as justTest host process crashed, without theStack overflow.detail the test asserts.The redirected stderr raises one last
ErrorDataReceivedwithnullData on EOF, after all the lines. Wait (bounded) for that before calling the exit callback. It shares the existing ~500ms budget, so it adds no latency when the stream is already drained (the common case), and stays bounded so a grandchild that keeps the pipe open (the Selenium/Edge case, issue 3375) cannot make us hang.Verification:
ProcessHelper.WaitForErrorStreamToDrain(waits for EOF, bounded when EOF never comes, no-op when the budget is spent or there is no error stream). Pass on net11.0 and net481.build.cmd -c Releasepasses locally.Process.Exitedpath: theWaitForExitAsyncpath drops stderr ~2/80 under load; with the drain wait it is 0/80.Note: the race is timing-dependent (~1/100 in CI), so I did not try to assert it deterministically through a real process exit - the unit tests cover the drain logic and
RunTestsShouldThrowOnStackOverflowExceptionstays as the end-to-end coverage.