Fix buildeventcontext serialization and data-flow bugs from #12946

[baronfel]

This PR cherry-picks some bugfixes from #12946 so they can be evaluated separately.

There are three fixes, each with a separate commit:

• 6895213 ensures that 'cached' ProjectStartedEventArgs has a BuildEventContext whose NodeId is the node where the project was built from - this is important for detecting when a 'cached' build is being served from cross-node boundaries. In addition the EvaluationId is set correctly so that eval-time data can be correctly looked up.
• eb09ade fixes a data flow bug where TargetStarted and TaskStarted events didn't have a correct EvaluationId, so it was hard to correlate project data
• 9a25921 fixes a serialization bug where use of legacy BuildEventcontext constructors was seeding serialized events with invalid EvaluationIds. This meant that ProjectStarted events that were derived from another project would miss key data correlations.

All of these gaps make writing automated binlog analysis tooling (and tooling like Loggers!) harder or in some cases impossible.

[Copilot]

Pull request overview

This PR cherry-picks targeted fixes from #12946 to close gaps in BuildEventContext NodeId/EvaluationId propagation and serialization, improving event correlation for loggers and binlog analysis tooling (especially for cache-served projects and target/task events).

Changes:

• Preserve ParentProjectBuildEventContext.EvaluationId by round-tripping it in ProjectStartedEventArgs stream serialization.
• Log cache-served ProjectStartedEventArgs using BuildRequestConfiguration.ResultsNodeId (with fallback) to avoid invalid/stale NodeId values.
• Ensure TargetStarted/TaskStarted derived contexts preserve EvaluationId, with new unit tests covering the regressions.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/Framework/ProjectStartedEventArgs.cs	Writes/reads ParentProjectBuildEventContext.EvaluationId and constructs the parent context with the 7-arg ctor including evaluationId.
src/Framework.UnitTests/CustomEventArgSerialization_Tests.cs	Adds regression coverage ensuring parent-context EvaluationId survives WriteToStream/CreateFromStream.
src/Build/BackEnd/Components/Scheduler/Scheduler.cs	Uses configuration.ResultsNodeId when logging cache-served requests to avoid invalid/stale scheduling assignments.
src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs	Preserves EvaluationId when creating target/task BuildEventContext instances.
src/Build.UnitTests/BackEnd/BuildManager_Tests.cs	Adds regression tests for cached ProjectStarted NodeId/EvaluationId and for target/task EvaluationId preservation.

[github-actions]

🔍 24-Dimension Code Review Summary

Findings Table (non-LGTM dimensions only)

#	Dimension	Severity	Finding	Verdict
6	Logging & Binlog Compat	MODERATE	Initially flagged — dismissed after validation. WriteToStream/CreateFromStream is IPC-only; binlog uses BuildEventArgsWriter which already has EvaluationId.	✅ Safe
4	Test Coverage	NIT	ShouldBe(3) assertion is slightly fragile; no backward-compat deserialization test for older IPC version	⚠️ Minor
22	Correctness	NIT	GetAssignedNodeForRequestConfiguration fallback can return InvalidNodeId in override-cache scenarios — but this is a pre-existing issue the PR doesn't worsen	⚠️ Minor (pre-existing)
10	Design	NIT	ResultsNodeId may diverge from "original build node" after results transfers — speculative, not confirmed	i️ Informational

Clean Dimensions (20/24)

✅ Backwards Compatibility, ✅ ChangeWave Discipline, ✅ Performance, ✅ Error Messages (N/A), ✅ String Comparison (N/A), ✅ API Surface, ✅ Target Authoring (N/A), ✅ Cross-Platform (N/A), ✅ Code Simplification, ✅ Concurrency, ✅ Naming, ✅ SDK Integration (N/A), ✅ Idiomatic C#, ✅ File I/O (N/A), ✅ Documentation, ✅ Build Infrastructure (N/A), ✅ Scope & PR Discipline, ✅ Evaluation Model (N/A), ✅ Dependencies (N/A), ✅ Security (N/A)

Key Validation Results

1. Binary log compatibility: ✅ NOT BROKEN — The serialization change only affects IPC (node-to-node communication via LogMessagePacketBase), not .binlog files. The BinaryLogger already writes EvaluationId via its own Write(BuildEventContext) method. IPC safety is guaranteed by the node handshake protocol ensuring same-version on both sides.

