[Copilot]

Context

ChangeWaves parses MSBUILDDISABLEFEATURESFROMVERSION once and caches it for the process lifetime. Any process that serves more than one build — a reused worker node (node reuse is on by default) or an MSBuild Server node — keeps applying the change wave of the first build it served. With worker nodes the entry-point process is fresh but the nodes are not, so projects within a single build disagree about which waves are enabled depending on which node evaluated them:

set MSBUILDDISABLEFEATURESFROMVERSION=17.12
dotnet msbuild All.proj -t:Probe -m:4   ->  1:17.12  2:17.12  3:17.12  4:17.12

set MSBUILDDISABLEFEATURESFROMVERSION=18.5
dotnet msbuild All.proj -t:Probe -m:4   ->  1:18.5   2:17.12  3:17.12  4:17.12
                                                     ^^^^^^^^^^^^^^^^^^ stale

Rather than resetting the cache per request, this makes the change wave part of the node handshake, so a node that resolved a different wave is never reused in the first place.

Changes Made

• src/Framework/BackEnd/Handshake.cs — folds the resolved change wave into the handshake salt:

  string changeWaveSalt = GetChangeWaveSalt();
  int salt = CommunicationsUtilities.GetHashCode($"{handshakeSalt}{toolsDirectory}{changeWaveSalt}");

  This covers node modes 1 (worker node), 2 (task host) and 8 (MSBuild Server, via ServerNodeHandshake), plus the out-of-proc RAR node. A node whose wave disagrees fails the handshake and the parent starts a fresh one.

• Resolved wave, not the raw variable — ChangeWaves.DisabledWave is used so values that mean the same thing (unset, invalid format, out of rotation) still permit node reuse.

• Empty contribution when nothing is disabled — the default case leaves the salt byte-identical to what previous versions computed, so no existing connection changes behavior.

• CLR2 task host excluded — MSBuildTaskHost.exe computes its handshake from its own legacy copy of this code, which is intentionally left untouched; including the wave there would break that connection. IsClr2TaskHost moves out of its #if NETFRAMEWORK block since the parent side needs it on both TFMs.

• documentation/wiki/ChangeWaves.md — notes that the wave participates in the handshake and that changing it starts new nodes instead of reusing existing ones.

Testing

New src/Build.UnitTests/BackEnd/Handshake_Tests.cs covers differing, identical and equivalent wave values for both Handshake and ServerNodeHandshake, plus the CLR2 exclusion. The two inequality assertions fail against the unmodified Handshake.cs.

End-to-end against a bootstrap build using the repro above, every project reports the current wave on repeated switches and when unset, with MSBUILDUSESERVER=0 and =1. Process listings show one additional node set / server per distinct wave, and reuse when the wave is unchanged.

Notes

$(MSBuildDisableFeaturesFromVersion) is a reserved property set from ChangeWaves.DisabledWave, which is why the repro's Message observes the node's cached state rather than the environment.

The CLR2 exclusion is covered by a unit test, but its end-to-end test only runs on Windows with .NET Framework 3.5 installed and was skipped locally.

Cross-version compatibility is worth a look during review: the .NET task host is the one handshake that is deliberately version-tolerant (VS parent, SDK child). Keeping the default contribution empty means mixed-version pairs are unaffected unless someone actually sets MSBUILDDISABLEFEATURESFROMVERSION, in which case a pre-change peer would be rejected.

• Fixes ChangeWaves state is not reset between builds in reused nodes (worker nodes and MSBuild Server) #14547

[dotnet-policy-service]

Hello @copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo.

[Copilot]

Pull request overview

This PR addresses stale ChangeWaves state in reused processes (out-of-proc worker nodes, task hosts, and MSBuild Server nodes) by making the resolved change wave participate in the node handshake, ensuring nodes with differing resolved waves are not reused across builds.

Changes:

• Fold the resolved change wave (ChangeWaves.DisabledWave) into the handshake salt so nodes/servers with a different resolved wave reject the connection and are restarted.
• Add unit tests validating handshake divergence for differing waves, equivalence for “no-op” wave values, and CLR2 task host exclusion.
• Document that changing MSBUILDDISABLEFEATURESFROMVERSION causes new nodes to be created instead of reusing existing ones.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/Framework/BackEnd/Handshake.cs	Incorporates resolved change wave into handshake salt (with CLR2 task host exclusion) to prevent cross-wave node reuse.
src/Build.UnitTests/BackEnd/Handshake_Tests.cs	Adds coverage for handshake behavior across differing/equivalent wave values and for server handshake hashing.
documentation/wiki/ChangeWaves.md	Documents handshake participation and the resulting node reuse behavior.

