Keep project active through post-build telemetry

src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs

@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using System.Threading.Tasks;
 using System.Xml;
 using Microsoft.Build.BackEnd;
 using Microsoft.Build.Collections;
@@ -674,6 +675,70 @@ public void TestProjectEvaluationIdPreservedInShallowClone()
             clone.ProjectEvaluationId.ShouldBe(expectedEvalId);
         }
 
+        [Fact]
+        public async Task CacheIfPossible_WhileProjectInstanceUsageIsHeld_DoesNotCache()
+        {
+            string projectBody = """
+                <Project ToolsVersion='msbuilddefaulttoolsversion' xmlns='msbuildnamespace'>
+                    <Target Name='Build' />
+                </Project>
+                """.Cleanup();
+
+            ProjectCollection collection = _env.CreateProjectCollection().Collection;
+            using ProjectFromString projectFromString = new(
+                projectBody,
+                new Dictionary<string, string>(),
+                ObjectModelHelpers.MSBuildDefaultToolsVersion,
+                collection);
+            Project project = projectFromString.Project;
+            project.FullPath = "foo";
+            ProjectInstance instance = project.CreateProjectInstance();
+
+            BuildRequestConfiguration configuration = new(new BuildRequestData(instance, [], null), "2.0")
+            {
+                ConfigurationId = 1,
+                IsCacheable = true,
+            };
+
+            _env.WithTransientTestState(new TransientConfigurationCacheFile(configuration));
+
+            configuration.CacheIfPossible();
+            configuration.IsCached.ShouldBeTrue();
+
+            using (configuration.AcquireProjectInstanceUsage())
+            {
+                configuration.IsCached.ShouldBeFalse();
+
+                using (configuration.AcquireProjectInstanceUsage())
+                {
+                    await Task.Run(configuration.CacheIfPossible);
+                    configuration.IsCached.ShouldBeFalse();
+
+                    // Caching nulls these out, so assert the project state itself and not just the flag.
+                    instance.GlobalPropertiesDictionary.ShouldNotBeNull();
+                    instance.PropertiesToBuildWith.ShouldNotBeNull();
+                    instance.ItemsToBuildWith.ShouldNotBeNull();
+                }
+
+                configuration.CacheIfPossible();
+                configuration.IsCached.ShouldBeFalse();
+            }
+
+            configuration.CacheIfPossible();
+            configuration.IsCached.ShouldBeTrue();
+        }
+
+        private sealed class TransientConfigurationCacheFile : TransientTestState
+        {
+            private readonly BuildRequestConfiguration _configuration;
+
+            internal TransientConfigurationCacheFile(BuildRequestConfiguration configuration)
+            {
+                _configuration = configuration;
+            }
+
+            public override void Revert() => _configuration.ClearCacheFile();
+        }
 
         [Fact]
         public void TestProjectEvaluationIdPreservedAcrossTranslateForFutureUse()

src/Build.UnitTests/BackEnd/RequestBuilder_Tests.cs

@@ -246,6 +246,48 @@ public void TestMissingProjectFile()
             Assert.Equal(typeof(InvalidProjectFileException), _buildRequestCompleted_Entry.Result.Exception.GetType());
         }
 
