diff --git a/src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs b/src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs index 1f66abcb633..ff5b47f7fdd 100644 --- a/src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs +++ b/src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs @@ -776,5 +776,41 @@ public void TestProjectEvaluationIdPreservedAcrossTranslateForFutureUse() deserialized.ProjectEvaluationId.ShouldBe(expectedEvalId); } + + /// + /// Verifies that 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. + /// + [Fact] + public void RemoveActivelyBuildingTargetIfOwnedByOnlyRemovesMatchingOwner() + { + BuildRequestData data = new BuildRequestData("file", new Dictionary(), "toolsVersion", Array.Empty(), 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(); + } } } diff --git a/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs index ad694052f21..1d1e96741a0 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs @@ -177,7 +177,7 @@ public async Task 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. diff --git a/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs b/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs index 5ed219b6fde..24d07287657 100644 --- a/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs +++ b/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs @@ -644,6 +644,27 @@ public Lookup BaseLookup public Dictionary ActivelyBuildingTargets => _activelyBuildingTargets ?? (_activelyBuildingTargets = new Dictionary(StringComparer.OrdinalIgnoreCase)); + /// + /// Removes from , but only if it is still + /// recorded as being built by . 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 to be + /// cached while the newer request is still using it. + /// + /// The name of the target that is no longer actively building for the calling request. + /// The global request id of the request which believes it owns the target entry. + internal void RemoveActivelyBuildingTargetIfOwnedBy(string targetName, int globalRequestId) + { + if (_activelyBuildingTargets is not null && + _activelyBuildingTargets.TryGetValue(targetName, out int owningRequestId) && + owningRequestId == globalRequestId) + { + _activelyBuildingTargets.Remove(targetName); + } + } + /// /// Keeps the in memory while the caller uses it, preventing a concurrent /// memory-pressure cache sweep. Retrieves the project first if it was already cached.