Guard ActivelyBuildingTargets removal against stale request cleanup after cancellation timeout

src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs

@@ -776,5 +776,41 @@ public void TestProjectEvaluationIdPreservedAcrossTranslateForFutureUse()
 
             deserialized.ProjectEvaluationId.ShouldBe(expectedEvalId);
         }
+
+        /// <summary>
+        /// Verifies that <see cref="BuildRequestConfiguration.RemoveActivelyBuildingTargetIfOwnedBy"/> only removes the
+        /// entry when it is still owned by the specified request id. This protects against a stale request (e.g. one
+        /// whose cancellation timed out but which is still executing) resuming and erroneously clearing a different,
+        /// newer request's still-active entry for the same target name and configuration.
+        /// </summary>
+        [Fact]
+        public void RemoveActivelyBuildingTargetIfOwnedByOnlyRemovesMatchingOwner()
+        {
+            BuildRequestData data = new BuildRequestData("file", new Dictionary<string, string>(), "toolsVersion", Array.Empty<string>(), null);
+            BuildRequestConfiguration configuration = new BuildRequestConfiguration(1, data, "2.0");
+
+            const int staleRequestId = 1;
+            const int newRequestId = 2;
+
+            // The stale request originally recorded that it is building "Build".
+            configuration.ActivelyBuildingTargets["Build"] = staleRequestId;
+
+            // A newer request reused this retained configuration and is now building "Build" instead.
+            configuration.ActivelyBuildingTargets["Build"] = newRequestId;
+
+            // The stale request finally resumes (e.g. after a cancellation timeout) and tries to mark its target as
+            // no longer building. Since it no longer owns the entry, this must be a no-op.
+            configuration.RemoveActivelyBuildingTargetIfOwnedBy("Build", staleRequestId);
+
+            configuration.ActivelyBuildingTargets.ShouldContainKey("Build");
+            configuration.ActivelyBuildingTargets["Build"].ShouldBe(newRequestId);
+            configuration.IsActivelyBuilding.ShouldBeTrue();
+
+            // The owning (new) request completes normally and removes its own entry.
+            configuration.RemoveActivelyBuildingTargetIfOwnedBy("Build", newRequestId);
+
+            configuration.ActivelyBuildingTargets.ShouldNotContainKey("Build");
+            configuration.IsActivelyBuilding.ShouldBeFalse();
+        }
     }
 }

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

@@ -177,7 +177,7 @@ public async Task<BuildResult> BuildTargets(ProjectLoggingContext loggingContext
                 // If there are still targets left on the stack, they need to be removed from the 'active targets' list
                 foreach (TargetEntry target in _targetsToBuild)
                 {
-                    configuration.ActivelyBuildingTargets.Remove(target.Name);
+                    configuration.RemoveActivelyBuildingTargetIfOwnedBy(target.Name, _requestEntry.Request.GlobalRequestId);
                 }
 
                 ((IBuildComponent)taskBuilder).ShutdownComponent();
@@ -505,7 +505,7 @@ await PushTargets(errorTargets, currentTargetEntry, currentTargetEntry.Lookup, t
                             }
                             catch
                             {
-                                _requestEntry.RequestConfiguration.ActivelyBuildingTargets.Remove(currentTargetEntry.Name);
+                                _requestEntry.RequestConfiguration.RemoveActivelyBuildingTargetIfOwnedBy(currentTargetEntry.Name, _requestEntry.Request.GlobalRequestId);
                                 throw;
                             }
                         }
@@ -527,7 +527,7 @@ await PushTargets(errorTargets, currentTargetEntry, currentTargetEntry.Lookup, t
                         }
 
                         // This target is no longer actively building.
-                        _requestEntry.RequestConfiguration.ActivelyBuildingTargets.Remove(currentTargetEntry.Name);
+                        _requestEntry.RequestConfiguration.RemoveActivelyBuildingTargetIfOwnedBy(currentTargetEntry.Name, _requestEntry.Request.GlobalRequestId);
 
                         _buildResult.AddResultsForTarget(currentTargetEntry.Name, targetResult);
 
@@ -628,7 +628,7 @@ private void PopDependencyTargetsOnTargetFailure(TargetEntry topEntry, TargetRes
                     entry.LeaveLegacyCallTargetScopes();
 
                     // This target is no longer actively building (if it was).
-                    _requestEntry.RequestConfiguration.ActivelyBuildingTargets.Remove(topEntry.Name);
+                    _requestEntry.RequestConfiguration.RemoveActivelyBuildingTargetIfOwnedBy(topEntry.Name, _requestEntry.Request.GlobalRequestId);
 
                     // If we come across an entry which requires us to stop processing (for instance, an aftertarget of the original
                     // CallTarget target) then we need to use that flag, not the one from the top entry.

src/Build/BackEnd/Shared/BuildRequestConfiguration.cs

@@ -644,6 +644,27 @@ public Lookup BaseLookup
         public Dictionary<string, int> ActivelyBuildingTargets => _activelyBuildingTargets ?? (_activelyBuildingTargets =
                                                                       new Dictionary<string, int>(StringComparer.OrdinalIgnoreCase));
 
+        /// <summary>
+        /// Removes <paramref name="targetName"/> from <see cref="ActivelyBuildingTargets"/>, but only if it is still
+        /// recorded as being built by <paramref name="globalRequestId"/>. This guards against a stale request (for
+        /// instance one whose cancellation timed out but which is still executing) resuming after its entry was
+        /// overwritten -- or the table was cleared and repopulated -- by a newer request reusing this retained
+        /// configuration. Without this check, the stale request could erroneously remove the newer request's
+        /// still-active entry, which could in turn cause the configuration's <see cref="ProjectInstance"/> to be
+        /// cached while the newer request is still using it.
+        /// </summary>
+        /// <param name="targetName">The name of the target that is no longer actively building for the calling request.</param>
+        /// <param name="globalRequestId">The global request id of the request which believes it owns the target entry.</param>
+        internal void RemoveActivelyBuildingTargetIfOwnedBy(string targetName, int globalRequestId)
+        {
+            if (_activelyBuildingTargets is not null &&
+                _activelyBuildingTargets.TryGetValue(targetName, out int owningRequestId) &&
+                owningRequestId == globalRequestId)
+            {
+                _activelyBuildingTargets.Remove(targetName);
+            }
+        }
+
         /// <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.