Fix RequestCores/ReleaseCores fallback in OOP TaskHost: throw NotImplementedException instead of logging error (#13345)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: JanProvaznik

src/Build.UnitTests/BackEnd/RequestCoresWithFallbackTask.cs

@@ -0,0 +1,80 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using Microsoft.Build.Framework;
+using Microsoft.Build.Utilities;
+
+namespace Microsoft.Build.UnitTests.BackEnd
+{
+    /// <summary>
+    /// A test task that calls IBuildEngine9.RequestCores with the same fallback pattern
+    /// used by real callers (MonoAOTCompiler, EmccCompile, ILStrip, EmitBundleBase):
+    ///   try { cores = be9.RequestCores(N); }
+    ///   catch (NotImplementedException) { be9 = null; /* use own estimate */ }
+    ///   finally { be9?.ReleaseCores(cores); }
+    /// Used by regression tests for https://github.com/dotnet/msbuild/issues/13333
+    /// </summary>
+    public class RequestCoresWithFallbackTask : Task
+    {
+        /// <summary>
+        /// Number of cores to request. Defaults to 1.
+        /// </summary>
+        public int CoreCount { get; set; } = 1;
+
+        /// <summary>
+        /// Whether to call ReleaseCores after the parallel work (via be9?.ReleaseCores pattern).
+        /// </summary>
+        public bool ReleaseAfter { get; set; }
+
+        /// <summary>
+        /// Number of cores granted (either from RequestCores or the fallback value).
+        /// </summary>
+        [Output]
+        public int GrantedCores { get; set; }
+
+        /// <summary>
+        /// True if NotImplementedException was caught and the fallback was used.
+        /// </summary>
+        [Output]
+        public bool UsedFallback { get; set; }
+
+        public override bool Execute()
+        {
+            int allowedParallelism = CoreCount;
+            IBuildEngine9? be9 = BuildEngine as IBuildEngine9;
+            try
+            {
+                if (be9 is not null)
+                {
+                    allowedParallelism = be9.RequestCores(CoreCount);
+                }
+            }
+            catch (NotImplementedException)
+            {
+                // This is the expected path when callbacks are not supported.
+                // Real callers null out be9 so ReleaseCores is skipped.
+                Log.LogMessage(MessageImportance.High, "RequestCores threw NotImplementedException, using fallback");
+                be9 = null;
+                UsedFallback = true;
+            }
+
+            GrantedCores = allowedParallelism;
+            Log.LogMessage(MessageImportance.High, $"GrantedCores = {GrantedCores}");
+
+            // Simulate parallel work...
+
+            if (ReleaseAfter)
+            {
+                // Real callers use be9?.ReleaseCores — skipped when be9 was nulled in catch.
+                if (be9 is not null)
+                {
+                    be9.ReleaseCores(allowedParallelism);
+                    Log.LogMessage(MessageImportance.High, $"ReleaseCores({allowedParallelism}) completed");
+                }
+            }
+
+            return true;
+        }
+    }
+}

src/Build.UnitTests/BackEnd/TaskHostCallback_Tests.cs

@@ -248,21 +248,28 @@ public void RequestCores_WorksWhenAutoEjectedInMultiThreadedMode()
         }
 
         /// <summary>
-        /// Verifies that RequestCores when callbacks are disabled logs error MSB5022 and returns 0.
+        /// Regression test for https://github.com/dotnet/msbuild/issues/13333
+        /// When callbacks are not supported (cross-version OOP TaskHost), RequestCores must
+        /// throw NotImplementedException (not log a build error and return 0).
+        /// Real callers (MonoAOTCompiler, EmccCompile, ILStrip, EmitBundleBase) catch this
+        /// exception and fall back to their own parallelism estimate. The previous behavior
+        /// of logging BuildErrorEventArgs caused the build to fail silently.
         /// </summary>
         [Fact]