+        [Fact]
+        public void BuildProject_ConfigurationCacheSweepDuringBuild_DoesNotCacheConfiguration()
+        {
+            BuildRequestConfiguration configuration = CreateTestProject(1);
+            try
+            {
+                TestTargetBuilder targetBuilder = (TestTargetBuilder)_host.GetComponent(BuildComponentType.TargetBuilder);
+                IConfigCache configCache = (IConfigCache)_host.GetComponent(BuildComponentType.ConfigCache);
+
+                configCache.AddConfiguration(configuration);
+
+                BuildRequest request = CreateNewBuildRequest(1, new string[1] { "target1" });
+                BuildRequestEntry entry = new BuildRequestEntry(request, configuration, CreateStubTaskEnvironment());
+                BuildResult result = new BuildResult(request);
+                result.AddResultsForTarget("target1", GetEmptySuccessfulTargetResult());
+                targetBuilder.SetResultsToReturn(result);
+
+                bool? cachedDuringBuild = null;
+
+                // Simulate the BuildRequestEngine memory pressure sweep running on another thread while
+                // the request builder is still using the project instance of the configuration it builds.
+                targetBuilder.ActionBeforeReturningResult = () =>
+                {
+                    Task.Run(() => configCache.WriteConfigurationsToDisk()).GetAwaiter().GetResult();
+                    cachedDuringBuild = configuration.IsCached;
+                };
+
+                _requestBuilder.BuildRequest(GetNodeLoggingContext(), entry);
+
+                WaitForEvent(_buildRequestCompletedEvent, "Build Request Completed");
+
+                cachedDuringBuild.ShouldBe(false);
+                _buildRequestCompleted_Entry.Result.Exception.ShouldBeNull();
+                Assert.Equal(BuildResultCode.Success, _buildRequestCompleted_Entry.Result.OverallResult);
+            }
+            finally
+            {
+                configuration.ClearCacheFile();
+                DeleteTestProject(configuration);
+            }
+        }
+
         [Fact]
         public void RequestThreadProcEventsIncludeRequestContext()
         {
@@ -500,6 +542,12 @@ internal void SetResultsToReturn(BuildResult result)
             _cache.AddResult(result);
         }
 
+        /// <summary>
+        /// Invoked right before the cached result is returned, allowing tests to observe the state
+        /// of the build while the request builder is still building the project.
+        /// </summary>
+        internal Action ActionBeforeReturningResult { get; set; }
+
         internal void SetNewBuildRequests(FullyQualifiedBuildRequest[] requests)
         {
             _newRequests = requests;
@@ -547,8 +595,9 @@ public Task<BuildResult> BuildTargets(ProjectLoggingContext loggingContext, Buil
                 }
             }
 
-            return Task<BuildResult>.FromResult(_cache.GetResultForRequest(entry.Request));
-        }
+            ActionBeforeReturningResult?.Invoke();
+
+            return Task<BuildResult>.FromResult(_cache.GetResultForRequest(entry.Request));        }
 
         #endregion
 

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

@@ -382,10 +382,11 @@ public void SubmitBuildRequest(BuildRequest request)
                         BuildResult resultToReport = new BuildResult(request, result, null);
                         BuildRequestConfiguration config = ((IConfigCache)_componentHost.GetComponent(BuildComponentType.ConfigCache))[request.ConfigurationId];
 
-                        // Retrieve the config if it has been cached, since this would contain our instance data.  It is safe to do this outside of a lock
-                        // since only one thread can run in the BuildRequestEngine at a time, and it is EvaluateRequestStates which would cause us to
-                        // cache configurations if we are running out of memory.
-                        config.RetrieveFromCache();
+                        // Retrieve the project if it is cached and keep it in memory while preparing the result for
+                        // transfer. Multiple in-proc request engines share the configuration cache, so another engine
+                        // may run a memory-pressure sweep concurrently.
+                        using BuildRequestConfiguration.ProjectInstanceUsageScope projectInstanceUsage =
+                            config.AcquireProjectInstanceUsage();
                         ((IBuildResults)resultToReport).SavedCurrentDirectory = config.SavedCurrentDirectory;
                         ((IBuildResults)resultToReport).SavedEnvironmentVariables = config.SavedEnvironmentVariables;
                         if ((request.BuildRequestDataFlags & BuildRequestDataFlags.IgnoreExistingProjectState) != BuildRequestDataFlags.IgnoreExistingProjectState)

src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs

@@ -490,11 +490,9 @@ public bool ShouldUseCache(BuildRequestConfiguration buildRequestConfiguration)
                 return false;
             }
 
-            // We need to retrieve the configuration if it's already loaded in order to access the Project property below.
-            if (buildRequestConfiguration.IsCached)
-            {
-                buildRequestConfiguration.RetrieveFromCache();
-            }
+            // Retrieve the project if it is cached and keep it in memory while checking its cache descriptors.
+            using BuildRequestConfiguration.ProjectInstanceUsageScope projectInstanceUsage =
+                buildRequestConfiguration.AcquireProjectInstanceUsage();
 
             // Check if there are any project cache items defined in the project
             return GetProjectCacheDescriptors(buildRequestConfiguration.Project).Any();