2. ChangeWave: ✅ NOT NEEDED — These are pure data-correctness fixes (invalid -1 → valid IDs). No behavioral change to gate.

3. Concurrency: ✅ SAFE — All Scheduler operations are serialized through BuildManager._syncLock and the ActionBlock work queue.

Actionable Checklist

• Optional: Consider hardening _logger.ProjectStartedEvents.Count.ShouldBe(3) to filter by project file path for long-term test stability
• Optional: Consider a defensive check if nodeId remains InvalidNodeId after both fallback paths in LogRequestHandledFromCache

Overall Assessment

This is a well-scoped set of bug fixes that correctly propagates EvaluationId through the event hierarchy and fixes stale NodeId in cache-served results. The changes are safe, well-tested, and don't introduce breaking changes. No blocking issues found.

Generated by Expert Code Review (on open) for issue #13854 · ● 18.5M

[github-actions]

Nit (Test Fragility): This exact-count assertion could become brittle if MSBuild's internal behavior ever emits additional ProjectStartedEventArgs (e.g., from future implicit imports or metaproject generation). A more resilient pattern would be:

_logger.ProjectStartedEvents.Count.ShouldBeGreaterThanOrEqualTo(3);
ProjectStartedEventArgs firstChildStarted = _logger.ProjectStartedEvents
    .First(p => p.ProjectFile == childProject);
ProjectStartedEventArgs cachedChildStarted = _logger.ProjectStartedEvents
    .Where(p => p.ProjectFile == childProject).ElementAt(1);

That said, for these minimal test projects (no SDK, no Directory.Build.props), ShouldBe(3) is likely stable in practice. Your call on whether to harden it.

[github-actions]

