Wait for testhost stderr to drain before reading its crash output

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

@@ -127,16 +127,32 @@ void InitializeAndStart()
                 process.OutputDataReceived += (sender, args) => outputCallBack(sender as Process, args.Data);
             }
 
+            // Set once the redirected stderr stream reaches EOF (signaled by a null Data event,
+            // which is raised after all stderr lines have been handed to errorCallback). This is
+            // the only reliable signal that the asynchronously-collected error output is complete:
+            // neither WaitForExit(timeout) nor WaitForExitAsync guarantees the ErrorDataReceived
+            // callbacks have run. The exit handler below waits (bounded) on this before reading.
+            ManualResetEventSlim? errorStreamClosed = null;
             if (errorCallback != null)
             {
-                process.ErrorDataReceived += (sender, args) => errorCallback(sender as Process, args.Data);
+                errorStreamClosed = new ManualResetEventSlim(initialState: false);
+                process.ErrorDataReceived += (sender, args) =>
+                {
+                    if (args.Data is null)
+                    {
+                        errorStreamClosed.Set();
+                    }
+
+                    errorCallback(sender as Process, args.Data);
+                };
             }
 
             if (exitCallBack != null)
             {
                 process.Exited += async (sender, args) =>
                 {
                     const int timeout = 500;
+                    var stopwatch = Stopwatch.StartNew();
 
                     if (sender is Process p)
                     {
@@ -148,9 +164,11 @@ void InitializeAndStart()
                             // See ticket https://github.com/microsoft/vstest/issues/3375 to get the links to all
                             // issues, discussions and documentations.
                             //
-                            // On .NET 5 and later, the solution is simple, we can simply use WaitForExitAsync which
-                            // correctly ensure that some time is given to the child process (or any grandchild) to
-                            // flush before exit happens.
+                            // On .NET 5 and later we use WaitForExitAsync to give the child process (and any
+                            // grandchild) some time to exit. NOTE: WaitForExitAsync only waits for the process
+                            // to exit; it does NOT guarantee that the asynchronous Output/ErrorDataReceived
+                            // callbacks have finished delivering. The bounded stderr drain after this block
+                            // ensures the captured error output is complete before exitCallBack reads it.
                             //
                             // For older frameworks, the solution is more tricky but it seems we can get the expected
                             // behavior using the parameterless 'WaitForExit()' combined with an awaited Task.Run call.
@@ -203,6 +221,13 @@ void InitializeAndStart()
                             // We "expect" TaskCanceledException, COMException (if process was disposed before calling
                             // the exit) or InvalidOperationException.
                         }
+
+                        // The process has exited. Within the SAME bounded budget used above, wait for the
+                        // redirected stderr to reach EOF so that asynchronously-collected error output
+                        // (e.g. a testhost crash callstack such as "Stack overflow.") is complete before the
+                        // exit callback consumes it. WaitForExit(timeout)/WaitForExitAsync do not guarantee
+                        // the ErrorDataReceived callbacks have run.
+                        WaitForErrorStreamToDrain(errorStreamClosed, timeout, stopwatch.ElapsedMilliseconds);
                     }
 
                     // If exit callback has code that access Process object, ensure that the exceptions handling should be done properly.
@@ -226,6 +251,28 @@ void InitializeAndStart()
         }
     }
 
