Replace ProjectCacheService null Project crash with diagnostic telemetry#13396
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents MSBuild builds from crashing when ProjectCacheService.HandleBuildResultAsync encounters a BuildRequestConfiguration whose ProjectInstance was never loaded locally (notably possible in VS scenarios). Instead of asserting, it logs diagnostic telemetry and safely skips cache result handling for that configuration.
Changes:
- Replace
VerifyThrowInternalNull(requestConfiguration.Project)with a null check. - Emit telemetry event
vs/msbuild/projectcache/nullprojectcapturing relevant configuration state. - Return early (skip plugin result handling) when
requestConfiguration.Projectis null.
| new Dictionary<string, string> | ||
| { | ||
| ["ConfigurationId"] = requestConfiguration.ConfigurationId.ToString(), | ||
| ["ProjectFullPath"] = Path.GetFileName(requestConfiguration.ProjectFullPath), |
Comment on lines
+828
to
+830
| { | ||
| ["ConfigurationId"] = requestConfiguration.ConfigurationId.ToString(), | ||
| ["ProjectFullPath"] = Path.GetFileName(requestConfiguration.ProjectFullPath), |
Comment on lines
+819
to
+838
| // The Project property may be null if the configuration was created from a remote node | ||
| // request or the project was never loaded locally (e.g., in VS scenario where ShouldUseCache | ||
| // returns true before checking IsLoaded). Skip cache result handling in this case. | ||
| if (requestConfiguration.Project is null) | ||
| { | ||
| _loggingService.LogTelemetry( | ||
| buildEventContext, | ||
| "vs/msbuild/projectcache/nullproject", | ||
| new Dictionary<string, string> | ||
| { | ||
| ["ConfigurationId"] = requestConfiguration.ConfigurationId.ToString(), | ||
| ["ProjectFullPath"] = Path.GetFileName(requestConfiguration.ProjectFullPath), | ||
| ["IsLoaded"] = requestConfiguration.IsLoaded.ToString(), | ||
| ["IsCached"] = requestConfiguration.IsCached.ToString(), | ||
| ["WasGeneratedByNode"] = requestConfiguration.WasGeneratedByNode.ToString(), | ||
| ["IsVsScenario"] = _isVsScenario.ToString(), | ||
| ["HasGlobalPlugins"] = (_globalProjectCacheDescriptor is not null).ToString(), | ||
| }); | ||
| return; | ||
| } |
Comment on lines
+828
to
+835
| { | ||
| ["ConfigurationId"] = requestConfiguration.ConfigurationId.ToString(), | ||
| ["ProjectFullPath"] = Path.GetFileName(requestConfiguration.ProjectFullPath), | ||
| ["IsLoaded"] = requestConfiguration.IsLoaded.ToString(), | ||
| ["IsCached"] = requestConfiguration.IsCached.ToString(), | ||
| ["WasGeneratedByNode"] = requestConfiguration.WasGeneratedByNode.ToString(), | ||
| ["IsVsScenario"] = _isVsScenario.ToString(), | ||
| ["HasGlobalPlugins"] = (_globalProjectCacheDescriptor is not null).ToString(), |
…ceful skip HandleBuildResultAsync crashed with InternalErrorException when configuration.Project was null. This can happen in the VS scenario where ShouldUseCache returns true before checking IsLoaded, allowing a configuration without a loaded ProjectInstance to reach this code. Instead of crashing, emit diagnostic telemetry with the configuration state (ConfigurationId, IsLoaded, IsCached, WasGeneratedByNode, IsVsScenario) and gracefully skip cache result handling.
YuliiaKovalova
force-pushed
the
dev/fix-projectcache-null-project
branch
from
356d931 to
328236d
Compare
This was referenced May 13, 2026
Closed
Closed
Closed
This was referenced Jun 2, 2026
Closed
Bump Microsoft.Build from 18.4.0 to 18.6.3
SkylineCommunications/Skyline.DataMiner.CICD.Packages#159
Open
This was referenced Jun 10, 2026
Closed
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HandleBuildResultAsyncinProjectCacheServicecrashes withInternalErrorException("Project unexpectedly null")when a build result arrives for a configuration whoseProjectInstancewas never loaded on the in-proc node.Root Cause
In VS (and global plugin) scenarios,
ShouldUseCachereturnstruewithout checkingIsLoaded. This is by design — it's needed for the pre-build cache request path (BuildManager.ExecuteSubmission), where the project hasn't been evaluated yet andPostCacheRequestloads it viaEvaluateProjectIfNecessary.However,
ShouldUseCacheis also called from the post-build path (BuildManager.HandleResult) to decide whether to notify cache plugins about the result. When a project was built on an out-of-proc worker node, the in-proc config cache entry never gets aProjectInstanceassigned —ProjectInstanceis intentionally not transferred back from worker nodes because it's too large to serialize across the named pipe (onlyBuildResultwith target outcomes is returned).This means
HandleBuildResultAsyncis called with a configuration whereIsLoadedis false and accessing.Projectwould crash.Fix
Replace the
VerifyThrowInternalNull(requestConfiguration.Project)assert with an!requestConfiguration.IsLoadedearly return.Why
IsLoadedinstead ofProject == null: TheProjectgetter has an internal assert (!IsCached) that throws if the configuration happens to be in cached-to-disk state.IsLoadedis a safe read-only check (_project?.IsLoaded == true) with no side effects or asserts. It also correctly handles the edge case of a partially-initializedProjectInstance(deserialized but not yetLateInitialize'd).Skipping is safe because:
HandleProjectFinishedAsyncrequires a loadedProjectInstanceto determine applicable plugins and provide project contextValidation
Diagnostic telemetry (
ProjectCacheState) deployed in a prior iteration confirmed the root cause across multiple VS repo users:IsLoaded=False,IsCached=False,WasGeneratedByNode=False,IsVsScenario=TrueBuildRequestDatawithoutProjectInstance(VS submits by path)ProjectInstancenever transferred back