Confirmed safe (for other reviewers' benefit): I initially investigated whether adding this field breaks .binlog backward compatibility. It does not — this WriteToStream/CreateFromStream pair is used exclusively for IPC between MSBuild nodes (via LogMessagePacketBase), not for the binary log file format. The binary logger uses BuildEventArgsWriter.Write(BuildEventContext) which already writes EvaluationId (see BuildEventArgsWriter.cs:1140).

IPC safety is guaranteed by the node handshake protocol — both sides are always the same MSBuild version. The version > 20 check in CreateFromStream (which is always true for .NET 6+ / .NET Framework 4.7.2+) correctly gates reading the extended fields.

[github-actions]

Question (Edge Case): In the fallback path, GetAssignedNodeForRequestConfiguration can return Scheduler.InvalidNodeId if the configuration was never scheduled in this session (e.g., from an override cache / isolated build scenario). This would result in a NodeLoggingContext with an invalid node ID.

However, I note this is a pre-existing issue — the original code before this PR called GetAssignedNodeForRequestConfiguration unconditionally and would have had the same problem. The fix here is strictly better since ResultsNodeId is more likely to be valid for cache-served results. Just flagging for awareness — if you want to be defensive:

if (nodeId == Scheduler.InvalidNodeId)
{
    nodeId = _schedulingData.GetAssignedNodeForRequestConfiguration(request.ConfigurationId);
}
// If still invalid, could default to in-proc node or skip logging

This is not blocking — the PR improves the status quo either way.

[jankratochvilcz]

Thank you! Left a couple of minor comments on code coverage, then I think it's good to go from my POV.

[jankratochvilcz]

Can we please expand a bit the coverage to also verify the other props are being transferred over? The reason is that since we're using constructor overloads where we pass a bunch of ints, I think it would be easy to introduce a regression where we e.g., pass things in the wrong order etc.,

[jankratochvilcz]

Plesae let's not reference "the fix" as it's not obvious from comments what this is referring to.

[jankratochvilcz]

Can we have a unit test on the fallback path? I'd be curious how we arrive to that scenario

[JanProvaznik]

Note

This comment was generated by GitHub Copilot CLI.

Review notes

I validated this branch locally (full build.cmd clean, 0 warnings; all 3 new tests pass) and then mutation-tested each hunk to check that its test actually guards it. Two things I think are blocking, plus some smaller items.

---

🔴 M1 — The Scheduler fix has zero regression coverage

src/Build.UnitTests/BackEnd/BuildManager_Tests.cs (CachedProjectStartedEventArgs_HasCorrectNodeIdAndEvaluationId)

Proven by mutation, not inference: I reverted Scheduler.cs:2215 back to the original int nodeId = _schedulingData.GetAssignedNodeForRequestConfiguration(request.ConfigurationId);, rebuilt, re-ran the test — it still passes.

Three independent reasons:

1. Wrong code path. _parameters (BuildManager_Tests.cs:77-82) sets only ShutdownInProcNodeOnBuildFinish, Loggers, EnableNodeReuse; MaxNodeCount stays at its default of 1. With one node the second <MSBuild> call is satisfied node-side by BuildRequestEngine.LogRequestHandledFromCache, so Scheduler.LogRequestHandledFromCache (Scheduler.cs:2206) is never entered. ProjectStartedEvents.Count == 3 passing confirms this — a scheduler-side cache log would have made it 4.
2. The NodeId assertion is a tautology even if the path were hit. Scheduler.cs:1436-1438 sets config.ResultsNodeId = nodeId, and the next statement request.ResumeExecution(nodeId) makes SchedulingData.cs:389 write the same nodeId into _configurationToNode. Old and new expressions return the identical int; they can only diverge with >1 node.
3. The two EvaluationId assertions test code already on main. git show origin/main:src/Build/BackEnd/Components/Logging/NodeLoggingContext.cs already contains int evaluationId = configuration?.ProjectEvaluationId ?? BuildEventContext.InvalidEvaluationId;. They pass without this PR.

For contrast, I ran the same mutation against the other two hunks:

• TargetAndTaskBuildEventContexts_PreserveEvaluationId fails when LoggingServiceLogMethods.cs:707/805 are reverted to the 6-arg ctor (EvaluationId should be 4 but was -1) — genuine guard, keep as-is. ✅
• TestProjectStartedParentContextEvaluationIdSerialization fails when the new field is dropped, and the pre-existing TestProjectStartedEventArgs does not — because its parent context new BuildEventContext(7, 8, 9, 10) already has EvaluationId == -1, masking the bug. The non-default 42 is what catches it. ✅

Suggested fix: clone _parameters with MaxNodeCount = 4; DisableInProcNode = true; and have two distinct parent projects on different nodes request the same child configuration, so HandleRequestBlockedByNewRequests → TrySatisfyRequestFromCache (Scheduler.cs:1775-1781) serves the second. Then confirm the test fails when line 2215 is reverted.

Alternatively a direct Scheduler_Tests unit test is cheap here: TestSimpleRequestWithCachedResultsSuccess (Scheduler_Tests.cs:127-149) already drives Scheduler.LogRequestHandledFromCache. The only missing piece is a capturing logging service — MockHost.LoggingService is settable, and MockLoggingService.CreateProjectStarted receives nodeLoggingContext.BuildEventContext, i.e. exactly the node id this PR changes. A [Theory] over (2 → 2), (InvalidNodeId → 1), (ResultsTransferredId → 1) covers all branches.

---

🔴 M2 — EvaluationId propagation silently disables BuildCheck property/env-var checks for metaprojects

src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs:558-592

TryGetProjectFullPath is an exclusive if / else if chain. The EvaluationId >= 0 branch has no fallback to _projectsByInstanceId — on a miss it falls through to the bottom and returns false:

if (buildEventContext.EvaluationId >= 0)            // 560
{
    if (_projectsByEvaluationId.TryGetValue(...)) { ...; return true; }
}                                                    // ← miss = no fallback
else if (buildEventContext.ProjectInstanceId >= 0)   // 568
{ ... }

Before this PR every target/task context had EvaluationId == -1, so it took the ProjectInstanceId branch. After LoggingServiceLogMethods.cs:707 and :805 they take the EvaluationId branch.

Concrete repro — solution metaproject. The two dictionaries are populated asymmetrically:

• BuildCheckBuildEventHandler.HandleProjectEvaluationStartedEvent guards the _projectsByEvaluationId population with if (!FileUtilities.IsMetaprojectFilename(eventArgs.ProjectFile)).
• BuildCheckBuildEventHandler.HandleProjectStartedRequest has no such guard, so _projectsByInstanceId does get metaproject entries.

So for App.sln.metaproj, a property read/write from inside a target now resolves EvaluationId >= 0 → miss in _projectsByEvaluationId → return false → BC0103 / BC0201 / BC0202 are silently skipped where they previously ran. Same failure mode for any project on a node that did not evaluate it (transferred ProjectInstance).

Callers reached with target/task contexts: ProcessPropertyRead / ProcessPropertyWrite (BuildCheckManagerProvider.cs:722, 739) via LoggingContext ← PropertyGroupIntrinsicTask, whose logging context is a TargetLoggingContext; and ProcessEnvironmentVariableReadEventArgs (:467).

This can't emit new warnings, so it's not a WarnAsError break — it's silent loss of analyzer coverage, which is harder to notice later.

Suggested fix — make the chain non-exclusive:

if (buildEventContext.EvaluationId >= 0 &&
    _projectsByEvaluationId.TryGetValue(buildEventContext.EvaluationId, out string? v))
{ projectFullPath = v; return true; }

if (buildEventContext.ProjectInstanceId >= 0 &&
    _projectsByInstanceId.TryGetValue(buildEventContext.ProjectInstanceId, out v))
{ projectFullPath = v; return true; }
// ... existing Count == 1 fallbacks ...

Caveat stated honestly: in a single in-proc-node build both data sources share one manager and EvaluationLoggingContext also fills _projectsByEvaluationId unfiltered, which masks the gap. The gap is real for EventArgs-only managers and for metaprojects.

---

🟡 Should fix

S1 — the new guard misses the second sentinel ResultsNodeId can hold. Scheduler.cs:2216

ResultsNodeId has two "not a node" values, not one: Scheduler.InvalidNodeId = -1 (covered) and Scheduler.ResultsTransferredId = -2 (Scheduler.cs:43, written at BuildRequestEngine.cs:409) — not covered. -2 is exactly BuildEventContext.InvalidNodeId, which NodeLoggingContext's ctor asserts against (Assumed.NotEqual(nodeId, BuildEventContext.InvalidNodeId, ...)) → InternalErrorException, build crash.

I verified this is not currently reachable — BuildRequestEngine.cs:407 guards the write with !MultiThreaded && NodeId != Scheduler.InProcNodeId, i.e. only inside out-of-proc worker processes, which never host a Scheduler. But _resultsNodeId is translated across the wire, and this PR introduces the first consumer that reads the field as a loggable node id. Hardening is one token:

if (nodeId is Scheduler.InvalidNodeId or Scheduler.ResultsTransferredId)

S2 — the new comment is factually wrong on both halves. Scheduler.cs:2210-2213

• "the node that originally built this project" — Scheduler.cs:1747 re-homes configuration.ResultsNodeId = parentRequest.AssignedNode on results transfer, so after a transfer it is not the originating node. That's precisely the multi-node scenario this PR targets.
• "the scheduling assignment, which may be invalid or stale" — "stale" is unsupported and inverted. SchedulingData.cs:389 rewrites _configurationToNode[configId] on every transition to Executing, so it always names the most recent node. In -mt mode it's ResultsNodeId that's frozen (RequestBuilder.NeedsResultsTransfer returns false unconditionally in MT, making Scheduler.cs:1747 unreachable there).
• The real defect being fixed is that GetAssignedNodeForRequestConfiguration returns InvalidNodeId when _configurationToNode has no live entry (SchedulingData.cs:622-631) — the common case for a purely cache-served request. Worth saying that directly.
• The test comment "Before the fix, this was the in-proc node (1) instead of the actual build node" is wrong for a MaxNodeCount = 1 test — node 1 is correct there.

S3 — documentation/wiki/Binary-Log.md is now stale, in the exact section this PR's audience reads.

• L91: "EvaluationId is present on all evaluation time events and on the ProjectStartedEventArgs" — it is now also on TargetStartedEventArgs and TaskStartedEventArgs. That's literally what the new TargetAndTaskBuildEventContexts_PreserveEvaluationId asserts.
• L93: "NodeId - indicates the node where the event was generated" — after Scheduler.cs:2215 a cache-served ProjectStartedEventArgs is generated by the scheduler but reports configuration.ResultsNodeId. That's a semantic redefinition of a documented field.

Given the stated goal is "writing automated binlog analysis tooling", tooling authors read this file to decide whether correlating by EvaluationId is legal.

S4 — the serialization test doesn't verify what a desync would actually corrupt. CustomEventArgSerialization_Tests.cs:703-743

It asserts _stream.Position and the parent context, but never calls VerifyGenericEventArg / VerifyProjectStartedEvent — unlike every sibling ProjectStarted assertion in the file. So projectId, projectFile, targetNames, properties go unchecked: exactly the fields a field-count desync mis-parses. Also submissionId: 1 is never verified, because InternalEquals deliberately excludes _submissionId — so the ShouldBe on the parent context can't catch a mis-ordered submissionId read/write.

S5 — housekeeping.

• No Fixes #12953 / Fixes #12998. Both issues are open and both are addressed here; they'll stay open after merge.
• The PR description attributes "In addition the EvaluationId is set correctly" to the Scheduler commit, but that commit touches only Scheduler.cs + tests — the EvaluationId half is already on main (NodeLoggingContext.cs, verified above).
• CustomEventArgSerialization_Tests.cs:706 cites ProjectStartedEvents for already-run projects have bad IDs #12953 (which is about a cache-served event's own NodeId/EvaluationId). That test is about parent-context linkage across IPC → Logger BuildEventContexts drop 'parent' context associations in certain use cases #12998, matching your own commit grouping.
• Scheduler.cs:2216 — 19 of 21 InvalidNodeId usages in that file use the bare form; Scheduler.InvalidNodeId self-qualification is redundant.
• BuildManager_Tests.cs — Count.ShouldBe(3) plus positional [1]/[2] is deterministic today but becomes racy the moment MaxNodeCount > 1, which the M1 fix requires. Filter by ProjectFile instead.

---

✅ Things I checked and disproved — no action needed

Recording these so nobody re-litigates them:

• Wire/serialization compat — not a break. version isn't an MSBuild schema version: s_defaultPacketVersion = (Environment.Version.Major * 10) + Environment.Version.Minor — it's the CLR version, so > 20 just means "not CLR 2.0" and the new Int32 is genuinely unversioned. But no version-skewed pair can carry this payload: worker nodes and MSBuild Server reject cross-version connections via the real AssemblyFileVersion in the handshake; the .NET task host (the one connection where versions may differ, via NetTaskHostHandshakeVersion = 99) cannot send a ProjectStartedEventArgs — its IBuildEngine log surface is only LogError/Warning/Message/CustomEvent; and CLR2 MSBuildTaskHost.exe doesn't compile ProjectStartedEventArgs.cs at all (it references GAC Microsoft.Build.Framework, Version=3.5.0.0, whose writer emits the matching 4 ints and whose packets arrive with version 20 → the else branch). The unguarded-write/guarded-read asymmetry also already existed for SubmissionId and ProjectInstanceId.
• Binlog FileFormatVersion bump — not needed. The binlog doesn't use this path; BuildEventArgsWriter.Write(BuildEventContext) is separate and already wrote EvaluationId. Record layout is byte-identical, only values change.
• BuildEventContext-keyed collections — no store/lookup asymmetry. The only product-code instances (ParallelLoggerHelpers.cs:20-33, ParallelConsoleLogger.cs:68,210,1700) all use ComparerContextNodeId / ComparerContextNodeIdTargetId, which ignore EvaluationId. Only absolute hash values shift.
• LoggingServiceLogMethods fix is complete. I enumerated every non-test new BuildEventContext(...). The non-2 LogTaskStarted constructs no context and has zero product callers; TargetLoggingContext/TaskLoggingContext get contexts solely from the two changed methods; WithInstanceIdAndContextId preserves _evaluationId.
• GetAssignedNodeForRequestConfiguration doesn't throw (TryGetValue, returns InvalidNodeId on miss), and the new fallback branch is live code (BuildRequestConfiguration._resultsNodeId defaults to Scheduler.InvalidNodeId; also set by CacheAggregator.cs:80 and BuildManager.cs:844).
• No new bogus-NodeId blast radius: {new nodeId == -1} ⊆ {old nodeId == -1}, so the TerminalLogger _nodes[NodeId - 1] hazard strictly shrinks.
• BuildEventContext.BuildRequestId => GetHashCode() has zero product consumers.
• No ChangeWave needed — no new error/warning, no build-outcome or default change. Gating this would require gating binlog content behind an env var, which is strictly worse. Fixing M2 is the right lever.
• No public API change — the 7-arg BuildEventContext ctor is pre-existing public; repo has no PublicAPI.*.txt / ref/ to update.
• No dependency changes.

---

Net: the LoggingServiceLogMethods and ProjectStartedEventArgs hunks look correct and well-targeted to me. The Scheduler.cs hunk is the one I'd be nervous merging — it's a real behavior change to ProjectStartedEventArgs.NodeId in multi-node cache-served builds, defended by a comment that misstates what both sides of the choice do, and by a test that provably doesn't execute it.