+    /// <summary>
+    /// Waits, bounded by the time remaining in <paramref name="budgetMilliseconds"/>, for the redirected
+    /// standard error stream to reach EOF (signaled via <paramref name="errorStreamClosed"/>). This ensures
+    /// all <see cref="Process.ErrorDataReceived"/> callbacks have completed - and therefore the captured
+    /// error output is complete - before it is consumed by the exit callback. It returns immediately when
+    /// there is no redirected error stream, when it has already drained, or when the budget is already
+    /// exhausted (e.g. a grandchild process keeps the pipe open), so the caller can never hang.
+    /// </summary>
+    internal static void WaitForErrorStreamToDrain(ManualResetEventSlim? errorStreamClosed, int budgetMilliseconds, long elapsedMilliseconds)
+    {
+        if (errorStreamClosed is null)
+        {
+            return;
+        }
+
+        var remainingMilliseconds = budgetMilliseconds - (int)elapsedMilliseconds;
+        if (remainingMilliseconds > 0)
+        {
+            errorStreamClosed.Wait(remainingMilliseconds);
+        }
+    }
+
     /// <inheritdoc/>
     public string? GetCurrentProcessFileName()
     {

test/vstest.console.UnitTests/ProcessHelperTests.cs

@@ -0,0 +1,102 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Diagnostics;
+using System.Threading;
+
+using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace Microsoft.VisualStudio.TestPlatform.CommandLine.UnitTests;
+
+/// <summary>
+/// Tests for <see cref="ProcessHelper.WaitForErrorStreamToDrain"/>, the bounded wait that lets the process
+/// exit callback observe the complete standard error output of a crashed test host. Without it, the exit
+/// callback could read the asynchronously-collected stderr before all ErrorDataReceived callbacks had run,
+/// dropping a crash callstack such as "Stack overflow." (the cause of the flaky
+/// RunTestsShouldThrowOnStackOverflowException test).
+/// </summary>
+[TestClass]
+public class ProcessHelperTests
+{
+    private const int BudgetMs = 500;
+
+    [TestMethod]
+    public void WaitForErrorStreamToDrainShouldReturnOnceTheErrorStreamCloses()
+    {
+        using var errorStreamClosed = new ManualResetEventSlim(initialState: false);
+
+        // The stream reaches EOF a little later, mimicking a slow ErrorDataReceived delivery.
+        var setter = new Thread(() =>
+        {
+            Thread.Sleep(150);
+            errorStreamClosed.Set();
+        })
+        { IsBackground = true };
+
+        var stopwatch = Stopwatch.StartNew();
+        setter.Start();
+        ProcessHelper.WaitForErrorStreamToDrain(errorStreamClosed, budgetMilliseconds: 5000, elapsedMilliseconds: 0);
+        stopwatch.Stop();
+        setter.Join();
+
+        Assert.IsTrue(errorStreamClosed.IsSet, "The method must wait until the error stream is drained.");
+        Assert.IsLessThan(
+            3000L,
+            stopwatch.ElapsedMilliseconds,
+            $"The method should return shortly after the stream closes, not at the budget timeout (took {stopwatch.ElapsedMilliseconds} ms).");
+    }
+
+    [TestMethod]
+    public void WaitForErrorStreamToDrainShouldBeBoundedWhenTheErrorStreamNeverCloses()
+    {
+        // Models a grandchild process keeping the pipe open: EOF never arrives.
+        using var errorStreamClosed = new ManualResetEventSlim(initialState: false);
+
+        var stopwatch = Stopwatch.StartNew();
+        ProcessHelper.WaitForErrorStreamToDrain(errorStreamClosed, BudgetMs, elapsedMilliseconds: 0);
+        stopwatch.Stop();
+
+        Assert.IsFalse(errorStreamClosed.IsSet, "Precondition: the stream never closes in this test.");
+        Assert.IsGreaterThanOrEqualTo(
+            150L,
+            stopwatch.ElapsedMilliseconds,
+            $"The method should wait roughly the budget for the stream (waited only {stopwatch.ElapsedMilliseconds} ms).");
+        Assert.IsLessThan(
+            5000L,
+            stopwatch.ElapsedMilliseconds,
+            $"The wait must be bounded so it cannot hang (took {stopwatch.ElapsedMilliseconds} ms).");
+    }
+
+    [TestMethod]
+    public void WaitForErrorStreamToDrainShouldNotWaitWhenTheBudgetIsAlreadyExhausted()
+    {
+        // The exit wait above already consumed the whole budget (e.g. a slow grandchild), so there is no
+        // time left to wait for stderr - we must not add any latency on top.
+        using var errorStreamClosed = new ManualResetEventSlim(initialState: false);
+
+        var stopwatch = Stopwatch.StartNew();
+        ProcessHelper.WaitForErrorStreamToDrain(errorStreamClosed, BudgetMs, elapsedMilliseconds: BudgetMs + 100);
+        stopwatch.Stop();
+
+        Assert.IsFalse(errorStreamClosed.IsSet);
+        Assert.IsLessThan(
+            250L,
+            stopwatch.ElapsedMilliseconds,
+            $"With the budget exhausted the method must return immediately (took {stopwatch.ElapsedMilliseconds} ms).");
+    }
+
+    [TestMethod]
+    public void WaitForErrorStreamToDrainShouldReturnImmediatelyWhenThereIsNoErrorStream()
+    {
+        var stopwatch = Stopwatch.StartNew();
+        ProcessHelper.WaitForErrorStreamToDrain(errorStreamClosed: null, BudgetMs, elapsedMilliseconds: 0);
+        stopwatch.Stop();
+
+        Assert.IsLessThan(
+            250L,
+            stopwatch.ElapsedMilliseconds,
+            $"With no redirected error stream the method must be a no-op (took {stopwatch.ElapsedMilliseconds} ms).");
+    }
+}