Fix TerminalLogger assert failure for metaproj files and cached project eval ID#13480
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes TerminalLogger crashes and restores correct evaluation ID propagation across caching and cross-node result transfers, improving BuildEventContext consistency in multi-node (/m, /mt) builds.
Changes:
- Make TerminalLogger’s
ProjectStarteddebug assertion tolerant of non-evaluated.metaprojprojects. - Persist and propagate project evaluation IDs through configuration caching, node logging, worker→central result transfer, and
BuildResultserialization. - Add/adjust unit tests for BuildResult/configuration serialization of evaluation IDs.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Framework/FileUtilities.cs | Makes .metaproj detection helpers null-tolerant for broader call-site safety. |
| src/Build/Logging/TerminalLogger/TerminalLogger.cs | Avoids Debug.Assert failure for .metaproj projects that never emit evaluation events. |
| src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs | Centralizes metaproject detection by using FileUtilities.IsMetaprojectFilename. |
| src/Build/BackEnd/Shared/BuildResult.cs | Adds versioned _evaluationId field to allow eval ID propagation via result serialization. |
| src/Build/BackEnd/Shared/BuildRequestConfiguration.cs | Persists evaluation ID independently of cached ProjectInstance accessibility and serializes it. |
| src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs | Populates BuildResult.EvaluationId for success/abort/exception paths. |
| src/Build/BackEnd/Components/Logging/NodeLoggingContext.cs | Uses persisted ProjectEvaluationId instead of reading from a potentially cached project. |
| src/Build/BackEnd/BuildManager/BuildManager.cs | Updates configuration evaluation ID from worker node results when available. |
| src/Build.UnitTests/TerminalLogger_Tests.cs | Whitespace-only adjustments. |
| src/Build.UnitTests/BackEnd/BuildResult_Tests.cs | Adds test to validate translation preserves EvaluationId. |
| src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs | Adds tests for ProjectEvaluationId preservation (translation/clone/future-use path). |
rainersigwald
left a comment
There was a problem hiding this comment.
#13489 should improve the test that flaked out on the current run. I'm not rerunning because I'm not convinced this is the right solution to the problem thouh.
|
@rainersigwald |
I interpreted "releases" there as MSBuild releases, not this specific version that changes rarely. But this is not blocking. |
|
Hit rerun after putting up #13546 to hopefully stabilize the test that flaked. |
Fixes #13095
Context
These metaproj files are never evaluated — they have
EvaluationId = -1. The TerminalLogger'sDebug.Assertassumes everyProjectStartedhas a priorProjectEvaluationFinished, which fails for metaproj files and crashes the build.Additionally, a pre-existing bug (#12953) causes real projects to lose their evaluation IDs when their
ProjectInstanceis cached (released from memory), and whenBuildResultpackets are sent from worker nodes to the central node without carrying the eval ID.Changes Made
TerminalLogger metaproj fix:
TerminalLogger.cs: Assert "EvalProjectInfo should have been captured before ProjectStarted"); will not fail now for metaproj filesEvaluation ID fixes (adapted from PR #12946):
BuildRequestConfiguration.cs: Added_projectEvaluationIdfield that persists through caching + IPC serialization in bothTranslate()andTranslateForFutureUse()NodeLoggingContext.cs: Useconfig.ProjectEvaluationIdinstead ofconfig.Project.EvaluationId(which is inaccessible when cached)BuildResult.cs: Added_evaluationIdfield with version 2 serializationRequestBuilder.cs: Populateresult.EvaluationIdat success, exception, and abort pathsBuildManager.cs: Updateconfig.ProjectEvaluationIdfrom worker node resultsTesting
Unit tests
BuildRequestConfiguration_TestsBuildResult_TestsIntegration testing:
Notes