From 66313477ef2782925823b2dc7a475681db5f3d3a Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Mon, 9 Mar 2026 13:36:21 +0100 Subject: [PATCH 1/2] Fix RequestCores fallback: throw NotImplementedException, don't log error The RequestCores/ReleaseCores fallback paths in OutOfProcTaskHostNode (when CallbacksSupported is false or under CLR2) were logging a build error via LogErrorFromResource and returning 0. This caused build failures for any task calling RequestCores in the OOP TaskHost because: 1. LogErrorFromResource logs a BuildErrorEventArgs which makes the build fail - but RequestCores is advisory, not fatal. 2. Returning 0 violates the IBuildEngine9.RequestCores contract which guarantees return value in [1, requestedCores]. All known callers (MonoAOTCompiler, EmccCompile, ILStrip, EmitBundleBase) catch NotImplementedException from the old code path and fall back to their own parallelism estimate. The new code no longer threw, so the catch blocks didn't fire, and the error event silently poisoned the build. Fix: - Restore throw NotImplementedException in fallback paths to match pre-PR behavior and what all callers expect - Fix TaskHostTask.HandleCoresRequest to default grantedCores to 1 (not 0) when the build engine doesn't support IBuildEngine9 Fixes #13333 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../BackEnd/RequestCoresWithFallbackTask.cs | 80 +++++++++++++++++++ .../BackEnd/TaskHostCallback_Tests.cs | 78 +++++++++++++++--- .../Instance/TaskFactories/TaskHostTask.cs | 4 +- src/MSBuild/OutOfProcTaskHostNode.cs | 17 ++-- 4 files changed, 160 insertions(+), 19 deletions(-) create mode 100644 src/Build.UnitTests/BackEnd/RequestCoresWithFallbackTask.cs diff --git a/src/Build.UnitTests/BackEnd/RequestCoresWithFallbackTask.cs b/src/Build.UnitTests/BackEnd/RequestCoresWithFallbackTask.cs new file mode 100644 index 00000000000..17abe286304 --- /dev/null +++ b/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 +{ + /// + /// 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 + /// + public class RequestCoresWithFallbackTask : Task + { + /// + /// Number of cores to request. Defaults to 1. + /// + public int CoreCount { get; set; } = 1; + + /// + /// Whether to call ReleaseCores after the parallel work (via be9?.ReleaseCores pattern). + /// + public bool ReleaseAfter { get; set; } + + /// + /// Number of cores granted (either from RequestCores or the fallback value). + /// + [Output] + public int GrantedCores { get; set; } + + /// + /// True if NotImplementedException was caught and the fallback was used. + /// + [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; + } + } +} diff --git a/src/Build.UnitTests/BackEnd/TaskHostCallback_Tests.cs b/src/Build.UnitTests/BackEnd/TaskHostCallback_Tests.cs index 301e8c57a52..ab3cd481f87 100644 --- a/src/Build.UnitTests/BackEnd/TaskHostCallback_Tests.cs +++ b/src/Build.UnitTests/BackEnd/TaskHostCallback_Tests.cs @@ -248,21 +248,28 @@ public void RequestCores_WorksWhenAutoEjectedInMultiThreadedMode() } /// - /// 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. /// [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 = $@" - + - <{nameof(RequestCoresTask)} CoreCount=""2""> - - + <{nameof(RequestCoresWithFallbackTask)} CoreCount=""4""> + + + "; @@ -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"); + } + + /// + /// 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). + /// + [Fact] + public void RequestAndReleaseCores_FallbackSkipsReleaseWhenCallbacksNotSupported() + { + using TestEnvironment env = TestEnvironment.Create(_output); + + // Explicitly do NOT set MSBUILDENABLETASKHOSTCALLBACKS — callbacks should be disabled + string projectContents = $@" + + + + <{nameof(RequestCoresWithFallbackTask)} CoreCount=""4"" ReleaseAfter=""true""> + + + + +"; + + 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("); } } } diff --git a/src/Build/Instance/TaskFactories/TaskHostTask.cs b/src/Build/Instance/TaskFactories/TaskHostTask.cs index 4a9dfdd5180..62729224986 100644 --- a/src/Build/Instance/TaskFactories/TaskHostTask.cs +++ b/src/Build/Instance/TaskFactories/TaskHostTask.cs @@ -675,7 +675,9 @@ private void HandleIsRunningMultipleNodesRequest(TaskHostIsRunningMultipleNodesR /// 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) { diff --git a/src/MSBuild/OutOfProcTaskHostNode.cs b/src/MSBuild/OutOfProcTaskHostNode.cs index 75716b705a7..d71951efd26 100644 --- a/src/MSBuild/OutOfProcTaskHostNode.cs +++ b/src/MSBuild/OutOfProcTaskHostNode.cs @@ -565,15 +565,17 @@ public IReadOnlyDictionary GetGlobalProperties() public int RequestCores(int requestedCores) { #if CLR2COMPATIBILITY - LogErrorFromResource("BuildEngineCallbacksInTaskHostUnsupported"); - return 0; + // CLR2 task host doesn't support resource management. + // Callers catch NotImplementedException and fall back to their own parallelism estimate. + 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); From a82472a22965f40fbb0ea8ddfaa0943f4d1a7ab0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Tue, 10 Mar 2026 10:40:50 +0100 Subject: [PATCH 2/2] Update src/MSBuild/OutOfProcTaskHostNode.cs Co-authored-by: Rainer Sigwald --- src/MSBuild/OutOfProcTaskHostNode.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/MSBuild/OutOfProcTaskHostNode.cs b/src/MSBuild/OutOfProcTaskHostNode.cs index d71951efd26..cc3e18e6525 100644 --- a/src/MSBuild/OutOfProcTaskHostNode.cs +++ b/src/MSBuild/OutOfProcTaskHostNode.cs @@ -566,7 +566,7 @@ public int RequestCores(int requestedCores) { #if CLR2COMPATIBILITY // CLR2 task host doesn't support resource management. - // Callers catch NotImplementedException and fall back to their own parallelism estimate. + // If they somehow get here, throw. throw new NotImplementedException(); #else ErrorUtilities.VerifyThrowArgumentOutOfRange(requestedCores > 0, nameof(requestedCores));