From 7e39d10d1f646ad52b31c78e2ceaf062a1186cf6 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Thu, 9 Jul 2026 12:50:13 +0200 Subject: [PATCH] Partially revert #13660: drop NuGet RestoreTask transient TaskHost workaround PR #13660 routed NuGet's RestoreTask to a transient (non-sidecar) TaskHost in /mt and MSBuild Server modes to avoid leaking NuGet's static singleton state across invocations. This removes that workaround so RestoreTask again follows normal TaskHost routing in all modes, while keeping the generic per-invocation TaskHost diagnostic logging (TaskHostDetails) introduced by the same PR. Removed: - TaskRouter.RequiresTransientTaskHost + RestoreTask allow-list entry - AssemblyTaskFactory forceTransientTaskHost branch (useSidecarTaskHost reverted) - BuildParameters.IsLongLivedHost / MarkProcessAsLongLivedHost infrastructure - OutOfProcServerNode.MarkProcessAsLongLivedHost() call - Associated TaskRouter integration tests and mock RestoreTask Kept: - TaskHostTask TaskHostDetails diagnostic logging - NodeProviderOutOfProcTaskHost.AcquireAndSetUpHost out-params (hostProcessId, wasNewlyCreated) and localized resource strings Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../BackEnd/TaskRouter_IntegrationTests.cs | 273 ------------------ .../BackEnd/BuildManager/BuildParameters.cs | 27 -- .../Components/RequestBuilder/TaskRouter.cs | 31 -- src/Build/BackEnd/Node/OutOfProcServerNode.cs | 8 - .../TaskFactories/AssemblyTaskFactory.cs | 21 +- 5 files changed, 1 insertion(+), 359 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/TaskRouter_IntegrationTests.cs b/src/Build.UnitTests/BackEnd/TaskRouter_IntegrationTests.cs index fa60eee0889..f7a6ec573fd 100644 --- a/src/Build.UnitTests/BackEnd/TaskRouter_IntegrationTests.cs +++ b/src/Build.UnitTests/BackEnd/TaskRouter_IntegrationTests.cs @@ -3,11 +3,8 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.IO; using System.Reflection; -using System.Text.RegularExpressions; -using Microsoft.Build.BackEnd; using Microsoft.Build.Execution; using Microsoft.Build.Framework; using Microsoft.Build.Shared; @@ -384,257 +381,6 @@ public void ExplicitTaskHostFactory_OverridesRoutingLogic() logger.FullLog.ShouldContain("TaskWithAttribute executed"); } - [Fact] - public void RequiresTransientTaskHost_ReturnsTrueForRestoreTask() - { - TaskRouter.RequiresTransientTaskHost(typeof(NuGet.Build.Tasks.RestoreTask)).ShouldBeTrue(); - } - - [Fact] - public void RequiresTransientTaskHost_ReturnsFalseForRegularTask() - { - TaskRouter.RequiresTransientTaskHost(typeof(NonEnlightenedTestTask)).ShouldBeFalse(); - TaskRouter.RequiresTransientTaskHost(typeof(AttributeTestTask)).ShouldBeFalse(); - } - - [Fact] - public void RequiresTransientTaskHost_RoutedToTaskHost_InMultiThreadedMode() - { - string projectContent = $@" - - - - - - -"; - - string projectFile = Path.Combine(_testProjectsDir, "RestoreTaskMT.proj"); - File.WriteAllText(projectFile, projectContent); - - var logger = new MockLogger(_output); - var buildParameters = new BuildParameters - { - MultiThreaded = true, - Loggers = [logger], - DisableInProcNode = false, - EnableNodeReuse = false, - }; - - var buildRequestData = new BuildRequestData( - projectFile, - new Dictionary(), - null, - ["TestTarget"], - null); - - BuildManager buildManager = BuildManager.DefaultBuildManager; - BuildResult result = buildManager.Build(buildParameters, buildRequestData); - - result.OverallResult.ShouldBe(BuildResultCode.Success); - TaskRouterTestHelper.AssertTaskUsedTaskHost(logger, "RestoreTask"); - logger.FullLog.ShouldContain("RestoreTask executed"); - } - - [Fact] - public void RequiresTransientTaskHost_RoutedToTaskHost_InServerMode() - { - string projectContent = $@" - - - - - - -"; - - string projectFile = Path.Combine(_testProjectsDir, "RestoreTaskServer.proj"); - File.WriteAllText(projectFile, projectContent); - - var logger = new MockLogger(_output); - var buildParameters = new BuildParameters - { - MultiThreaded = false, - Loggers = [logger], - DisableInProcNode = false, - EnableNodeReuse = false, - - // Simulate running under the MSBuild Server. In production this flag is - // set process-wide by OutOfProcServerNode via BuildParameters.MarkProcessAsLongLivedHost. - IsLongLivedHost = true, - }; - - var buildRequestData = new BuildRequestData( - projectFile, - new Dictionary(), - null, - ["TestTarget"], - null); - - BuildManager buildManager = BuildManager.DefaultBuildManager; - BuildResult result = buildManager.Build(buildParameters, buildRequestData); - - result.OverallResult.ShouldBe(BuildResultCode.Success); - TaskRouterTestHelper.AssertTaskUsedTaskHost(logger, "RestoreTask"); - logger.FullLog.ShouldContain("RestoreTask executed"); - } - - [Fact] - public void RequiresTransientTaskHost_GetsFreshProcess_OnEachInvocation_InMultiThreadedMode() - { - string projectContent = $@" - - - - - - -"; - - string projectFile = Path.Combine(_testProjectsDir, "RestoreTaskFreshProcess.proj"); - File.WriteAllText(projectFile, projectContent); - - var logger = new MockLogger(_output); - var buildParameters = new BuildParameters - { - MultiThreaded = true, - Loggers = [logger], - DisableInProcNode = false, - - // Reuse must stay ON so we test the workaround, not natural process death. - // With reuse off the test cannot distinguish "transient TaskHost (workaround)" - // from "sidecar TaskHost that happened to die between builds". - EnableNodeReuse = true, - }; - - var buildRequestData = new BuildRequestData( - projectFile, - new Dictionary(), - null, - ["TestTarget"], - null); - - // Two separate Build cycles. The workaround forces nodeReuse=false on the spawned - // TaskHost so it dies at EndBuild, giving the next Build a fresh process. - BuildManager buildManager = BuildManager.DefaultBuildManager; - BuildResult result1 = buildManager.Build(buildParameters, buildRequestData); - BuildResult result2 = buildManager.Build(buildParameters, buildRequestData); - - result1.OverallResult.ShouldBe(BuildResultCode.Success); - result2.OverallResult.ShouldBe(BuildResultCode.Success); - TaskRouterTestHelper.AssertTaskUsedTaskHost(logger, "RestoreTask"); - - int[] pids = ExtractReportedPids(logger.FullLog); - - pids.Length.ShouldBe(2, $"Expected two RestoreTask invocations to log a PID. Log:{Environment.NewLine}{logger.FullLog}"); - pids[0].ShouldNotBe(pids[1], "Each build must spawn a fresh TaskHost so NuGet static state cannot leak across builds."); - pids.ShouldNotContain(Process.GetCurrentProcess().Id, "TaskHost should be out-of-process from the test runner."); - } - - [Fact] - public void RequiresTransientTaskHost_GetsFreshProcess_OnEachInvocation_InServerMode() - { - string projectContent = $@" - - - - - - -"; - - string projectFile = Path.Combine(_testProjectsDir, "RestoreTaskFreshProcessServer.proj"); - File.WriteAllText(projectFile, projectContent); - - var logger = new MockLogger(_output); - var buildParameters = new BuildParameters - { - MultiThreaded = false, - Loggers = [logger], - DisableInProcNode = false, - - // Reuse must stay ON so we test the workaround, not natural process death. - EnableNodeReuse = true, - - // Simulate running under the MSBuild Server. - IsLongLivedHost = true, - }; - - var buildRequestData = new BuildRequestData( - projectFile, - new Dictionary(), - null, - ["TestTarget"], - null); - - // Two separate Build cycles. The workaround forces nodeReuse=false on the spawned - // TaskHost so it dies at EndBuild, giving the next Build a fresh process. - BuildManager buildManager = BuildManager.DefaultBuildManager; - BuildResult result1 = buildManager.Build(buildParameters, buildRequestData); - BuildResult result2 = buildManager.Build(buildParameters, buildRequestData); - - result1.OverallResult.ShouldBe(BuildResultCode.Success); - result2.OverallResult.ShouldBe(BuildResultCode.Success); - TaskRouterTestHelper.AssertTaskUsedTaskHost(logger, "RestoreTask"); - - int[] pids = ExtractReportedPids(logger.FullLog); - - pids.Length.ShouldBe(2, $"Expected two RestoreTask invocations to log a PID. Log:{Environment.NewLine}{logger.FullLog}"); - pids[0].ShouldNotBe(pids[1], "Each build must spawn a fresh TaskHost so NuGet static state cannot leak across builds."); - pids.ShouldNotContain(Process.GetCurrentProcess().Id, "TaskHost should be out-of-process from the test runner."); - } - - private static int[] ExtractReportedPids(string log) - { - var pids = new List(); - foreach (Match m in Regex.Matches(log, @"RestoreTask executed in PID=(\d+)")) - { - pids.Add(int.Parse(m.Groups[1].Value)); - } - - return pids.ToArray(); - } - - [Fact] - public void RequiresTransientTaskHost_RunsInProcess_WhenNoMTOrServer() - { - string projectContent = $@" - - - - - - -"; - - string projectFile = Path.Combine(_testProjectsDir, "RestoreTaskNoMT.proj"); - File.WriteAllText(projectFile, projectContent); - - var logger = new MockLogger(_output); - var buildParameters = new BuildParameters - { - MultiThreaded = false, - Loggers = [logger], - DisableInProcNode = false, - EnableNodeReuse = false, - }; - - var buildRequestData = new BuildRequestData( - projectFile, - new Dictionary(), - null, - ["TestTarget"], - null); - - BuildManager buildManager = BuildManager.DefaultBuildManager; - BuildResult result = buildManager.Build(buildParameters, buildRequestData); - - result.OverallResult.ShouldBe(BuildResultCode.Success); - - TaskRouterTestHelper.AssertTaskRanInProcess(logger, "RestoreTask"); - logger.FullLog.ShouldContain("RestoreTask executed"); - } - private string CreateTestProject(string taskName, string taskClass) { return $@" @@ -735,25 +481,6 @@ public override bool Execute() #endregion } -// Test task in the NuGet.Build.Tasks namespace to simulate the real RestoreTask for routing tests. -namespace NuGet.Build.Tasks -{ - /// - /// Simulates the NuGet RestoreTask for testing task routing workaround. - /// Has the same full name (NuGet.Build.Tasks.RestoreTask) that TaskRouter checks. - /// - public class RestoreTask : Task - { - public override bool Execute() - { - // Include the OS PID so tests can verify each invocation runs in a fresh - // TaskHost process - Log.LogMessage(MessageImportance.High, $"RestoreTask executed in PID={Process.GetCurrentProcess().Id}"); - return true; - } - } -} - // Custom attribute definition in Microsoft.Build.Framework namespace to match what TaskRouter expects // TaskRouter looks for attributes with FullName = "Microsoft.Build.Framework.MSBuildMultiThreadableTaskAttribute" // Since the real attribute is internal in Framework, we define our own test version here diff --git a/src/Build/BackEnd/BuildManager/BuildParameters.cs b/src/Build/BackEnd/BuildManager/BuildParameters.cs index 9565e8a884b..c0c0fcde2ce 100644 --- a/src/Build/BackEnd/BuildManager/BuildParameters.cs +++ b/src/Build/BackEnd/BuildManager/BuildParameters.cs @@ -64,12 +64,6 @@ public class BuildParameters : ITranslatable /// private static string s_startupDirectory = Environment.CurrentDirectory; - /// - /// Process-wide flag indicating that the engine is hosted in a long-lived process - /// (e.g., the MSBuild Server) that persists across multiple build invocations. - /// - private static bool s_isLongLivedHost; - /// /// Indicates whether we should warn when a property is uninitialized when it is used. /// @@ -334,7 +328,6 @@ internal BuildParameters(BuildParameters other, bool resetEnvironment = false) Question = other.Question; IsBuildCheckEnabled = other.IsBuildCheckEnabled; IsTelemetryEnabled = other.IsTelemetryEnabled; - IsLongLivedHost = other.IsLongLivedHost; ProjectCacheDescriptor = other.ProjectCacheDescriptor; _enableTargetOutputLogging = other.EnableTargetOutputLogging; } @@ -806,25 +799,6 @@ internal IToolsetProvider ToolsetProvider /// internal bool IsOutOfProc { get; set; } - /// - /// True when the engine is hosted in a long-lived process (e.g., MSBuild Server) - /// that persists across multiple build invocations. Used to opt tasks whose static - /// singleton state would leak across invocations out of sidecar TaskHost reuse. - /// See https://github.com/dotnet/msbuild/issues/13315. - /// - internal bool IsLongLivedHost - { - get => _isLongLivedHost; - set => _isLongLivedHost = value; - } - - private bool _isLongLivedHost = s_isLongLivedHost; - - /// - /// Marks the current process as a long-lived host. - /// - internal static void MarkProcessAsLongLivedHost() => s_isLongLivedHost = true; - /// public ProjectLoadSettings ProjectLoadSettings { @@ -1004,7 +978,6 @@ void ITranslatable.Translate(ITranslator translator) translator.Translate(ref _reportFileAccesses); translator.Translate(ref _enableTargetOutputLogging); translator.Translate(ref _multiThreaded); - translator.Translate(ref _isLongLivedHost); // ProjectRootElementCache is not transmitted. // ResetCaches is not transmitted. diff --git a/src/Build/BackEnd/Components/RequestBuilder/TaskRouter.cs b/src/Build/BackEnd/Components/RequestBuilder/TaskRouter.cs index 7831afe7f54..b0814b13fe1 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TaskRouter.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TaskRouter.cs @@ -83,37 +83,6 @@ private static bool HasMultiThreadableTaskAttribute(Type taskType) }); } - /// - /// Full name of a task whose static singleton state makes it unsafe to run in a - /// long-lived sidecar TaskHost (which persists across invocations). Such tasks must - /// instead run in an explicit (transient) TaskHost that terminates after execution, - /// ensuring static state is cleaned up. - /// This is a temporary workaround until the task authors fix their static state issues. - /// See https://github.com/dotnet/msbuild/issues/13315 - /// - private const string TaskRequiringTransientTaskHostFullName = "NuGet.Build.Tasks.RestoreTask"; - - /// - /// Determines if a task must be routed to an explicit (transient) TaskHost rather than - /// a reusable sidecar, because its static singleton state would leak across invocations. - /// Such tasks should run in a TaskHost that terminates after execution so all static - /// state is cleaned up. - /// - /// The type of the task to evaluate. - /// True if the task requires a transient TaskHost; false otherwise. - public static bool RequiresTransientTaskHost(Type taskType) - { - ArgumentNullException.ThrowIfNull(taskType); - - string? fullName = taskType.FullName; - if (fullName is null) - { - return false; - } - - return string.Equals(fullName, TaskRequiringTransientTaskHostFullName, StringComparison.Ordinal); - } - /// /// Clears the thread-safety cache. Used primarily for testing. /// diff --git a/src/Build/BackEnd/Node/OutOfProcServerNode.cs b/src/Build/BackEnd/Node/OutOfProcServerNode.cs index 0c8ae60bd10..01708c2a34c 100644 --- a/src/Build/BackEnd/Node/OutOfProcServerNode.cs +++ b/src/Build/BackEnd/Node/OutOfProcServerNode.cs @@ -108,13 +108,6 @@ public NodeEngineShutdownReason Run(out Exception? shutdownException) return NodeEngineShutdownReason.Error; } - // Mark the process as a long-lived host so per-build BuildParameters instances - // inherit IsLongLivedHost = true. This drives the workaround that routes tasks - // whose static state would leak across invocations (e.g., NuGet RestoreTask) to - // a transient TaskHost instead of a reusable sidecar. - // See https://github.com/dotnet/msbuild/issues/13315. - BuildParameters.MarkProcessAsLongLivedHost(); - while (true) { NodeEngineShutdownReason shutdownReason = RunInternal(out shutdownException, handshake); @@ -388,7 +381,6 @@ private void HandleServerNodeBuildCommand(ServerNodeBuildCommand command) Directory.SetCurrentDirectory(command.StartupDirectory); CommunicationsUtilities.SetEnvironment(command.BuildProcessEnvironment); - Traits.UpdateFromEnvironment(); Thread.CurrentThread.CurrentCulture = command.Culture; diff --git a/src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs b/src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs index 9ad3591fbd0..b64c63ee083 100644 --- a/src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs +++ b/src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs @@ -343,23 +343,6 @@ internal ITask CreateTaskInstance( } } - // Workaround for tasks whose static singleton state would leak across invocations - // (e.g., NuGet RestoreTask). In MT mode or when MSBuild server is active, these tasks - // must run in a transient (non-sidecar) TaskHost so static state is cleaned up after - // each invocation. See https://github.com/dotnet/msbuild/issues/13315 - bool forceTransientTaskHost = false; - if (_loadedType?.Type != null && TaskRouter.RequiresTransientTaskHost(_loadedType.Type)) - { - bool isMultiThreaded = buildComponentHost?.BuildParameters?.MultiThreaded == true; - bool isLongLivedHost = buildComponentHost?.BuildParameters?.IsLongLivedHost == true; - - if (isMultiThreaded || isLongLivedHost) - { - useTaskFactory = true; - forceTransientTaskHost = true; - } - } - taskLoggingContext?.TargetLoggingContext?.ProjectLoggingContext?.ProjectTelemetry?.AddTaskExecution(GetType().FullName, isTaskHost: useTaskFactory); if (useTaskFactory) @@ -374,9 +357,7 @@ internal ITask CreateTaskInstance( // If the task host factory is explicitly requested, do not act as a sidecar task host. // This is important as customers use task host factories for short lived tasks to release // potential locks. - // Also disable sidecar for tasks that require a transient TaskHost so their - // static state is cleaned up between invocations. - bool useSidecarTaskHost = !forceTransientTaskHost && !(_factoryIdentityParameters.TaskHostFactoryExplicitlyRequested ?? false); + bool useSidecarTaskHost = !(_factoryIdentityParameters.TaskHostFactoryExplicitlyRequested ?? false); TaskHostTask task = new( taskLocation,