-        public void RequestCores_LogsErrorWhenCallbacksNotSupported()
+        public void RequestCores_ThrowsNotImplementedWhenCallbacksNotSupported()
         {
             using TestEnvironment env = TestEnvironment.Create(_output);
 
-            // Explicitly do NOT set MSBUILDENABLETASKHOSTCALLBACKS
+            // Explicitly do NOT set MSBUILDENABLETASKHOSTCALLBACKS — callbacks should be disabled.
+            // Use RequestCoresWithFallbackTask which catches NotImplementedException like real callers do.
             string projectContents = $@"
 <Project>
-    <UsingTask TaskName=""{nameof(RequestCoresTask)}"" AssemblyFile=""{typeof(RequestCoresTask).Assembly.Location}"" TaskFactory=""TaskHostFactory"" />
+    <UsingTask TaskName=""{nameof(RequestCoresWithFallbackTask)}"" AssemblyFile=""{typeof(RequestCoresWithFallbackTask).Assembly.Location}"" TaskFactory=""TaskHostFactory"" />
     <Target Name=""Test"">
-        <{nameof(RequestCoresTask)} CoreCount=""2"">
-            <Output PropertyName=""Result"" TaskParameter=""GrantedCores"" />
-        </{nameof(RequestCoresTask)}>
+        <{nameof(RequestCoresWithFallbackTask)} CoreCount=""4"">
+            <Output PropertyName=""GrantedResult"" TaskParameter=""GrantedCores"" />
+            <Output PropertyName=""FellBack"" TaskParameter=""UsedFallback"" />
+        </{nameof(RequestCoresWithFallbackTask)}>
     </Target>
 </Project>";
 
@@ -274,9 +281,60 @@ public void RequestCores_LogsErrorWhenCallbacksNotSupported()
                 new BuildParameters { MaxNodeCount = 4, EnableNodeReuse = false, Loggers = [logger] },
                 new BuildRequestData(projectInstance, targetsToBuild: ["Test"]));
 
-            // MSB5022 error should be logged — the callback was not forwarded
-            logger.ErrorCount.ShouldBeGreaterThan(0);
-            logger.FullLog.ShouldContain("MSB5022");
+            // Build must succeed — the task catches NotImplementedException and falls back.
+            buildResult.OverallResult.ShouldBe(BuildResultCode.Success);
+
+            // No errors should be logged — NotImplementedException is caught by the task, not by MSBuild.
+            logger.ErrorCount.ShouldBe(0);
+
+            // The task should have used its fallback path (NotImplementedException was thrown).
+            logger.FullLog.ShouldContain("RequestCores threw NotImplementedException, using fallback");
+
+            // GrantedCores should be the task's own fallback (CoreCount), not 0.
+            logger.FullLog.ShouldContain("GrantedCores = 4");
+        }
+
+        /// <summary>
+        /// Regression test for https://github.com/dotnet/msbuild/issues/13333
+        /// When callbacks are not supported, the full caller pattern (RequestCores with catch,
+        /// then skip ReleaseCores) must work. This matches MonoAOTCompiler/EmccCompile/ILStrip:
+        ///   try { cores = be9.RequestCores(N); }
+        ///   catch (NotImplementedException) { be9 = null; }
+        ///   finally { be9?.ReleaseCores(cores); }
+        /// ReleaseCores must NOT be called when the fallback fires (be9 is nulled).
+        /// </summary>
+        [Fact]
+        public void RequestAndReleaseCores_FallbackSkipsReleaseWhenCallbacksNotSupported()
+        {
+            using TestEnvironment env = TestEnvironment.Create(_output);
+
+            // Explicitly do NOT set MSBUILDENABLETASKHOSTCALLBACKS — callbacks should be disabled
+            string projectContents = $@"
+<Project>
+    <UsingTask TaskName=""{nameof(RequestCoresWithFallbackTask)}"" AssemblyFile=""{typeof(RequestCoresWithFallbackTask).Assembly.Location}"" TaskFactory=""TaskHostFactory"" />
+    <Target Name=""Test"">
+        <{nameof(RequestCoresWithFallbackTask)} CoreCount=""4"" ReleaseAfter=""true"">
+            <Output PropertyName=""GrantedResult"" TaskParameter=""GrantedCores"" />
+            <Output PropertyName=""FellBack"" TaskParameter=""UsedFallback"" />
+        </{nameof(RequestCoresWithFallbackTask)}>
+    </Target>
+</Project>";
+
+            TransientTestProjectWithFiles project = env.CreateTestProjectWithFiles(projectContents);
+            ProjectInstance projectInstance = new(project.ProjectFile);
+
+            var logger = new MockLogger(_output);
+            BuildResult buildResult = BuildManager.DefaultBuildManager.Build(
+                new BuildParameters { MaxNodeCount = 4, EnableNodeReuse = false, Loggers = [logger] },
+                new BuildRequestData(projectInstance, targetsToBuild: ["Test"]));
+
+            // Build must succeed.
+            buildResult.OverallResult.ShouldBe(BuildResultCode.Success);
+            logger.ErrorCount.ShouldBe(0);
+
+            // Fallback fired — ReleaseCores should have been skipped (be9 nulled in catch).
+            logger.FullLog.ShouldContain("RequestCores threw NotImplementedException, using fallback");
+            logger.FullLog.ShouldNotContain("ReleaseCores(");
         }
     }
 }

