Skip HandleBuildResultAsync when ProjectInstance is not loaded on in-proc node#13445
Open
YuliiaKovalova wants to merge 1 commit intomainfrom
Open
Skip HandleBuildResultAsync when ProjectInstance is not loaded on in-proc node#13445YuliiaKovalova wants to merge 1 commit intomainfrom
YuliiaKovalova wants to merge 1 commit intomainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates MSBuild’s project cache result handling to gracefully no-op when the BuildRequestConfiguration does not have a locally loaded ProjectInstance (common in VS builds where projects may execute on out-of-proc nodes and the in-proc configuration remains unloaded), removing the prior crash-diagnostics telemetry emission.
Changes:
- Remove diagnostic crash telemetry emission from
HandleBuildResultAsyncwhen the configuration has no locally loaded project instance. - Add an
IsLoaded-based early return to skip cache result handling when the project isn’t loaded on the in-proc node. - Expand inline documentation explaining why
HandleBuildResultAsynccan be reached with an unloaded configuration in VS/out-of-proc scenarios.
src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs
Outdated
Show resolved
Hide resolved
…ultAsync Telemetry data confirmed the root cause: in the VS scenario, ShouldUseCache returns true without checking IsLoaded, allowing configurations built on remote nodes (where Project is never loaded locally) to reach HandleBuildResultAsync with a null Project. This is expected behavior — the project cache plugin doesn't need to process results for projects that were never loaded on the in-proc node. Replace the diagnostic telemetry emission with a simple null check and early return.
92fa8f4 to
269a512
Compare
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
Related to https://github.com/dotnet/msbuild/pull/13332/changes
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