Fix some subtle invariants around needing new projectContextIds, especially for cached project contexts

Author: baronfel

src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs

@@ -527,18 +527,27 @@ private int GenerateNewProjectContextId(string projectFile)
         /// <summary>
         /// Validates that a preexisting projectContextId is valid and updates the project file map to track which contexts apply to which project files.
         /// </summary>
-        /// <param name="projectContextId">The preexisting project context ID to validate.</param>
+        /// <param name="remoteNodeEvaluationContext">The remote node evaluation context containing the project context ID to validate.</param>
         /// <param name="projectFile">The project file path to be associated with this project context ID.</param>
         /// <param name="currentNodeId">The node ID of the current node - used to validate that only the in-proc scheduler node is re-using cached project context IDs.</param>
-        private void ValidatePreexistingProjectContextId(int projectContextId, string projectFile, int currentNodeId)
+        /// <returns>Either the same context, if the projectContextId exists in the map already, or a new context with a generated projectContextId if it did not.</returns>
+        private BuildEventContext ValidatePreexistingProjectContextId(BuildEventContext remoteNodeEvaluationContext, string projectFile, int currentNodeId)
         {
+            if (remoteNodeEvaluationContext.ProjectContextId == BuildEventContext.InvalidProjectContextId)
+            {
+                // No projectContextId was provided, so generate a new one to represent this invocation of the project
+                int newProjectContextId = GenerateNewProjectContextId(projectFile);
+                return remoteNodeEvaluationContext.WithProjectContextId(newProjectContextId);
+            }
+
             // A projectContextId was provided, so use it with some sanity checks
-            if (_projectFileMap.TryGetValue(projectContextId, out string existingProjectFile))
+            else if (_projectFileMap.TryGetValue(remoteNodeEvaluationContext.ProjectContextId, out string existingProjectFile))
             {
                 if (!projectFile.Equals(existingProjectFile, StringComparison.OrdinalIgnoreCase))
                 {
-                    ErrorUtilities.ThrowInternalError("ContextID {0} was already in the ID-to-project file mapping but the project file {1} did not match the provided one {2}!", projectContextId, existingProjectFile, projectFile);
+                    ErrorUtilities.ThrowInternalError("ContextID {0} was already in the ID-to-project file mapping but the project file {1} did not match the provided one {2}!", remoteNodeEvaluationContext.ProjectContextId, existingProjectFile, projectFile);
                 }
+                return remoteNodeEvaluationContext;
             }
             else
             {
@@ -547,10 +556,11 @@ private void ValidatePreexistingProjectContextId(int projectContextId, string pr
                 // So we only need this sanity check for the in-proc node.
                 if (currentNodeId == Scheduler.InProcNodeId)
                 {
-                    ErrorUtilities.ThrowInternalError("ContextID {0} should have been in the ID-to-project file mapping but wasn't!", projectContextId);
+                    ErrorUtilities.ThrowInternalError("ContextID {0} should have been in the ID-to-project file mapping but wasn't!", remoteNodeEvaluationContext.ProjectContextId);
                 }
 
-                _projectFileMap[projectContextId] = projectFile;
+                _projectFileMap[remoteNodeEvaluationContext.ProjectContextId] = projectFile;
+                return remoteNodeEvaluationContext;
             }
         }
 
@@ -566,16 +576,18 @@ public ProjectStartedEventArgs CreateProjectStartedForCachedProject(
         {
             Debug.Assert(currentNodeBuildEventContext.NodeId == Scheduler.InProcNodeId, "Cached project build events should only be logged on the in-proc scheduler node.");
 
-            ValidatePreexistingProjectContextId(
-                remoteNodeEvaluationBuildEventContext.ProjectContextId,
+            BuildEventContext validatedRemoteNodeEvaluationBuildContext = ValidatePreexistingProjectContextId(
+                remoteNodeEvaluationBuildEventContext,
                 projectFile,
                 currentNodeBuildEventContext.NodeId);
 
             int configurationId = remoteNodeEvaluationBuildEventContext.ProjectInstanceId;
 
+            // we need to be a valid BuildEventContext for a ProjectLoggingContext out of this, so we need to make sure
+            // we have ProjectContextId set.
             BuildEventContextBuilder thisEventBuildEventContext = parentBuildEventContext
                 .WithProjectInstanceId(configurationId)
-                .WithProjectContextId(remoteNodeEvaluationBuildEventContext.ProjectContextId);
+                .WithProjectContextId(validatedRemoteNodeEvaluationBuildContext.ProjectContextId);
 
             ProjectStartedEventArgs buildEvent = new(
                 projectId: configurationId,
@@ -588,7 +600,7 @@ public ProjectStartedEventArgs CreateProjectStartedForCachedProject(
                 parentBuildEventContext: parentBuildEventContext,
                 globalProperties: globalProperties,
                 toolsVersion: toolsVersion,
-                originalBuildEventContext: remoteNodeEvaluationBuildEventContext)
+                originalBuildEventContext: validatedRemoteNodeEvaluationBuildContext)
             {
                 BuildEventContext = thisEventBuildEventContext,
             };

src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs

@@ -66,9 +66,13 @@ public static (ProjectStartedEventArgs, ProjectLoggingContext) CreateForLocalBui
                 requestEntry.RequestConfiguration.Project?.ItemsToBuildWith);
 
             IDictionary<string, string> globalProperties = requestEntry.RequestConfiguration.GlobalProperties.ToDictionary();
-
+            
+            BuildEventContext parentBuildEventContext = requestEntry.Request.ParentBuildEventContext == BuildEventContext.Invalid
+                ? nodeLoggingContext.BuildEventContext.WithEvaluationId(requestEntry.RequestConfiguration.ProjectEvaluationId).WithSubmissionId(requestEntry.Request.SubmissionId)
+                : requestEntry.Request.ParentBuildEventContext;
+            
             ProjectStartedEventArgs args = nodeLoggingContext.LoggingService.CreateProjectStartedForLocalProject(
-                requestEntry.Request.ParentBuildEventContext,
+                parentBuildEventContext,
                 requestEntry.RequestConfiguration.ConfigurationId,
                 requestEntry.RequestConfiguration.ProjectFullPath,
                 string.Join(";", requestEntry.Request.Targets),
@@ -112,28 +116,27 @@ public static ProjectLoggingContext CreateForCacheBuild(
         /// </summary>
         private static BuildEventContext CreateAndLogProjectStartedForCache(
             NodeLoggingContext nodeLoggingContext,
-            BuildRequest request,
+            BuildRequest newRequestThatWasServedFromCache,
             BuildRequestConfiguration configuration)
         {
             // Create a remote node evaluation context with the original evaluation ID
             BuildEventContext remoteNodeEvaluationBuildEventContext = BuildEventContext.CreateInitial(
                 configuration.ResultsNodeId, // Use the node that originally built this project configuration
-                request.SubmissionId)
+                newRequestThatWasServedFromCache.SubmissionId)
                 .WithEvaluationId(configuration.ProjectEvaluationId)
-                .WithProjectInstanceId(configuration.ConfigurationId)
-                // TODO: should we keep this data on the 'original' context - it is _very likely_ not the project
-                // context Id of the request that created the cached result, but it is the only data we have at this point (so far)
-                .WithProjectContextId(request.ProjectContextId);
+                .WithProjectInstanceId(configuration.ConfigurationId);
+                // we don't know the projectContextId of the remote eval, so we don't set it at all.
+                // the new request _does not have_ a valid projectContextId to go off of.
 
             IDictionary<string, string> globalProperties = configuration.GlobalProperties.ToDictionary();
 
             ProjectStartedEventArgs args = nodeLoggingContext.LoggingService.CreateProjectStartedForCachedProject(
                 nodeLoggingContext.BuildEventContext, // Current node context
                 remoteNodeEvaluationBuildEventContext, // Original remote node context
-                request.ParentBuildEventContext,
+                newRequestThatWasServedFromCache.ParentBuildEventContext,
                 globalProperties,
                 configuration.ProjectFullPath,
-                string.Join(";", request.Targets),
+                string.Join(";", newRequestThatWasServedFromCache.Targets),
                 configuration.ToolsVersion);
 
             nodeLoggingContext.LoggingService.LogProjectStarted(args);