diff --git a/src/Build.UnitTests/BackEnd/HandshakeTempDir_Tests.cs b/src/Build.UnitTests/BackEnd/HandshakeTempDir_Tests.cs new file mode 100644 index 00000000000..9a658af9120 --- /dev/null +++ b/src/Build.UnitTests/BackEnd/HandshakeTempDir_Tests.cs @@ -0,0 +1,161 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.IO; +using Microsoft.Build.Internal; +using Microsoft.Build.UnitTests; +using Shouldly; +using Xunit; + +namespace Microsoft.Build.Engine.UnitTests.BackEnd; + +/// +/// Regression tests for https://github.com/dotnet/msbuild/issues/13594. +/// +/// Two MSBuild invocations launched with different temporary directories +/// (e.g. different TMP/TEMP/TMPDIR environment variables) +/// must NOT reuse each other's worker nodes. Otherwise build A finishing and +/// clearing its temp folder breaks the still-running build B which expected its +/// own folder to remain available on the shared node. +/// +/// The contract validated here: the salt incorporates +/// the per-process effective temp directory for non-TaskHost paths, so the +/// handshake key differs when the temp directory differs and the node-reuse +/// lookup will refuse to bind to a foreign node. +/// +/// TaskHost paths are intentionally exempted from the salt input. By default +/// task-host processes do not survive past a single build (cross-build TaskHost +/// reuse requires the rarely-set MSBUILDREUSETASKHOSTNODES=1 opt-in and +/// crashed parents kill sidecars via the pipe-link monitor), so the bug from +/// #13594 cannot manifest there in normal use. Excluding TaskHost paths from +/// the salt also avoids introducing a handshake-protocol mismatch on the NET +/// TaskHost path (parent .NET Framework MSBuild ↔ child shipped from a +/// separately-released .NET SDK), which intentionally tolerates parent/child +/// version skew. +/// +public sealed class HandshakeTempDir_Tests +{ + private readonly ITestOutputHelper _output; + + public HandshakeTempDir_Tests(ITestOutputHelper output) + { + _output = output; + } + + [Fact] + public void Handshake_DifferentTempDirectory_ProducesDifferentKey() + { + // Use distinct, fully qualified directories so Path.GetTempPath() returns + // a different normalized value in each TestEnvironment scope. + string keyA; + string keyB; + + using (TestEnvironment env = TestEnvironment.Create(_output)) + { + TransientTestFolder folder = env.CreateFolder(createFolder: true); + env.SetTempPath(folder.Path); + keyA = new Handshake(HandshakeOptions.NodeReuse).GetKey(); + } + + using (TestEnvironment env = TestEnvironment.Create(_output)) + { + TransientTestFolder folder = env.CreateFolder(createFolder: true); + env.SetTempPath(folder.Path); + keyB = new Handshake(HandshakeOptions.NodeReuse).GetKey(); + } + + keyA.ShouldNotBe(keyB, + "Two MSBuild invocations using different temp directories must produce " + + "distinct handshake keys so they do not reuse each other's nodes (issue #13594)."); + } + + [Fact] + public void Handshake_SameTempDirectory_ProducesSameKey() + { + // Sanity check: the temp dir contribution is deterministic, so two handshakes + // built under the same temp dir still match (otherwise legitimate node reuse breaks). + using TestEnvironment env = TestEnvironment.Create(_output); + TransientTestFolder folder = env.CreateFolder(createFolder: true); + env.SetTempPath(folder.Path); + + string key1 = new Handshake(HandshakeOptions.NodeReuse).GetKey(); + string key2 = new Handshake(HandshakeOptions.NodeReuse).GetKey(); + + key1.ShouldBe(key2, + "Identical environments must still produce identical handshake keys; " + + "otherwise node reuse would never succeed."); + } + + [Fact] + public void ServerNodeHandshake_DifferentTempDirectory_ProducesDifferentHash() + { + // MSBuildServer pipe names are derived from ServerNodeHandshake.ComputeHash(); + // changing TMP must yield a different server pipe so different temp envs do not + // collide on the same long-lived server process. + string hashA; + string hashB; + + using (TestEnvironment env = TestEnvironment.Create(_output)) + { + TransientTestFolder folder = env.CreateFolder(createFolder: true); + env.SetTempPath(folder.Path); + hashA = new ServerNodeHandshake(HandshakeOptions.None).ComputeHash(); + } + + using (TestEnvironment env = TestEnvironment.Create(_output)) + { + TransientTestFolder folder = env.CreateFolder(createFolder: true); + env.SetTempPath(folder.Path); + hashB = new ServerNodeHandshake(HandshakeOptions.None).ComputeHash(); + } + + hashA.ShouldNotBe(hashB, + "MSBuildServer pipe-name hashes must differ when the temp directory differs " + + "so two builds with different TMP cannot share the same server (issue #13594)."); + } + + /// + /// TaskHost handshakes (here: NET TaskHost) intentionally do NOT incorporate the temp + /// directory in their salt. This pins that exemption so a future change cannot accidentally + /// re-introduce a handshake-protocol mismatch on the NET TaskHost path, which is the only + /// handshake that tolerates parent (VS) / child (SDK) version skew across release trains. + /// + /// The exemption is also safe with respect to the original #13594 bug because cross-build + /// TaskHost reuse is gated on the off-by-default MSBUILDREUSETASKHOSTNODES=1 escape + /// hatch (see Traits.EscapeHatches.ReuseTaskHostNodes) and on the parent surviving + /// long enough to send NodeBuildComplete(PrepareForReuse=true); if the parent + /// crashes, the sidecar shuts itself down via OnLinkStatusChanged(ConnectionFailed). + /// + [Fact] + public void Handshake_NetTaskHost_DifferentTempDirectory_ProducesSameKey() + { + const HandshakeOptions netTaskHostOptions = HandshakeOptions.NET | HandshakeOptions.TaskHost; + // A toolsDirectory must be supplied for NET TaskHost handshakes; the actual path + // is irrelevant as long as it is identical across the two constructions. + string toolsDirectory = Path.Combine(Path.GetTempPath(), "fixed_tools_dir_for_test"); + + string keyA; + string keyB; + + using (TestEnvironment env = TestEnvironment.Create(_output)) + { + TransientTestFolder folder = env.CreateFolder(createFolder: true); + env.SetTempPath(folder.Path); + keyA = new Handshake(netTaskHostOptions, toolsDirectory).GetKey(); + } + + using (TestEnvironment env = TestEnvironment.Create(_output)) + { + TransientTestFolder folder = env.CreateFolder(createFolder: true); + env.SetTempPath(folder.Path); + keyB = new Handshake(netTaskHostOptions, toolsDirectory).GetKey(); + } + + keyA.ShouldBe(keyB, + "NET TaskHost handshake salt must NOT incorporate Path.GetTempPath(): the NET " + + "TaskHost path tolerates VS-vs-SDK version skew and an unsynchronized salt input " + + "would introduce a protocol mismatch. The bug from #13594 cannot manifest here " + + "in practice anyway because cross-build TaskHost reuse requires the off-by-default " + + "MSBUILDREUSETASKHOSTNODES=1 opt-in."); + } +} diff --git a/src/Framework/BackEnd/Handshake.cs b/src/Framework/BackEnd/Handshake.cs index 30f8b10e930..1d3fa073402 100644 --- a/src/Framework/BackEnd/Handshake.cs +++ b/src/Framework/BackEnd/Handshake.cs @@ -3,6 +3,7 @@ using System; using System.Diagnostics; +using System.IO; using System.Reflection; using Microsoft.Build.Framework; using Microsoft.Build.Shared; @@ -77,13 +78,31 @@ protected Handshake(HandshakeOptions nodeType, bool includeSessionId, string? to var options = (int)nodeType | (handshakeVersion << 24); CommunicationsUtilities.Trace($"Building handshake for node type {nodeType}, (version {handshakeVersion}): options {options}."); - // Calculate salt from environment and tools directory + // Calculate salt from environment, tools directory, and (for non-TaskHost paths) the + // effective temp directory. The temp directory is included so that two MSBuild invocations + // launched with different TMP/TEMP/TMPDIR environments do not reuse each other's worker + // nodes — without this, build A finishing and clearing its per-build temp folder would + // break a still-running build B that inherited the same node and expected its own temp + // folder to remain available (https://github.com/dotnet/msbuild/issues/13594). + // + // TaskHost paths are intentionally excluded from the temp-dir contribution: + // - NET TaskHost (parent .NET Framework MSBuild ↔ child shipped from a separately- + // released .NET SDK) tolerates version skew via NetTaskHostHandshakeVersion below; + // adding a salt input that isn't synchronized across both drops would break VS+SDK + // combinations until both sides picked up the change. + // - CLR2 TaskHost has NodeReuse disabled by design, so there is no cross-build reuse + // surface to protect. string handshakeSalt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT") ?? ""; - - int salt = CommunicationsUtilities.GetHashCode($"{handshakeSalt}{toolsDirectory}"); + bool isTaskHost = IsHandshakeOptionEnabled(nodeType, HandshakeOptions.TaskHost); + string tempDirectory = isTaskHost ? string.Empty : Path.GetTempPath(); + int salt = CommunicationsUtilities.GetHashCode($"{handshakeSalt}{toolsDirectory}{tempDirectory}"); CommunicationsUtilities.Trace($"Handshake salt is {handshakeSalt}"); CommunicationsUtilities.Trace($"Tools directory root is {toolsDirectory}"); + if (!isTaskHost) + { + CommunicationsUtilities.Trace($"Temp directory is {tempDirectory}"); + } // Get session ID if needed (expensive call) int sessionId = 0;