Save thread working directory for fallback in Expander and Modifiers

[JanProvaznik]

Fixes #12851 #12800

Context

there are 2 places where it's impractical to pass TaskEnvironment but the code relies on CWD

1. Expander
2. Modifier on TaskItems -> FullPath

Changes Made

introduce thread local static CWD variable tied to lifetime of a TaskEnvironment

Testing

updated existing tests to dispose the environment

Notes

this makes the Aspire starter project build

[JanProvaznik]

@rainersigwald @AR-May this was in the prototype branch and seems very painful to avoid the thread static, so I just took it and made the lifecycle better. Should I try more to figure out the expander and metadata another way? I think it's quite bad UX to ban taskitem.GetMetadata("FullPath") in enlightened tasks :( that seems like something that customers would expect to work.

[baronfel]

For my own info, why use ThreadStatic attribute over ThreadLocal<T>? ThreadLocal seems to make it more clear to consumers that the value is indeed thread-local, and it's the same model we'd have to use for future AsyncLocal work (if/when we async-ify things).

[JanProvaznik]

No reason to prefer either for this use case I think, I'll change it to threadlocal since that's more modern (maybe will need preprocessor since that file might be also compiled for 35 takshost which does not have threadlocal)

[JanProvaznik]

I tried and didn't like it. The benefit of ThreadLocal is per-thread initialization which we don't use, so it's extra conditional compilation with no benefit.

[Copilot]

Pull request overview

This PR introduces thread-local working directory management to fix path resolution issues in multithreaded builds (issue #12851). The solution adds a thread-static CurrentThreadWorkingDirectory field in FileUtilities that is managed through the TaskEnvironment lifecycle via the IDisposable pattern. This allows the Expander and Modifiers (specifically %(FullPath) and Path.GetFullPath()) to resolve relative paths correctly in multithreaded mode without accessing the actual process current directory.

Key Changes:

• Added IDisposable to ITaskEnvironmentDriver for cleaning up thread-local state
• Thread-static CurrentThreadWorkingDirectory field set/cleared by MultiThreadedTaskEnvironmentDriver
• Fallback to thread-local CWD in Modifiers and WellKnownFunctions when current directory is unavailable

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/Shared/FileUtilities.cs	Adds CurrentThreadWorkingDirectory thread-static field for multithreaded path resolution
src/Shared/Modifiers.cs	Uses thread-local CWD as fallback when currentDirectory is null for %(FullPath) modifier
src/Build/Evaluation/Expander/WellKnownFunctions.cs	Uses thread-local CWD for Path.GetFullPath() function in property expansion
src/Framework/ITaskEnvironmentDriver.cs	Extends interface to implement IDisposable for cleanup
src/Framework/TaskEnvironment.cs	Adds internal Dispose() method to trigger driver cleanup
src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs	Sets thread-static on property setter and clears it in Dispose()
src/Build/BackEnd/TaskExecutionHost/MultiProcessTaskEnvironmentDriver.cs	Implements empty Dispose() for singleton pattern
src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEntry.cs	Disposes TaskEnvironment when build request completes
src/Build.UnitTests/BackEnd/TaskEnvironment_Tests.cs	Updates tests to properly dispose task environments

Comments suppressed due to low confidence (1)

src/Build.UnitTests/BackEnd/TaskEnvironment_Tests.cs:360

• This test doesn't validate that FileUtilities.CurrentThreadWorkingDirectory is properly set during the test and cleaned up after disposal. Since the PR introduces thread-local state management, consider adding assertions to verify:

1. That FileUtilities.CurrentThreadWorkingDirectory is set to the correct value when ProjectDirectory is set
2. That FileUtilities.CurrentThreadWorkingDirectory is null after Dispose() is called

This would ensure the core functionality of the PR (thread-local working directory management) is properly tested.

        [Fact]
        public void TaskEnvironment_MultithreadedEnvironment_ShouldBeIsolatedFromSystem()
        {
            string testVarName = $"MSBUILD_MULTITHREADED_ISOLATION_TEST_{Guid.NewGuid():N}";
            string testVarValue = "multithreaded_test_value";

            using var driver = new MultiThreadedTaskEnvironmentDriver(
                GetResolvedTempPath(),
                new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase));
            var multithreadedEnvironment = new TaskEnvironment(driver);

            try
            {
                // Verify system and environement doesn't have the test variable initially
                Environment.GetEnvironmentVariable(testVarName).ShouldBeNull();
                multithreadedEnvironment.GetEnvironmentVariable(testVarName).ShouldBeNull();

                // Set variable in multithreaded environment
                multithreadedEnvironment.SetEnvironmentVariable(testVarName, testVarValue);

                // Multithreaded should have the value but system should not
                multithreadedEnvironment.GetEnvironmentVariable(testVarName).ShouldBe(testVarValue);
                Environment.GetEnvironmentVariable(testVarName).ShouldBeNull();
            }
            finally
            {
                Environment.SetEnvironmentVariable(testVarName, null);
            }
        }

[AR-May]

We probably want to trace where this currentDirectory comes from and fix the value there as well:

msbuild/src/Build/Evaluation/Expander.cs

Line 2549 in 60df124

string directoryToUse = item.Value.ProjectDirectory ?? Directory.GetCurrentDirectory();

msbuild/src/Build/Evaluation/Expander.cs

Line 3309 in 60df124

string directoryToUse = sourceOfMetadata.ProjectDirectory ?? Directory.GetCurrentDirectory();

There are fall back calls to Directory.GetCurrentDirectory() which we should replace.

[baronfel]

At this point should we consider using tools like the BannedApiAnalyzer to blanket disallow IO methods from the BCL without explicitly allow listing them at each call site? This was incredibly helpful for me in other such scenarios in the SDK.

[AR-May]

Last time I checked there still were a lot of places where IO methods from the BCL are used, and it was for the most part legit for the multithreaded scenarios. Not everything goes through our file system abstraction layer. We can try to do that in separate PR and see.

[JanProvaznik]

i debugged on what thread those places in expander you mentioned are hit, and I couldn't get there easily with the Projectdirectory being null (mostly it's on BuildRequest thread and they're populated), perhaps it'll happen when you create taskitems in a custom task 🤔

[AR-May]

nit: consider adding to the comment that on the multiprocess mode this property is not set.