Ensure buildcheck lifetime per build (#10649)

src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs

@@ -283,8 +283,11 @@ public void CleanupForBuild()
                         throw new AggregateException(deactivateExceptions);
                     }
 
-                    var buildCheckManager = (_componentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider)!.Instance;
+                    IBuildCheckManagerProvider buildCheckProvider = (_componentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider);
+                    var buildCheckManager = buildCheckProvider!.Instance;
                     buildCheckManager.FinalizeProcessing(_nodeLoggingContext);
+                    // Clears the instance so that next call (on node reuse) to 'GetComponent' leads to reinitialization.
+                    buildCheckProvider.ShutdownComponent();
                 },
                 isLastTask: true);
 

src/Build/BackEnd/Components/Logging/LoggingService.cs

@@ -854,6 +854,8 @@ public void InitializeComponent(IBuildComponentHost buildComponentHost)
                 _onlyLogCriticalEvents = buildComponentHost.BuildParameters.OnlyLogCriticalEvents;
 
                 _serviceState = LoggingServiceState.Initialized;
+
+                _buildEngineDataRouter = (buildComponentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider)?.BuildEngineDataRouter;
             }
         }
 

src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs

@@ -798,13 +798,17 @@ public void LogIncludeFile(BuildEventContext buildEventContext, string filePath)
 
         #endregion
 
+#nullable enable
+        private IBuildEngineDataRouter? _buildEngineDataRouter;
+
         public void ProcessPropertyRead(PropertyReadInfo propertyReadInfo, CheckLoggingContext checkContext)
-            => BuildCheckManagerProvider.GlobalBuildEngineDataRouter?.ProcessPropertyRead(propertyReadInfo, checkContext);
+            => _buildEngineDataRouter?.ProcessPropertyRead(propertyReadInfo, checkContext);
 
         public void ProcessPropertyWrite(PropertyWriteInfo propertyWriteInfo, CheckLoggingContext checkContext)
-            => BuildCheckManagerProvider.GlobalBuildEngineDataRouter?.ProcessPropertyWrite(propertyWriteInfo, checkContext);
+            => _buildEngineDataRouter?.ProcessPropertyWrite(propertyWriteInfo, checkContext);
 
         public void ProcessProjectEvaluationStarted(ICheckContext checkContext, string projectFullPath)
-            => BuildCheckManagerProvider.GlobalBuildEngineDataRouter?.ProcessProjectEvaluationStarted(checkContext, projectFullPath);
+            => _buildEngineDataRouter?.ProcessProjectEvaluationStarted(checkContext, projectFullPath);
+#nullable disable
     }
 }

src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs

