[vs18.8] Replace ToolTask #13351 revert with STA-safe EOF fix (#13917)

documentation/wiki/ChangeWaves.md

@@ -41,6 +41,7 @@ Change wave checks around features will be removed in the release that accompani
 - [AbsolutePath.GetCanonicalForm optimization - avoid expensive Path.GetFullPath calls when paths don't need canonicalization](https://github.com/dotnet/msbuild/pull/13369)
 - [TaskHostTask forwards request-level global properties (e.g. MSBuildRestoreSessionId) to out-of-proc TaskHost in -mt mode](https://github.com/dotnet/msbuild/pull/13443)
 - [Fix ShouldTreatWarningAsError in OOP TaskHost checking wrong collection (WarningsAsMessages instead of WarningsAsErrors)](https://github.com/dotnet/msbuild/issues/11952)
+- [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)

eng/Versions.props

@@ -1,9 +1,9 @@
-<Project>
+<Project>
 
   <Import Project="Version.Details.props" />
 
   <PropertyGroup>
-    <VersionPrefix>18.8.2</VersionPrefix><DotNetFinalVersionKind>release</DotNetFinalVersionKind><!-- Keep next to VersionPrefix to create a conflict in forward-flow -->
+    <VersionPrefix>18.8.3</VersionPrefix><DotNetFinalVersionKind>release</DotNetFinalVersionKind><!-- Keep next to VersionPrefix to create a conflict in forward-flow -->
     <PreReleaseVersionLabel>servicing</PreReleaseVersionLabel>
     <PackageValidationBaselineVersion>18.7.0-preview-26230-02</PackageValidationBaselineVersion>
     <AssemblyVersion>15.1.0.0</AssemblyVersion>

src/Utilities.UnitTests/ToolTask_Tests.cs

@@ -1112,6 +1112,72 @@ public void ToolTaskThatTimeoutAndRetry(int repeats, bool timeoutOnFirstExecutio
             }
         }
 
+        /// <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 30s EOF timeout.
+                t.MockCommandLineCommands = NativeMethodsShared.IsWindows
+                    ? "/c echo hello & start /b ping -n 40 127.0.0.1 > nul"
+                    : "-c \"echo hello; sleep 40 &\"";
+
+                // Outer task timeout is generous; the EOF timeout (30s) is what bounds us.
+                t.Timeout = 60000;
+
+                var sw = Stopwatch.StartNew();
+                bool result = t.Execute();
+                sw.Stop();
+
+                _output.WriteLine(engine.Log);
+
+                engine.Log.ShouldContain("hello");
+                // The task must return within ~30s (EOF timeout) even though the grandchild lives longer.
+                sw.Elapsed.TotalSeconds.ShouldBeLessThan(35, "ToolTask should be bounded by the 30s EOF timeout, not the grandchild's lifetime");
+                // The diagnostic message must appear so CI reports show why the wait ended.
+                engine.Log.ShouldContain("Pipe EOF not received");
+            }
+        }
+
+        /// <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/Resources/Strings.resx

@@ -168,6 +168,9 @@
   <data name="ToolTask.EnvironmentVariableHeader">
     <value>Environment Variables passed to tool:</value>
   </data>
+  <data name="ToolTask.PipeEOFTimeout" xml:space="preserve">
+    <value>Pipe EOF not received within {0} seconds. A grandchild process may still be holding the pipe open. Output already delivered has been logged.</value>
+  </data>
   <data name="ToolTask.ValidateParametersFailed">
     <value>MSB6011: Invalid parameters passed to the {0} task.</value>
     <comment>{StrBegin="MSB6011: "}</comment>