Ensure we pass unique binlog paths to each BuildHost (#78599)

Otherwise if we launch multiple processes, they might step atop each other and cause locking issues.

Author: jasonmalinowski

SpellingExclusions.dic

@@ -1,7 +1,6 @@
-stackalloc
+stackalloc
 awaitable
-Refactorings
-Infos
-cref
-binlog
-Namer
+Refactorings
+Infos
+cref
+binlog

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/BinlogNamer.cs renamed to src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/BinLogPathProvider.cs

@@ -4,14 +4,15 @@
 
 using System.Composition;
 using Microsoft.CodeAnalysis.Host.Mef;
+using Microsoft.CodeAnalysis.MSBuild;
 using Microsoft.CodeAnalysis.Options;
 using Microsoft.Extensions.Logging;
 using Microsoft.VisualStudio.Composition;
 
 namespace Microsoft.CodeAnalysis.LanguageServer.HostWorkspace;
 
-[Export(typeof(BinlogNamer)), Shared]
-internal sealed class BinlogNamer
+[Export(typeof(IBinLogPathProvider)), Shared]
+internal sealed class BinLogPathProvider : IBinLogPathProvider
 {
     /// <summary>
     /// The suffix to use for the binary log name; incremented each time we have a new build. Should be incremented with <see cref="Interlocked.Increment(ref int)"/>.
@@ -28,13 +29,13 @@ internal sealed class BinlogNamer
 
     [ImportingConstructor]
     [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
-    public BinlogNamer(IGlobalOptionService globalOptionService, ILoggerFactory loggerFactory)
+    public BinLogPathProvider(IGlobalOptionService globalOptionService, ILoggerFactory loggerFactory)
     {
         _globalOptionService = globalOptionService;
-        _logger = loggerFactory.CreateLogger<BinlogNamer>();
+        _logger = loggerFactory.CreateLogger<BinLogPathProvider>();
     }
 
-    internal string? GetMSBuildBinaryLogPath()
+    public string? GetNewLogPath()
     {
         if (_globalOptionService.GetOption(LanguageServerProjectSystemOptionsStorage.BinaryLogPath) is not string binaryLogDirectory)
             return null;

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileBasedProgramsProjectSystem.cs

@@ -41,7 +41,7 @@ public FileBasedProgramsProjectSystem(
         IAsynchronousOperationListenerProvider listenerProvider,
         ProjectLoadTelemetryReporter projectLoadTelemetry,
         ServerConfigurationFactory serverConfigurationFactory,
-        BinlogNamer binlogNamer)
+        IBinLogPathProvider binLogPathProvider)
             : base(
                 workspaceFactory.FileBasedProgramsProjectFactory,
                 workspaceFactory.TargetFrameworkManager,
@@ -52,7 +52,7 @@ public FileBasedProgramsProjectSystem(
                 listenerProvider,
                 projectLoadTelemetry,
                 serverConfigurationFactory,
-                binlogNamer)
+                binLogPathProvider)
     {
         _lspServices = lspServices;
         _logger = loggerFactory.CreateLogger<FileBasedProgramsProjectSystem>();

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileBasedProgramsWorkspaceProviderFactory.cs

@@ -8,6 +8,7 @@
 using Microsoft.CodeAnalysis.LanguageServer.Handler;
 using Microsoft.CodeAnalysis.LanguageServer.HostWorkspace.ProjectTelemetry;
 using Microsoft.CodeAnalysis.MetadataAsSource;
+using Microsoft.CodeAnalysis.MSBuild;
 using Microsoft.CodeAnalysis.Options;
 using Microsoft.CodeAnalysis.ProjectSystem;
 using Microsoft.CodeAnalysis.Shared.TestHooks;
@@ -33,10 +34,10 @@ internal sealed class FileBasedProgramsWorkspaceProviderFactory(
     IAsynchronousOperationListenerProvider listenerProvider,
     ProjectLoadTelemetryReporter projectLoadTelemetry,
     ServerConfigurationFactory serverConfigurationFactory,
-    BinlogNamer binlogNamer) : ILspMiscellaneousFilesWorkspaceProviderFactory
+    IBinLogPathProvider binLogPathProvider) : ILspMiscellaneousFilesWorkspaceProviderFactory
 {
     public ILspMiscellaneousFilesWorkspaceProvider CreateLspMiscellaneousFilesWorkspaceProvider(ILspServices lspServices, HostServices hostServices)
     {
-        return new FileBasedProgramsProjectSystem(lspServices, metadataAsSourceFileService, workspaceFactory, fileChangeWatcher, globalOptionService, loggerFactory, listenerProvider, projectLoadTelemetry, serverConfigurationFactory, binlogNamer);
+        return new FileBasedProgramsProjectSystem(lspServices, metadataAsSourceFileService, workspaceFactory, fileChangeWatcher, globalOptionService, loggerFactory, listenerProvider, projectLoadTelemetry, serverConfigurationFactory, binLogPathProvider);
     }
 }

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs

@@ -43,7 +43,7 @@ internal abstract class LanguageServerProjectLoader
     protected readonly ILoggerFactory LoggerFactory;
     private readonly ILogger _logger;
     private readonly ProjectLoadTelemetryReporter _projectLoadTelemetryReporter;
-    private readonly BinlogNamer _binlogNamer;
+    private readonly IBinLogPathProvider _binLogPathProvider;
     protected readonly ImmutableDictionary<string, string> AdditionalProperties;
 
     /// <summary>
@@ -98,7 +98,7 @@ protected LanguageServerProjectLoader(
         IAsynchronousOperationListenerProvider listenerProvider,
         ProjectLoadTelemetryReporter projectLoadTelemetry,
         ServerConfigurationFactory serverConfigurationFactory,
-        BinlogNamer binlogNamer)
+        IBinLogPathProvider binLogPathProvider)
     {
         ProjectFactory = projectFactory;
         _targetFrameworkManager = targetFrameworkManager;
@@ -108,7 +108,7 @@ protected LanguageServerProjectLoader(
         LoggerFactory = loggerFactory;
         _logger = loggerFactory.CreateLogger(nameof(LanguageServerProjectLoader));
         _projectLoadTelemetryReporter = projectLoadTelemetry;
-        _binlogNamer = binlogNamer;
+        _binLogPathProvider = binLogPathProvider;
         var workspace = projectFactory.Workspace;
         var razorDesignTimePath = serverConfigurationFactory.ServerConfiguration?.RazorDesignTimePath;
 
@@ -145,9 +145,7 @@ private async ValueTask ReloadProjectsAsync(ImmutableSegmentedList<ProjectToLoad
 
         // TODO: support configuration switching
 
-        var binaryLogPath = _binlogNamer.GetMSBuildBinaryLogPath();
-
-        await using var buildHostProcessManager = new BuildHostProcessManager(globalMSBuildProperties: AdditionalProperties, binaryLogPath: binaryLogPath, loggerFactory: LoggerFactory);
+        await using var buildHostProcessManager = new BuildHostProcessManager(globalMSBuildProperties: AdditionalProperties, binaryLogPathProvider: _binLogPathProvider, loggerFactory: LoggerFactory);
         var toastErrorReporter = new ToastErrorReporter();
 
         try

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs

@@ -34,7 +34,7 @@ public LanguageServerProjectSystem(
         IAsynchronousOperationListenerProvider listenerProvider,
         ProjectLoadTelemetryReporter projectLoadTelemetry,
         ServerConfigurationFactory serverConfigurationFactory,
-        BinlogNamer binlogNamer)
+        IBinLogPathProvider binLogPathProvider)
             : base(
                 workspaceFactory.HostProjectFactory,
                 workspaceFactory.TargetFrameworkManager,
@@ -45,7 +45,7 @@ public LanguageServerProjectSystem(
                 listenerProvider,
                 projectLoadTelemetry,
                 serverConfigurationFactory,
-                binlogNamer)
+                binLogPathProvider)
     {
         _logger = loggerFactory.CreateLogger(nameof(LanguageServerProjectSystem));
         var workspace = ProjectFactory.Workspace;

src/Workspaces/MSBuild/Core/MSBuild/BuildHostProcessManager.cs

@@ -25,15 +25,15 @@ internal sealed class BuildHostProcessManager : IAsyncDisposable
     private readonly ImmutableDictionary<string, string> _globalMSBuildProperties;
     private readonly ILoggerFactory? _loggerFactory;
     private readonly ILogger? _logger;
-    private readonly string? _binaryLogPath;
+    private readonly IBinLogPathProvider? _binaryLogPathProvider;
 
     private readonly SemaphoreSlim _gate = new(initialCount: 1);
     private readonly Dictionary<BuildHostProcessKind, BuildHostProcess> _processes = [];
 
-    public BuildHostProcessManager(ImmutableDictionary<string, string>? globalMSBuildProperties = null, string? binaryLogPath = null, ILoggerFactory? loggerFactory = null)
+    public BuildHostProcessManager(ImmutableDictionary<string, string>? globalMSBuildProperties = null, IBinLogPathProvider? binaryLogPathProvider = null, ILoggerFactory? loggerFactory = null)
     {
         _globalMSBuildProperties = globalMSBuildProperties ?? ImmutableDictionary<string, string>.Empty;
-        _binaryLogPath = binaryLogPath;
+        _binaryLogPathProvider = binaryLogPathProvider;
         _loggerFactory = loggerFactory;
         _logger = loggerFactory?.CreateLogger<BuildHostProcessManager>();
     }
@@ -244,10 +244,10 @@ private void AppendBuildHostCommandLineArgumentsConfigureProcess(ProcessStartInf
             AddArgument(processStartInfo, globalMSBuildProperty.Key + '=' + globalMSBuildProperty.Value);
         }
 
-        if (_binaryLogPath is not null)
+        if (_binaryLogPathProvider?.GetNewLogPath() is string binaryLogPath)
         {
             AddArgument(processStartInfo, "--binlog");
-            AddArgument(processStartInfo, _binaryLogPath);
+            AddArgument(processStartInfo, binaryLogPath);
         }
 
         AddArgument(processStartInfo, "--locale");

src/Workspaces/MSBuild/Core/MSBuild/IBinLogPathProvider.cs

@@ -0,0 +1,15 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+namespace Microsoft.CodeAnalysis.MSBuild;
+
+internal interface IBinLogPathProvider
+{
+    /// <summary>
+    /// Returns a new log path. Each call will return a new name, so that way we don't have collisions if multiple processes are writing to different logs.
+    /// </summary>
+    /// <returns>A new path, or null if no logging is currently wanted. An instance is allowed to switch between return null and returning non-null if
+    /// the user changes configuration.</returns>
+    string? GetNewLogPath();
+}

src/Workspaces/MSBuild/Test/BuildHostProcessManagerTests.cs

@@ -4,6 +4,7 @@
 
 using System.Collections.Generic;
 using System.Collections.Immutable;
+using Moq;
 using Roslyn.Test.Utilities;
 using Xunit;
 
@@ -65,7 +66,10 @@ internal void ProcessStartInfo_PassesBinLogPath(BuildHostProcessKind buildHostKi
     {
         const string BinaryLogPath = "test.binlog";
 
-        var processStartInfo = new BuildHostProcessManager(binaryLogPath: BinaryLogPath)
+        var binLogPathProviderMock = new Mock<IBinLogPathProvider>(MockBehavior.Strict);
+        binLogPathProviderMock.Setup(m => m.GetNewLogPath()).Returns(BinaryLogPath);
+
+        var processStartInfo = new BuildHostProcessManager(binaryLogPathProvider: binLogPathProviderMock.Object)
             .CreateBuildHostStartInfo(buildHostKind, pipeName: "");
 
 #if NET