@@ -26,15 +26,11 @@ namespace Microsoft.Build.Experimental.BuildCheck.Infrastructure;
 /// </summary>
 internal sealed class BuildCheckManagerProvider : IBuildCheckManagerProvider
 {
-    private static IBuildCheckManager? s_globalInstance;
+    private IBuildCheckManager? _instance;
 
-    internal static IBuildCheckManager GlobalInstance => s_globalInstance ?? throw new InvalidOperationException("BuildCheckManagerProvider not initialized");
+    public IBuildCheckManager Instance => _instance ?? new NullBuildCheckManager();
 
-    public IBuildCheckManager Instance => GlobalInstance;
-
-    public IBuildEngineDataRouter BuildEngineDataRouter => (IBuildEngineDataRouter)GlobalInstance;
-
-    public static IBuildEngineDataRouter? GlobalBuildEngineDataRouter => (IBuildEngineDataRouter?)s_globalInstance;
+    public IBuildEngineDataRouter BuildEngineDataRouter => (IBuildEngineDataRouter)Instance;
 
     internal static IBuildComponent CreateComponent(BuildComponentType type)
     {
@@ -46,25 +42,24 @@ public void InitializeComponent(IBuildComponentHost host)
     {
         ErrorUtilities.VerifyThrow(host != null, "BuildComponentHost was null");
 
-        if (s_globalInstance == null)
+        if (_instance == null)
         {
-            IBuildCheckManager instance;
             if (host!.BuildParameters.IsBuildCheckEnabled)
             {
-                instance = new BuildCheckManager();
+                _instance = new BuildCheckManager();
             }
             else
             {
-                instance = new NullBuildCheckManager();
+                _instance = new NullBuildCheckManager();
             }
-
-            // We are fine with the possibility of double creation here - as the construction is cheap
-            //  and without side effects and the actual backing field is effectively immutable after the first assignment.
-            Interlocked.CompareExchange(ref s_globalInstance, instance, null);
         }
     }
 
-    public void ShutdownComponent() => GlobalInstance.Shutdown();
+    public void ShutdownComponent()
+    {
+        _instance?.Shutdown();
+        _instance = null;
+    } 
 
     internal sealed class BuildCheckManager : IBuildCheckManager, IBuildEngineDataRouter
     {

src/BuildCheck.UnitTests/EndToEndTests.cs

@@ -45,6 +45,7 @@ public void PropertiesUsageAnalyzerTest(bool buildInOutOfProcessNode)
         PrepareSampleProjectsAndConfig(
             buildInOutOfProcessNode,
             out TransientTestFile projectFile,
+            out _,
             "PropsCheckTest.csproj");
 
         string output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path} -check", out bool success);
@@ -62,6 +63,65 @@ public void PropertiesUsageAnalyzerTest(bool buildInOutOfProcessNode)
         Regex.Matches(output, "BC0203 .* Property").Count.ShouldBe(2);
     }
 
+    [Fact]
+    public void ConfigChangeReflectedOnReuse()
+    {
+        PrepareSampleProjectsAndConfig(
+            // we need out of proc build - to test node reuse
+            true,
+            out TransientTestFile projectFile,
+            out TransientTestFile editorconfigFile,
+            "PropsCheckTest.csproj");
+
+        // Build without BuildCheck - no findings should be reported
+        string output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path}", out bool success);
+        _env.Output.WriteLine(output);
+        _env.Output.WriteLine("=========================");
+        success.ShouldBeTrue(output);
+        output.ShouldNotContain("BC0201");
+        output.ShouldNotContain("BC0202");
+        output.ShouldNotContain("BC0203");
+
+        // Build with BuildCheck - findings should be reported
+        output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path} -check", out success);
+        _env.Output.WriteLine(output);
+        _env.Output.WriteLine("=========================");
+        success.ShouldBeTrue(output);
+        output.ShouldContain("warning BC0201");
+        output.ShouldContain("warning BC0202");
+        output.ShouldContain("warning BC0203");
+
+        // Flip config in editorconfig
+        string editorConfigChange = """
+
+                                    build_check.BC0201.Severity=error
+                                    build_check.BC0202.Severity=error
+                                    build_check.BC0203.Severity=error
+                                    """;
+
+        File.AppendAllText(editorconfigFile.Path, editorConfigChange);
+
+        // Build with BuildCheck - findings with new severity should be reported
+        output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path} -check", out success);
+        _env.Output.WriteLine(output);
+        _env.Output.WriteLine("=========================");
+        // build should fail due to error checks
+        success.ShouldBeFalse(output);
+        output.ShouldContain("error BC0201");
+        output.ShouldContain("error BC0202");
+        output.ShouldContain("error BC0203");
+
+        // Build without BuildCheck - no findings should be reported
+        output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path}", out success);
+        _env.Output.WriteLine(output);
+        _env.Output.WriteLine("=========================");
+        success.ShouldBeTrue(output);
+        output.ShouldNotContain("BC0201");
+        output.ShouldNotContain("BC0202");
+        output.ShouldNotContain("BC0203");
+    }
+
+
     [Theory]
     [InlineData(true, true)]
     [InlineData(false, true)]
@@ -467,6 +527,7 @@ private void PopulateXmlAttribute(XmlDocument doc, XmlNode node, string attribut
     private void PrepareSampleProjectsAndConfig(
         bool buildInOutOfProcessNode,
         out TransientTestFile projectFile,
+        out TransientTestFile editorconfigFile,
         string entryProjectAssetName,
         IEnumerable<string>? supplementalAssetNames = null,
         IEnumerable<(string RuleId, string Severity)>? ruleToSeverity = null,
@@ -485,7 +546,7 @@ private void PrepareSampleProjectsAndConfig(
             TransientTestFile supplementalFile = _env.CreateFile(workFolder, supplementalAssetName, supplementalContent);
         }
 
-        _env.CreateFile(workFolder, ".editorconfig", ReadEditorConfig(ruleToSeverity, ruleToCustomConfig, testAssetsFolderName));
+        editorconfigFile = _env.CreateFile(workFolder, ".editorconfig", ReadEditorConfig(ruleToSeverity, ruleToCustomConfig, testAssetsFolderName));
 
         // OSX links /var into /private, which makes Path.GetTempPath() return "/var..." but Directory.GetCurrentDirectory return "/private/var...".
         // This discrepancy breaks path equality checks in MSBuild checks if we pass to MSBuild full path to the initial project.
@@ -514,6 +575,7 @@ private void PrepareSampleProjectsAndConfig(
         => PrepareSampleProjectsAndConfig(
             buildInOutOfProcessNode,
             out projectFile,
+            out _,
             "Project1.csproj",
             new[] { "Project2.csproj", "ImportedFile1.props" },
             ruleToSeverity,