@@ -530,6 +528,12 @@ public void PostCacheRequest(CacheRequest cacheRequest, CancellationToken cancel
 
             async ValueTask<(CacheResult Result, int ProjectContextId)> ProcessCacheRequestAsync()
             {
+                // The configuration cache is shared with the in-proc node's BuildRequestEngine, which may run a
+                // memory-pressure sweep on its own thread. Keep the project in memory for this entire operation:
+                // the ProjectInstance below is handed to the plugin and is used until the query completes.
+                using BuildRequestConfiguration.ProjectInstanceUsageScope projectInstanceUsage =
+                    cacheRequest.Configuration.AcquireProjectInstanceUsage();
+
                 EvaluateProjectIfNecessary(cacheRequest.Submission, cacheRequest.Configuration);
 
                 BuildRequestData buildRequest = new BuildRequestData(
@@ -839,15 +843,13 @@ public async Task HandleBuildResultAsync(
                 return;
             }
 
-            // If the ProjectInstance was evicted to disk to save memory, restore it before accessing
-            // the Project property below (whose getter asserts !IsCached).
-            if (requestConfiguration.IsCached)
+            List<ProjectCacheDescriptor> projectCacheDescriptors;
+            // Retrieve the project if it is cached and keep it in memory while determining which plugins apply.
+            using (requestConfiguration.AcquireProjectInstanceUsage())
             {
-                requestConfiguration.RetrieveFromCache();
+                projectCacheDescriptors = GetProjectCacheDescriptors(requestConfiguration.Project).ToList();
             }
 
-            // Filter to plugins which apply to the project, if any
-            List<ProjectCacheDescriptor> projectCacheDescriptors = GetProjectCacheDescriptors(requestConfiguration.Project).ToList();
             if (projectCacheDescriptors.Count == 0)
             {
                 return;

src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs

@@ -1152,11 +1152,16 @@ private async Task<BuildResult> BuildProject()
 
             buildCheckManager?.SetDataSource(BuildCheckDataSource.BuildExecution);
 
-            // Make sure it is null before loading the configuration into the request, because if there is a problem
-            // we do not wand to have an invalid projectLoggingContext floating around. Also if this is null the error will be
-            // logged with the node logging context
+            // Make sure it is null before loading or restoring the configuration so an invalid context is not reused.
+            // The failure paths below create a temporary project logging context before propagating the error.
             _projectLoggingContext = null;
 
+            // The configuration cache is shared across the node and may be swept for memory pressure on a
+            // BuildRequestEngine thread. A configuration becomes cacheable after its last target, but post-build
+            // telemetry still accesses its ProjectInstance, so keep the project in memory for this entire operation.
+            using BuildRequestConfiguration.ProjectInstanceUsageScope projectInstanceUsage =
+                AcquireProjectInstanceUsageWithProjectLoggingContext();
+
             try
             {
                 // Load the project
@@ -1192,11 +1197,7 @@ private async Task<BuildResult> BuildProject()
             catch
             {
                 // make sure that any errors thrown by a child project are logged in the context of their parent project: create a temporary projectLoggingContext
-                _projectLoggingContext = new ProjectLoggingContext(
-                    _nodeLoggingContext,
-                    _requestEntry.Request,
-                    _requestEntry.RequestConfiguration.ProjectFullPath,
-                    _requestEntry.RequestConfiguration.ToolsVersion);
+                CreateTemporaryProjectLoggingContext();
 
                 throw;
             }
@@ -1211,7 +1212,7 @@ private async Task<BuildResult> BuildProject()
             {
                 // Determine the set of targets we need to build
                 (string name, TargetBuiltReason reason)[] allTargets = _requestEntry.RequestConfiguration
-   .GetTargetsUsedToBuildRequest(_requestEntry.Request).ToArray();
+                    .GetTargetsUsedToBuildRequest(_requestEntry.Request).ToArray();
                 if (MSBuildEventSource.Log.IsEnabled())
                 {
                     MSBuildEventSource.Log.BuildProjectStart(_requestEntry.RequestConfiguration.ProjectFullPath, string.Join(", ", allTargets));
@@ -1221,8 +1222,6 @@ private async Task<BuildResult> BuildProject()
                 // Make sure to extract known immutable folders from properties and register them for fast up-to-date check
                 ConfigureKnownImmutableFolders();
 
-                // See comment on Microsoft.Build.Internal.Utilities.GenerateToolsVersionToUse
-                _requestEntry.RequestConfiguration.RetrieveFromCache();
                 if (_requestEntry.RequestConfiguration.Project.UsingDifferentToolsVersionFromProjectFile)
                 {
                     _projectLoggingContext.LogComment(MessageImportance.Low,
@@ -1314,6 +1313,28 @@ BuildResult CopyTargetResultsFromProxyTargetsToRealTargets(BuildResult resultFro
             }
         }
 
+        private BuildRequestConfiguration.ProjectInstanceUsageScope AcquireProjectInstanceUsageWithProjectLoggingContext()
+        {
+            try
+            {
+                return _requestEntry.RequestConfiguration.AcquireProjectInstanceUsage();
+            }
+            catch
+            {
+                CreateTemporaryProjectLoggingContext();
+                throw;
+            }
+        }
+
+        private void CreateTemporaryProjectLoggingContext()
+        {
+            _projectLoggingContext = new ProjectLoggingContext(
+                _nodeLoggingContext,
+                _requestEntry.Request,
+                _requestEntry.RequestConfiguration.ProjectFullPath,
+                _requestEntry.RequestConfiguration.ToolsVersion);
+        }
+
         private void UpdateStatisticsPostBuild()
         {
             ITelemetryCollector telemetryCollector = _nodeLoggingContext?.TelemetryCollector;

src/Build/BackEnd/Shared/BuildRequestConfiguration.cs

@@ -125,6 +125,11 @@ internal class BuildRequestConfiguration : IEquatable<BuildRequestConfiguration>
         /// </summary>
         private Dictionary<string, int> _activelyBuildingTargets;
 
+        /// <summary>
+        /// The number of operations currently using the in-memory project state.
+        /// </summary>
+        private int _projectInstanceUsageCount;
+
         /// <summary>
         /// The node where this configuration's master results are stored.
         /// </summary>
@@ -639,6 +644,48 @@ public Lookup BaseLookup
         public Dictionary<string, int> ActivelyBuildingTargets => _activelyBuildingTargets ?? (_activelyBuildingTargets =
                                                                       new Dictionary<string, int>(StringComparer.OrdinalIgnoreCase));
 
+        /// <summary>
+        /// Keeps the <see cref="ProjectInstance"/> in memory while the caller uses it, preventing a concurrent
+        /// memory-pressure cache sweep. Retrieves the project first if it was already cached.
+        /// </summary>
+        /// <remarks>
+        /// The usage count belongs to this configuration. A shallow clone that shares the same
+        /// <see cref="ProjectInstance"/> has independent synchronization and usage tracking.
+        /// </remarks>
+        internal ProjectInstanceUsageScope AcquireProjectInstanceUsage() => new(this);
+
+        /// <summary>
+        /// Tracks one active <see cref="ProjectInstance"/> consumer. Consume with <see langword="using"/>;
+        /// do not dispose copies.
+        /// </summary>
+        internal readonly struct ProjectInstanceUsageScope : IDisposable
+        {
+            private readonly BuildRequestConfiguration _configuration;
+
+            internal ProjectInstanceUsageScope(BuildRequestConfiguration configuration)
+            {
+                _configuration = configuration;
+
+                lock (_configuration._syncLock)
+                {
+                    _configuration.RetrieveFromCache();
+                    Assumed.False(_configuration.IsCached, "Configuration could not be retrieved before accessing the project.");
+                    _configuration._projectInstanceUsageCount++;
+                }
+            }
+
+            public void Dispose()
+            {
+                Assumed.NotNull(_configuration, "ProjectInstance usage scope was not initialized.");
+
+                lock (_configuration._syncLock)
+                {
+                    Assumed.True(_configuration._projectInstanceUsageCount > 0, "No active ProjectInstance usage to complete.");
+                    _configuration._projectInstanceUsageCount--;
+                }
+            }
+        }
+
         /// <summary>
         /// Holds a snapshot of the environment at the time we blocked.
         /// </summary>
@@ -725,7 +772,7 @@ public void CacheIfPossible()
         {
             lock (_syncLock)
             {
-                if (IsActivelyBuilding || IsCached || !IsLoaded || !IsCacheable)
+                if (_projectInstanceUsageCount > 0 || IsActivelyBuilding || IsCached || !IsLoaded || !IsCacheable)
                 {
                     return;
                 }