Suppressed comments (3)

src/Build.UnitTests/BackEnd/Handshake_Tests.cs:72

• ChangeWaves.Wave17_12 is not defined anymore, so this test will not compile. Use ChangeWaves.LowestWave (or another in-rotation wave) instead.

            string firstKey = GetKeyForChangeWave(ChangeWaves.Wave17_12.ToString(), env, s_workerNodeOptions);
            string secondKey = GetKeyForChangeWave(ChangeWaves.Wave17_12.ToString(), env, s_workerNodeOptions);

src/Build.UnitTests/BackEnd/Handshake_Tests.cs:112

• ChangeWaves.Wave17_12 is not defined (current ChangeWaves rotation is Wave18_*), so this won’t compile. Use an existing wave such as ChangeWaves.LowestWave for the first value.

            SetChangeWave(ChangeWaves.Wave17_12.ToString(), env);
            string firstHash = new ServerNodeHandshake(options).ComputeHash();

            SetChangeWave(ChangeWaves.HighestWave.ToString(), env);
            string secondHash = new ServerNodeHandshake(options).ComputeHash();

src/Build.UnitTests/BackEnd/Handshake_Tests.cs:129

• ChangeWaves.Wave17_12 doesn’t exist in the current ChangeWaves implementation, which will break compilation. Use ChangeWaves.LowestWave (or another existing wave) here as well.

            string firstKey = GetKeyForChangeWave(ChangeWaves.Wave17_12.ToString(), env, options);
            string secondKey = GetKeyForChangeWave(ChangeWaves.HighestWave.ToString(), env, options);

[JanProvaznik]

/review

[github-actions]

✅ Expert Code Review (command) completed successfully!

[github-actions]

Review: Include resolved change wave in node handshake

Overall assessment: LGTM with one minor nit

This is a well-designed, well-tested fix for a real correctness problem: reused worker/server nodes silently applying the wrong change wave across builds. The implementation is clean and the test coverage is thorough.

What the PR does

Folds the resolved change wave (not the raw env-var string) into the handshake salt. Nodes that resolved a different wave refuse the connection, so MSBuild starts a fresh node instead of reusing a stale one. Values that resolve to the same wave (unset, invalid format, out-of-rotation high value) all produce the same empty salt, preserving node reuse for those equivalent cases. CLR2 task host is explicitly excluded because its MSBuildTaskHost.exe has its own copy of the handshake code that knows nothing about change waves.

Correctness

Backwards compatibility: The handshake salt change is intentional and appropriate. Existing nodes built against older MSBuild will refuse the new handshake and be recycled — exactly the desired behaviour. No silent data corruption.

Thread safety: ChangeWaves.DisabledWave reads _cachedWave (a static field) without a lock. This was already the case before this PR; GetChangeWaveSalt() is called once in the Handshake constructor, so no new concurrency concern is introduced.

#if NETFRAMEWORK guard removal: IsClr2TaskHost was previously guarded by #if NETFRAMEWORK because it was only needed there. This PR promotes it to an unconditional member so GetChangeWaveSalt() can call it. Since HandshakeOptions.CLR2 is a valid flag on all target frameworks, the property is correct everywhere — removing the guard is safe.

Test coverage

Five focused tests covering: different waves → different handshakes, same wave → same handshake, equivalent env-var values → same handshake, server node handshake differentiation, and CLR2 exclusion. All key scenarios are exercised.

Inline comment

One minor doc comment issue flagged inline (dangling sentence fragment on the GetChangeWaveSalt XML doc — no functional impact).

Generated by Expert Code Review (command) for #14557 · sonnet46 · 36.1 AIC · ⌖ 5.2 AIC · ⊞ 4.9K
_{Comment /review to run again}

[JanProvaznik]

@copilot this does not build and address:

MINOR — Dangling sentence fragment in XML doc

The sentence "there is nothing to segregate, so the salt stays identical to what previous versions computed." has no antecedent — it appears to be a leftover from a prior draft of the comment. Every sentence before it describes the resolved-wave approach, then this sentence refers to an unspecified subject. Please remove it or rewrite the paragraph so every sentence is complete.

[Copilot]

@copilot this does not build and address:

MINOR — Dangling sentence fragment in XML doc
...

Fixed in 4a24369. Removed the dangling XML-doc fragment and replaced stale Wave17_12 test references with LowestWave.