src/Build/Instance/TaskFactories/TaskHostTask.cs

@@ -675,7 +675,9 @@ private void HandleIsRunningMultipleNodesRequest(TaskHostIsRunningMultipleNodesR
         /// </summary>
         private void HandleCoresRequest(TaskHostCoresRequest request)
         {
-            int grantedCores = 0;
+            // Default to 1 for RequestCores (not 0) to satisfy the API contract (return ∈ [1, requested]).
+            // For ReleaseCores, 0 is correct as it's just an acknowledgment.
+            int grantedCores = request.IsRelease ? 0 : 1;
 
             if (request.IsRelease)
             {

src/MSBuild/OutOfProcTaskHostNode.cs

@@ -565,15 +565,17 @@ public IReadOnlyDictionary<string, string> GetGlobalProperties()
         public int RequestCores(int requestedCores)
         {
 #if CLR2COMPATIBILITY
-            LogErrorFromResource("BuildEngineCallbacksInTaskHostUnsupported");
-            return 0;
+            // CLR2 task host doesn't support resource management.
+            // If they somehow get here, throw.
+            throw new NotImplementedException();
 #else
             ErrorUtilities.VerifyThrowArgumentOutOfRange(requestedCores > 0, nameof(requestedCores));
 
             if (!CallbacksSupported)
             {
-                LogErrorFromResource("BuildEngineCallbacksInTaskHostUnsupported");
-                return 0;
+                // Callbacks not available (cross-version scenario). Throw so callers' existing
+                // catch (NotImplementedException) blocks fire and fall back gracefully.
+                throw new NotImplementedException();
             }
 
             var request = new TaskHostCoresRequest(requestedCores, isRelease: false);
@@ -585,15 +587,14 @@ public int RequestCores(int requestedCores)
         public void ReleaseCores(int coresToRelease)
         {
 #if CLR2COMPATIBILITY
-            LogErrorFromResource("BuildEngineCallbacksInTaskHostUnsupported");
-            return;
+            // CLR2 task host doesn't support resource management.
+            throw new NotImplementedException();
 #else
             ErrorUtilities.VerifyThrowArgumentOutOfRange(coresToRelease > 0, nameof(coresToRelease));
 
             if (!CallbacksSupported)
             {
-                LogErrorFromResource("BuildEngineCallbacksInTaskHostUnsupported");
-                return;
+                throw new NotImplementedException();
             }
 
             var request = new TaskHostCoresRequest(coresToRelease, isRelease: true);