Fix ToolTask hang when grandchild processes inherit pipe handles

documentation/wiki/ChangeWaves.md

@@ -31,6 +31,7 @@ Change wave checks around features will be removed in the release that accompani
 
 ### 18.6
 - [AbsolutePath.GetCanonicalForm optimization - avoid expensive Path.GetFullPath calls when paths don't need canonicalization](https://github.com/dotnet/msbuild/pull/13369)
+- [Fix ToolTask hang when tool spawns grandchild processes that inherit stdout/stderr pipe handles](https://github.com/dotnet/msbuild/issues/2981)
 
 ### 18.5
 - [FindUnderPath and AssignTargetPath tasks no longer throw on invalid path characters when using TaskEnvironment.GetAbsolutePath](https://github.com/dotnet/msbuild/pull/13069)

src/Utilities.UnitTests/ToolTask_Tests.cs

@@ -1040,6 +1040,68 @@ public void ToolTaskThatTimeoutAndRetry(int repeats, int initialDelay, int follo
             }
         }
 
+        /// <summary>
+        /// Verifies that ToolTask does not hang when the tool process spawns a grandchild
+        /// process that inherits stdout/stderr pipe handles and outlives the tool.
+        /// This is a regression test for https://github.com/dotnet/msbuild/issues/2981.
+        /// </summary>
+        [Fact]
+        public void ToolTaskDoesNotHangWhenGrandchildInheritsPipeHandles()
+        {
+            using (MyTool t = new MyTool())
+            {
+                MockEngine3 engine = new MockEngine3();
+                t.BuildEngine = engine;
+
+                // cmd echoes "hello", then starts a background ping that inherits
+                // pipe handles. cmd exits immediately; ping outlives the 2s EOF timeout.
+                t.MockCommandLineCommands = NativeMethodsShared.IsWindows
+                    ? "/c echo hello & start /b ping -n 10 127.0.0.1 > nul"
+                    : "-c \"echo hello; sleep 10 &\"";
+
+                // Set a generous timeout - without the fix this would hang for the full ping duration
+                t.Timeout = 30000;
+
+                bool result = t.Execute();
+
+                // The tool should complete without hanging.
+                // The exit code may be non-zero depending on timing, but the key thing
+                // is that Execute() returns at all rather than hanging forever.
+                _output.WriteLine(engine.Log);
+                engine.Log.ShouldContain("hello");
+            }
+        }
+
+        /// <summary>
+        /// Verifies that ToolTask still captures all output from the tool process
+        /// even with the grandchild pipe fix enabled. This is a regression test for
+        /// https://github.com/dotnet/msbuild/issues/10378 where switching to
+        /// WaitForExit(int) caused output to be lost.
+        /// </summary>
+        [Fact]
+        public void ToolTaskCapturesAllOutputWithFix()
+        {
+            using (MyTool t = new MyTool())
+            {
+                MockEngine3 engine = new MockEngine3();
+                t.BuildEngine = engine;
+
+                // Echo multiple lines to verify all output is captured
+                t.MockCommandLineCommands = NativeMethodsShared.IsWindows ?
+                    "/c echo line1 & echo line2 & echo line3"
+                    : "-c \"echo line1; echo line2; echo line3\"";
+
+                bool result = t.Execute();
+
+                _output.WriteLine(engine.Log);
+
+                result.ShouldBeTrue();
+                engine.Log.ShouldContain("line1");
+                engine.Log.ShouldContain("line2");
+                engine.Log.ShouldContain("line3");
+            }
+        }
+
         /// <summary>
         /// A simple implementation of <see cref="ToolTask"/> to sleep for a while.
         /// </summary>

src/Utilities/ToolTask.cs

@@ -745,6 +745,12 @@ protected virtual int ExecuteTool(
             _standardErrorDataAvailable = new ManualResetEvent(false);
             _standardOutputDataAvailable = new ManualResetEvent(false);
 
+            if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_6))
+            {
+                _standardOutputEOF = new ManualResetEvent(false);
+                _standardErrorEOF = new ManualResetEvent(false);
+            }
+
             _toolExited = new ManualResetEvent(false);
             _terminatedTool = false;
             _toolTimeoutExpired = new ManualResetEvent(false);
@@ -838,6 +844,9 @@ protected virtual int ExecuteTool(
                     _standardErrorDataAvailable.Dispose();
                     _standardOutputDataAvailable.Dispose();
 
+                    _standardOutputEOF?.Dispose();
+                    _standardErrorEOF?.Dispose();
+
                     _toolExited.Dispose();
                     _toolTimeoutExpired.Dispose();
 
@@ -1064,13 +1073,40 @@ private void TerminateToolProcess(Process proc, bool isBeingCancelled)
         /// process is still finishing up, this method waits until it is done.
         /// </summary>
         /// <remarks>
-        /// This method is a hack, but it needs to be called after both
-        /// Process.WaitForExit() and Process.Kill().
+        /// On both .NET Framework and modern .NET, the parameterless Process.WaitForExit() waits not
+        /// only for the process to exit, but also for stdout/stderr pipe EOF via
+        /// AsyncStreamReader.WaitUtilEOF() (Framework) or awaiting the EOF task (Core).
+        /// If the tool spawned child processes that inherited the pipe handles, the EOF wait blocks
+        /// forever even though the tool itself has exited — causing the entire build node to hang.
         /// </remarks>
         /// <param name="proc"></param>
-        private static void WaitForProcessExit(Process proc)
+        private void WaitForProcessExit(Process proc)
         {
-            proc.WaitForExit();
+            if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_6))
+            {
+                // Step 1: Wait for the process handle to be signaled.
+                // Use int.MaxValue to avoid blocking on pipe EOF,
+                // as Process.WaitForExit does not wait for EOF when any timeout is provided.
+                proc.WaitForExit(int.MaxValue);
+
+                // Step 2: Wait for the AsyncStreamReader to deliver all remaining data.
+                // When the pipe reaches EOF, AsyncStreamReader flushes its StringBuilder
+                // (delivering any final partial line) and sends Data=null via the callback.
+                // Our ReceiveStandardErrorOrOutputData handler signals the EOF events.
+                //
+                // Use a bounded timeout as a safety net for the grandchild case where
+                // EOF never arrives because grand child inherited the pipe and keeps it open.
+                const int eofTimeoutSec = 2;
+
+                WaitHandle[] eofEvents = [_standardOutputEOF, _standardErrorEOF];
+                WaitHandle.WaitAll(eofEvents, TimeSpan.FromSeconds(eofTimeoutSec));
+            }
+            else
+            {
+                // Legacy behavior: parameterless WaitForExit waits for pipe EOF.
+                // This can hang if grandchild processes hold pipe handles.
+                proc.WaitForExit();
+            }
 
             // Process.WaitForExit() may return prematurely. We need to check to be sure.
             while (!proc.HasExited)
@@ -1236,38 +1272,54 @@ protected void ReceiveExitNotification(object sender, EventArgs e)
         /// <param name="dataAvailableSignal"></param>
         private void ReceiveStandardErrorOrOutputData(DataReceivedEventArgs e, Queue dataQueue, ManualResetEvent dataAvailableSignal)
         {
-            // NOTE: don't ignore empty string, because we need to log that
-            if (e.Data != null)
+            if (e.Data == null)
             {
-                ErrorUtilities.VerifyThrow(dataQueue != null,
-                    "The data queue must be available.");
-
-                // synchronize access to the queue -- this is a producer-consumer problem
-                // NOTE: we lock the entire queue instead of using synchronized queue
-                // wrappers, because ManualResetEvents don't have ref counts, and it's
-                // difficult to discretely signal the availability of each instance of
-                // data in the queue -- so instead we let the consumer lock and empty
-                // the queue and reset the ManualResetEvent, before we add more data
-                // into the queue, and signal the ManualResetEvent again
-                lock (dataQueue.SyncRoot)
+                // The AsyncStreamReader sends Data=null when the pipe reaches EOF.
+                // Signal the appropriate EOF event so WaitForProcessExit knows
+                // all data from this stream has been delivered.
+                ManualResetEvent eofEvent = (dataQueue == _standardErrorData) ? _standardErrorEOF : _standardOutputEOF;
+                if (eofEvent != null)
                 {
-                    dataQueue.Enqueue(e.Data);
-
-                    ErrorUtilities.VerifyThrow(dataAvailableSignal != null,
-                        "The signalling event must be available.");
-
-                    // signal the availability of data
-                    // NOTE: intentionally, do the signalling inside the lock, because
-                    // ManualResetEvents don't have ref counts, and we want to make sure
-                    // we don't signal the notification just before the consumer resets it
                     lock (_eventCloseLock)
                     {
                         if (!_eventsDisposed)
                         {
-                            dataAvailableSignal.Set();
+                            eofEvent.Set();
                         }
                     }
                 }
+
+                return;
+            }
+
+            // NOTE: don't ignore empty string, because we need to log that
+            ErrorUtilities.VerifyThrow(dataQueue != null, "The data queue must be available.");
+
+            // synchronize access to the queue -- this is a producer-consumer problem
+            // NOTE: we lock the entire queue instead of using synchronized queue
+            // wrappers, because ManualResetEvents don't have ref counts, and it's
+            // difficult to discretely signal the availability of each instance of
+            // data in the queue -- so instead we let the consumer lock and empty
+            // the queue and reset the ManualResetEvent, before we add more data
+            // into the queue, and signal the ManualResetEvent again
+            lock (dataQueue.SyncRoot)
+            {
+                dataQueue.Enqueue(e.Data);
+
+                ErrorUtilities.VerifyThrow(dataAvailableSignal != null,
+                    "The signalling event must be available.");
+
+                // signal the availability of data
+                // NOTE: intentionally, do the signalling inside the lock, because
+                // ManualResetEvents don't have ref counts, and we want to make sure
+                // we don't signal the notification just before the consumer resets it
+                lock (_eventCloseLock)
+                {
+                    if (!_eventsDisposed)
+                    {
+                        dataAvailableSignal.Set();
+                    }
+                }
             }
         }
 
@@ -1770,6 +1822,18 @@ private bool LogEnvironmentVariable(bool alreadyLoggedEnvironmentHeader, string
         /// </summary>
         private bool _eventsDisposed;
 
+        /// <summary>
+        /// Signalled when the stdout AsyncStreamReader reaches EOF (sends Data=null).
+        /// Used by WaitForProcessExit to know when all stdout data has been delivered.
+        /// </summary>
+        private ManualResetEvent _standardOutputEOF;
+
+        /// <summary>
+        /// Signalled when the stderr AsyncStreamReader reaches EOF (sends Data=null).
+        /// Used by WaitForProcessExit to know when all stderr data has been delivered.
+        /// </summary>
+        private ManualResetEvent _standardErrorEOF;
+
         /// <summary>
         /// List of name, value pairs to be passed to the spawned tool's environment.
         /// May be null.