Don't discard the XML cache after restore when it reloads from disk

[ViktorHofer]

Fixes #14556.

Context

The implicit restore issues its build request with BuildRequestDataFlags.ClearCachesAfterBuild, and BuildManager.CheckAllSubmissionsComplete responds by discarding the entire ProjectRootElementCache — every .props/.targets in the import closure, not just what restore touched.

That is load-bearing for the classic cache: autoReloadFromDisk is false there, so nothing else would notice that restore rewrote nuget.g.props/nuget.g.targets, and the process exits after the build anyway so there is no cross-build value to lose.

Both facts invert for the cache the MSBuild Server entry node reuses across builds, which is constructed with autoReloadFromDisk: true (XMake.cs passes reuseProjectRootElementCache: s_isServerNode). The timestamp check in IsInvalidEntry already covers restore's edits, and the cache being discarded is precisely the one meant to survive. The flush there buys nothing and costs a full re-parse of the import closure on the build half of every dotnet build.

Change

Skip only the ProjectRootElementCache.Clear() when the cache reloads from disk, behind change wave 18.10:

if (_buildParameters?.ProjectRootElementCache is { } projectRootElementCache &&
    !(projectRootElementCache.AutoReloadFromDisk && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_10)))
{
    projectRootElementCache.Clear();
}

FileMatcher.ClearCaches();
FileUtilities.ClearFileExistenceCache();

This mirrors DiscardImplicitReferences(), which already returns early for an auto-reloading cache with the same justification.

FileMatcher and FileUtilities keep being cleared unconditionally, deliberately: they cache negative results (a file that did not exist, a glob that matched nothing) that no timestamp check can invalidate, and that restore invalidates precisely by creating files.

AutoReloadFromDisk is new on ProjectRootElementCacheBase (virtual false, overridden by ProjectRootElementCache). No public API change.

Measurement

Timing the evaluation that follows a restore-like submission on a dotnet new console project (116 files, 1.4 MB of XML), in-process so that JIT and process startup do not mask the effect. Measured by building two binaries rather than toggling the change wave, since opting out of a wave also disables unrelated features gated on it:

cache	scenario	before	after
reloads from disk	1 project	+250.2 ms / +11.88 MB	~0 ms / +0.16 MB
reloads from disk	12 projects	+380.2 ms / +12.73 MB	~0 ms / +0.25 MB
does not reload	1 project	+274.2 ms / +11.87 MB	unchanged

The allocation column is the signal worth trusting; 11.9 MB matches parsing the whole import closure from scratch, and it was identical on every round. The millisecond deltas after the fix land slightly negative, which is measurement noise (the post-flush pass runs later and is better warmed) — read them as zero, not as a speedup.

Two things worth knowing about the shape of the cost: it is paid once per build, by whichever project is evaluated first, which then repopulates the cache for everything behind it — so it is a fixed tax rather than a per-project one, and it shrinks as a fraction of a large build. And it lands only on the entry node; worker nodes were already exempt.

Testing

• ClearCachesAfterBuildKeepsCacheThatReloadsFromDisk — an auto-reloading cache survives the flush, and the wave-opt-out case asserts the old behavior is preserved.
• ClearCachesAfterBuildStillClearsCacheThatDoesNotReloadFromDisk — a cache that cannot notice disk changes is still discarded.

Both use autoReloadFromDisk: true for the positive/negative wave pair so that DiscardImplicitReferences (which early-returns in that configuration) cannot be the reason an entry disappears, leaving the flush as the only variable.

Existing *ProjectRootElementCache* tests pass.

[Copilot]

Pull request overview

This PR adjusts MSBuild’s ClearCachesAfterBuild behavior so that an auto-reloading ProjectRootElementCache (as used by MSBuild Server) is not unnecessarily discarded after the implicit restore submission, avoiding a full re-parse of the import closure on the subsequent build.

Changes:

• Introduces ProjectRootElementCacheBase.ClearCachesAfterBuild() to let caches decide how to respond to BuildRequestDataFlags.ClearCachesAfterBuild.
• Overrides ClearCachesAfterBuild() in ProjectRootElementCache to preserve cache contents when autoReloadFromDisk is enabled (behind a change wave).
• Adds unit tests covering both the preserved-cache (wave enabled) and legacy (wave opted out / non-reloading cache) behaviors, and documents the change in ChangeWaves.md.

Show a summary per file

File	Description
src/Build/Evaluation/ProjectRootElementCacheBase.cs	Adds an overridable ClearCachesAfterBuild() hook for ClearCachesAfterBuild-flag handling.
src/Build/Evaluation/ProjectRootElementCache.cs	Implements wave-gated “don’t clear if auto-reloading” behavior for the main PRE cache.
src/Build/BackEnd/BuildManager/BuildManager.cs	Switches from calling Clear() directly to calling ClearCachesAfterBuild() on the PRE cache.
src/Build.UnitTests/BackEnd/BuildManager_Tests.cs	Adds regression tests validating preserved/cleared behavior across wave enablement and autoReload settings.
documentation/wiki/ChangeWaves.md	Documents the behavior change under the relevant wave section.

Copilot's findings

• Files reviewed: 5/5 changed files
• Comments generated: 1

[ViktorHofer]

Ran a self-review over the current state of the branch (rebased onto main, wave moved to 18.11, cache decision pushed inside ProjectRootElementCache per @rainersigwald's feedback). Posting the findings so they're visible before anyone spends more time reviewing — I intend to address at least the two majors.

🔴 The justifying comment is factually wrong for NuGet-cache paths

IsInvalidEntry has a second early-out before the timestamp comparison:

// ProjectRootElementCache.cs:174-178
if (!Traits.Instance.EscapeHatches.AlwaysDoImmutableFilesUpToDateCheck
    && FileClassifier.Shared.IsNonModifiable(projectFile))
{
    return false;   // never revalidated
}

FileClassifier.RegisterKnownImmutableLocations registers NuGetPackageFolders — i.e. ~/.nuget/packages — as immutable. That's precisely the directory restore writes to, so every cached PRE for a package's build/*.props / build/*.targets is pinned and the auto-reload timestamp check is skipped by design. My comment claiming "auto reload properly invalidates cache entries whose file changed on disk. That covers whatever restore rewrote" is not true for the one file class restore owns most directly.

Concrete scenario on a Server entry node with the wave enabled: package re-extracted at the same path between two builds (local repack + dotnet nuget locals global-packages --clear) is served stale until the node is killed.

Worth noting this hole already exists for reused worker nodes — OutOfProcNode sets autoReloadFromDisk: true and the pre-existing comment in CheckAllSubmissionsComplete explicitly relies on it — so this PR widens it to the entry node rather than creating a new class of bug. It's also disarmed by the change wave and by MSBUILDALWAYSCHECKIMMUTABLEFILESUPTODATE.

Two options, and I'd like input on which:

1. Just fix the comment to state the real invariant (entries under FileClassifier-immutable roots are never revalidated; matches what reused worker nodes already do).
2. Additionally, when skipping the full clear, evict just the entries the timestamp check can't cover. FileClassifier.Shared.IsInNugetCache(path) is exactly the right predicate and is narrower than IsNonModifiable, so it leaves the SDK/VS targets that make up the bulk of the import closure — and the perf win — in place. DiscardImplicitReferences already does this style of weak-cache rebuild, so it's ~10 lines.

My inclination is (2) plus the comment fix, since it keeps essentially all of the measured win while closing the one gap restore can actually cause.

🔴 Neither test proves the correctness invariant

Both new tests assert only cache retention mechanics (TryGet(...) null / not-null). Neither exercises what the PR actually rests on: a restore submission rewrites an imported file, and the build that follows must observe the new content even though the cache wasn't flushed. If IsInvalidEntry regressed tomorrow, both tests would still pass while -restore silently produced stale builds.

Adding an end-to-end test: a Restore target that rewrites an imported .props, then a second BuildRequest whose target errors unless it sees the new value. (XMake_Tests.RestoreFirstClearsProjectRootElementCache has the right shape but only covers the default autoReloadFromDisk: false path.)

🟡 Moderate

• BuildRequestDataFlags.ClearCachesAfterBuild XML doc is now stale. It's public in Microsoft.Build.Framework and states caches "will be cleared"; that contract is now conditional. Third parties can reach an auto-reloading cache through the public ProjectCollection(..., reuseProjectRootElementCache: true, ...) overload, so this needs amending.
• Deleted imports. IsInvalidEntry returns false when GetFileInfoNoThrow is null ("if the file doesn't exist on disk, use the cached version"), so an unconditioned import that restore deletes is also served stale. Narrow — the SDK path guards nuget.g.props with Condition="Exists(...)" — but it belongs in the same corrected comment.
• No diagnostic breadcrumb. Every other decision in this cache traces via DebugTraceCache (MSBUILDDEBUGXMLCACHE=1); the one new decision that changes observable build behavior emits nothing. Adding a trace on both branches.

Things that checked out

• Change wave gate placement and opt-out: opting out falls straight through to Clear(), byte-for-byte the pre-PR behavior. _autoReloadFromDisk is constructor-set and never mutated, so no ordering concern at the _syncLock-held call site.
• SimpleProjectRootElementCache correctly needs no override — it does no timestamp validation at all, so the base => Clear() is right.
• base.ClearCachesAfterBuild() → virtual Clear() is not a recursion hazard; the base calls Clear(), which dispatches to the derived override and does not re-enter.
• No issue found with the strong/weak cache split, preserveFormatting variants, or _fileLoadLocks. Files that did not exist when the cache was populated aren't represented in the PRE cache at all — only in the FileMatcher / FileUtilities caches, which this PR correctly keeps flushing unconditionally.
• Test env restoration is sound: TestEnvironment.Cleanup unconditionally clears MSBUILDDISABLEFEATURESFROMVERSION and resets wave state.

Nits I'll fold in

ClearCachesAfterBuild is an imperative name for a method whose derived job is to not clear — renaming to ClearCachesAfterBuildIfNeeded. ChangeWaves.md entry links the issue while its two siblings link the PR. The [InlineData] second column is always equal to the first. One dead ResetStateForTests() in the test that sets no environment variable. Stray whitespace hunk.

[rainersigwald]

FileClassifier.RegisterKnownImmutableLocations registers NuGetPackageFolders — i.e. ~/.nuget/packages — as immutable. That's precisely the directory restore writes to, so every cached PRE for a package's build/*.props / build/*.targets is pinned and the auto-reload timestamp check is skipped by design. My comment claiming "auto reload properly invalidates cache entries whose file changed on disk. That covers whatever restore rewrote" is not true for the one file class restore owns most directly.

Note that this is FINE. We assume that files in that directory are not modified across builds except they can be created by restore. The files that restore may modify in place on disk should be in obj/, not in NuGetPackageFolders.

[ViktorHofer]

Retracting the first major from my previous comment — it was wrong, and the reasoning is worth writing down because it's the crux of why this change is safe.

I claimed the FileClassifier.Shared.IsNonModifiable early-out in IsInvalidEntry was a hole, because NuGetPackageFolders is registered immutable and restore writes there. That conflates "restore writes into this directory" with "restore rewrites a file this cache already read". NuGet package content is immutable by version: restore extracts Foo/1.0.0 once and never rewrites content at that path afterwards. That property is the whole reason FileClassifier classifies the folder as immutable in the first place, and the rest of MSBuild already depends on it in more load-bearing places than this one. My failing scenario required locally repacking a package while reusing the same version and clearing the global-packages folder, which is already unsupported across long-lived MSBuild processes and is not something this PR makes worse.

So, going through everything a restore actually writes:

What restore writes	FileClassifier immutable?	Why a stale cache entry can't result
obj/*.nuget.g.props, obj/*.nuget.g.targets	No — under the project	Timestamp changes → IsInvalidEntry reloads it
~/.nuget/packages/**	Yes	Content is fixed by package version, never rewritten in place
SDK / VS install targets	Yes	A build does not write there at all

The only genuinely mutable thing restore produces is the generated imports next to the project, and those are exactly what the timestamp check covers. There is no category left where restore invalidates an entry that auto-reload would miss — which means the narrow IsInNugetCache eviction I floated as option (2) is solving a non-problem, and I've dropped it. Went with option (1): the early return stays, and the comment now states the real invariant instead of the vaguer (and, as written, false) "auto reload covers whatever restore rewrote".

Same correction applies to the deleted-import moderate I raised. Restore-generated imports are Condition="Exists(...)"-guarded, and FileUtilities.ClearFileExistenceCache() is still cleared unconditionally right below, so the Exists check re-evaluates for real and the import simply isn't taken. A stale PRE for a deleted file can linger in the weak cache but nothing reaches it. Documenting rather than coding around it.

The rest of the previous list stands and is being addressed: the end-to-end regression test (which is the thing that actually pins this invariant down, and matters more now that the argument rests entirely on the timestamp check), the public BuildRequestDataFlags.ClearCachesAfterBuild doc, a DebugTraceCache breadcrumb on the skip path, and the nits including the rename to ClearCachesAfterBuildIfNeeded.

[ViktorHofer]

Hah you are fast. I was just telling Copilot the same after it posted the expert reviewer analysis.

[ViktorHofer]

That said, I often updated nuget package cache files myself in the past when doing innerloop stuff / tests. I wonder if we need to come up with a better innerloop pattern or somehow force the cache invalidation in these cases. Or maybe just document that you need to kill the msbuild processes in such a case.

[rainersigwald]

Yeah, "use -nr:false or set the trait when doing quick-and-dirty dev loops over package contents" should be documented.

[ViktorHofer]

Is /nr:false actually sufficient? We now also spawn Server when nodereuse is false but terminate it at the end of the session.

EDIT: OK thinking about it, it should be. Cause the server wouldn't accept other session during that time as implemented today.

[rainersigwald]

Yeah, this kind of cache flush stuff will be one of the big design hurdles to get to a multiplexed server but I think it's fine now.

[ViktorHofer]

Agreed. And I think a /nr:false launched server should - by design - operated isolated, not accept other sessions and terminate at the end.

[ViktorHofer]

Code review — 🔴 Request changes

Reviewed restore-preserve-xml-cache against origin/main (merge base e02ab1fa8e). The optimization is the right idea and the change-wave plumbing is clean, but the invariant it rests on doesn't hold in two paths, and both are reachable from a normal restore. Verified by building the branch and running the new tests (4/4 pass), plus two end-to-end A/B repros against the wave.

---

🔴 Blocking 1 — a file deleted by restore is still served from the retained cache

src/Build/Evaluation/ProjectRootElementCache.cs (root cause at lines 179-186):

FileInfo fileInfo = FileUtilities.GetFileInfoNoThrow(projectFile);

// If the file doesn't exist on disk, go ahead and use the cached version.
// It's an in-memory project that hasn't been saved yet.
if (fileInfo == null)
{
    return false;          // <-- deletion is invisible to "reloads from disk"
}

IsInvalidEntry deliberately treats a missing file as valid. So "a cache that reloads changed files from disk already notices whatever an implicit restore rewrote" is true for rewrites but not for deletions. And Evaluator.ExpandAndLoadImports never pre-checks existence — it calls ProjectRootElement.OpenProjectOrSolution → cache.Get(...) first, and only consults FileSystems.Default.FileExists inside the catch (InvalidProjectFileException) handler (Evaluator.cs:2258-2264, 2326). A cache hit therefore never touches the disk at all.

This is reachable: NuGet deletes obj\<proj>.nuget.g.props / .nuget.g.targets when a project stops needing them (last PackageReference removed, RestoreProjectStyle change), and repo-custom Restore targets regenerate their import sets.

Repro — ProjectCollection(..., reuseProjectRootElementCache: true), ClearCachesAfterBuild on submission 1, plain build on submission 2, with a Restore target that <Delete>s the import:

--- wave 18.11 ENABLED (new behavior) ---
Import file exists after restore: False
Second build: Success
  WARN: OBSERVED PropertyFromImport=[before]   <-- stale; deleted file resurrected

--- MSBUILDDISABLEFEATURESFROMVERSION=18.11 (old behavior) ---
Second build: Failure
  ERROR: MSB4019 The imported project "...\generated.props" was not found.

Suggested fix: make the auto-reload path sweep rather than keep — walk _weakCache under _locker and ForgetEntry anything whose file is missing or whose timestamp moved, instead of relying on lazy per-read revalidation. That keeps the win for the unchanged majority and closes the hole.

---

🟠 Blocking 2 — the "immutable locations can't be invalidated by a restore" comment is not correct

// The entries it never revalidates are the ones
// under a location FileClassifier treats as immutable - the NuGet cache, where a package's content is
// fixed by its version and is never rewritten in place, ...

IsInvalidEntry short-circuits on FileClassifier.Shared.IsNonModifiable (lines 174-177), and FileClassifier.RegisterKnownImmutableLocations registers NuGetPackageFolders into _knownImmutableDirectories (src/Framework/FileClassifier.cs:280-288; isCustomLogicLocation: true only excludes it from the built-in logic snapshot, not from IsNonModifiable). Registration happens in RequestBuilder.cs:1543, i.e. after the first evaluation — so an entry can be cached while the folder is still "mutable" and then become permanently un-revalidatable.

"A package's content is fixed by its version and is never rewritten in place" is a NuGet convention, not an enforced invariant. It's violated by the standard local-package inner loop (repack a *-dev version, dotnet nuget locals global-packages --clear, restore) and is more likely still when RestorePackagesPath / NUGET_PACKAGES points at a repo-local folder. Same A/B: wave enabled → stale, wave disabled → correct.

Caveat on this one: my repro registers a scratch folder as NuGetPackageFolders, which is synthetic. What isn't synthetic is the code path (immutable ⇒ never revalidated) and the fact that the wave flips the observable result. At minimum the comment should stop asserting it's safe; better would be to exclude immutable-classified entries from the "keep" decision.

---

Non-blocking

Blast radius is wider than the description suggests. ClearCachesAfterBuild is on the public Microsoft.Build.Execution.BuildRequestDataFlags, and reuseProjectRootElementCache: true is a public ProjectCollection overload — third-party build servers and long-lived hosts get the new behavior too, and for them this flag was the documented way to force a re-read. Meanwhile in MSBuild.exe it's a no-op unless MSBUILDUSESERVER=1 (XMake.cs:1706; every other cache in the process is autoReloadFromDisk: false — BuildManager.cs:1614, BuildParameters.cs:244). Worth scoping the ChangeWaves.md entry and the XML doc to what actually changes.

Memory. The restore boundary was the only point the server node's cache was ever emptied — MSBUILDCLEARXMLCACHEONBUILDMANAGER is off by default and DiscardImplicitReferences early-returns when _autoReloadFromDisk. Steady state is now up to s_maximumStrongCacheSize (200) fully parsed XmlDocument graphs held strongly for the life of the node. That's a fine trade-off, but it should be stated.

No perf data. The ChangeWaves.md entry claims the following build "does not re-parse the import closure" — a measurement for the server scenario would help justify a wave feature.

No binlog signal. DebugTraceCache(...) only writes to Trace under MSBUILDDEBUGXMLCACHE=1. A stale-import report will arrive as "MSBuild used an old .props file" with a binlog that says nothing about whether the cache was kept or flushed. Consider a low-importance message.

Tests. Hygiene is good — ChangeWaves.ResetStateForTests() placement is right and nothing leaks (TestEnvironment.Cleanup unsets the env var and resets waves), collections are serialized, and the File.SetLastWriteTime(..., DateTime.Now.AddMinutes(-1)) trick is sound across filesystem granularities. The gap is coverage:

• ClearCachesAfterBuildKeptCacheStillSeesRewrittenImport only exercises rewrite; both regressions above live in the paths it doesn't touch. A test named after "the invariant the optimization rests on" passing while the invariant is broken is worth fixing.
• cache.TryGet(importPath).ShouldBeNull() isn't a passive probe — TryGet → Get → ForgetEntryIfExists mutates the cache before the third phase runs.
• ClearCachesAfterBuildStillClearsCacheThatDoesNotReloadFromDisk also passes if the cache was never populated; assert non-null before the flush.
• The two pure-cache tests would fit better in Evaluation/ProjectRootElementCache_Tests.cs (which already has GetProjectRootElementChangedOnDisk1/2), keeping one end-to-end test here for the wiring.

Thread safety (minor). DebugTraceCache("Keeping cache after build ...", _weakCache.Count) reads _weakCache outside _locker, and the argument is evaluated eagerly even when the env var is unset. Compare DiscardStrongReferences, which logs _strongCache.Count inside the lock.

Naming (nit). ClearCachesAfterBuildIfNeeded — plural for one cache, "IfNeeded" conveys no criterion, and the Evaluation-layer base class now carries a name borrowed from a BackEnd flag plus a doc comment about "restore". Something like InvalidateForExternalFileChanges() keeps the decision in the cache without importing the vocabulary.

Comments (nit). The three added blocks assert facts about NuGet, the SDK and VS — exactly the claims that rot, and one is already wrong. Trimming to the mechanism ("entries are revalidated by timestamp on read; entries under immutable locations are not") would age better.

---

What's clean

Change-wave discipline is solid: Wave18_11 is the current in-development wave, the gate polarity is right (!_autoReloadFromDisk || !AreFeaturesEnabled(...) ⇒ base ⇒ Clear()), both paths are tested, and it's documented. SimpleProjectRootElementCache correctly inherits the clearing default — the fail-safe direction is right. The doc-only change to the public enum needs no PublicAPI.Unshipped.txt update. Keeping FileMatcher.ClearCaches() / ClearFileExistenceCache() unconditional is correct and the comment explaining why is the best one in the diff. Commit sequence is tight and single-concern.

Suggested actions

• Handle deleted files — sweep the cache, or drop the optimization
• Handle FileClassifier-immutable entries, or stop asserting they're safe
• Regression tests for both (delete-during-restore ⇒ MSB4019; immutable-location rewrite ⇒ new content observed)
• Attach profiling data for the server scenario; state the memory trade-off
• Scope the ChangeWaves.md entry and public XML doc to what actually changes
• Add a binlog-visible kept-vs-cleared signal
• Move the _weakCache.Count trace inside _locker; strengthen the two weak test assertions

[ViktorHofer]

Follow-up: retracting one finding, narrowing the other

After discussion with @ViktorHofer, a correction to my review above.

❌ Retracting "Blocking 2" (FileClassifier-immutable locations)

This was wrong and I withdraw it. Immutability is a design contract, not an unproven assumption:

• FileClassifier's own documentation defines IsNonModifiable as "the file is not expected to change over time, other than when it is first created" (src/Framework/FileClassifier.cs:30-32).
• The locations it covers — the versioned .NET SDK installation and the versioned NuGet global packages folder — are immutable assets by design. Content is addressed by version; additions, deletions and in-place modifications are not supported operations.
• IsInvalidEntry has skipped revalidation for those paths for years, entirely independent of this PR. Anyone who mutates them is already operating outside the supported mode, and the existing remedies apply: the MSBUILDALWAYSDOIMMUTABLEFILESUPTODATECHECK escape hatch, or -nodeReuse:false for an isolated run.

So this PR doesn't introduce a hole here — it inherits an existing, deliberate contract. My review treated a documented design invariant as an unverified claim, which was the wrong lens. Apologies for the noise. The comment in ClearCachesAfterBuildIfNeeded describing this is accurate as written.

✅ "Blocking 1" (deletion) stands — and it is unrelated to immutability

Worth being explicit that the deletion case has nothing to do with the above: the file in question (obj\<proj>.nuget.g.props, or any restore-generated import) lives in a fully mutable location that is revalidated on every read. The gap is narrower and more mundane than my original write-up implied — IsInvalidEntry handles rewrite but not delete:

// ProjectRootElementCache.cs:181-186
FileInfo fileInfo = FileUtilities.GetFileInfoNoThrow(projectFile);

// If the file doesn't exist on disk, go ahead and use the cached version.
// It's an in-memory project that hasn't been saved yet.
if (fileInfo == null)
{
    return false;   // deliberately "valid" - for the unsaved in-memory-project case
}

That return false is correct for the case it was written for, but it means a deleted file is served from cache indefinitely, and Evaluator.ExpandAndLoadImports never probes existence before the cache lookup (it only consults FileSystems.Default.FileExists inside the catch (InvalidProjectFileException) handler).

I re-verified this myself, task-free, against this branch — ProjectCollection(..., reuseProjectRootElementCache: true), ClearCachesAfterBuild on submission 1, then re-evaluate:

===== scenario=delete   wave18.11=enabled  (this PR) =====
Restore submission:             Success
Import exists after restore:    False
Second evaluation:              Succeeded
  OBSERVED PropertyFromImport = [before]        <-- deleted file resurrected from cache

===== scenario=delete   wave18.11=disabled (today) =====
Import exists after restore:    False
Second evaluation:              Failed
  MSB4019: The imported project "...\generated.props" was not found.

===== scenario=rewrite  wave18.11=enabled  =====
  OBSERVED PropertyFromImport = [after]         <-- your invariant holds
===== scenario=rewrite  wave18.11=disabled =====
  OBSERVED PropertyFromImport = [after]

Rewrite behaves identically with and without the wave, exactly as the PR claims. Delete diverges.

Suggested fix, preserving the optimization: in the auto-reload branch of ClearCachesAfterBuildIfNeeded, sweep instead of unconditionally keeping — walk _weakCache under _locker and ForgetEntry any entry whose backing file no longer exists. That is O(cache size) once per restore, needs no re-parse of the surviving entries, and closes the only path where lazy revalidation cannot help. A regression test that deletes an import in the Restore target and asserts MSB4019 in the following build would lock it in.

Everything else in my original review is unchanged

Specifically still worth considering, all non-blocking: scoping the ChangeWaves.md entry and public XML doc to the configurations that actually change (server / hosts using reuseProjectRootElementCache: true), the memory note about the strong cache no longer being reset in a server node, perf data, a binlog-visible kept-vs-cleared signal, the _weakCache.Count read outside _locker, and the two weak test assertions.

[ViktorHofer]

Perf data + test hardening

Two follow-ups from the review: measurements for the optimization, and the test changes that don't depend on a product fix.

Perf: measured on this branch

I couldn't find a good home for this in src/MSBuild.Benchmarks — it's BenchmarkDotNet micro-benchmarks, and this needs a multi-submission BuildManager session — so this is a one-off harness rather than something to check in.

Setup. Bootstrap built from this branch. A host configured exactly like the MSBuild Server entry node (ProjectCollection(..., reuseProjectRootElementCache: true)), then per run: evaluate all projects (populating the cache the way the restore submission's own evaluation does) → one submission carrying ClearCachesAfterBuild → measure evaluating all projects again, which is the work the build after the restore has to do. Synthetic SDK-style net11.0 projects, so the import closure is the real ~250-file SDK closure. A/B is MSBUILDDISABLEFEATURESFROMVERSION=18.11. Each number is the median of 5 separate process launches, one restore+build cycle each.

projects	wave enabled (this PR)	wave disabled (today)	delta
1	57 ms	149 ms	−62% (2.6x)
25	1572 ms	1853 ms	−15%
100	4597 ms	5054 ms	−9%

So the claim in ChangeWaves.md holds, and the shape makes sense: the SDK closure is parsed once and shared across all projects in the build, so the saving is close to a constant (~100-450 ms of re-parsing) that a single-project build feels most and a large repo amortizes. Worth putting the single-project number in the PR description — for dotnet build on one project it's the difference between ~150 ms and ~57 ms of post-restore evaluation.

Memory. Peak working set was lower with the cache kept (81 MB vs 95 MB single project, 93 MB vs 95 MB at 100 projects), because discarding forces a re-parse that allocates a fresh set of XmlDocuments while the old ones are still awaiting collection. That's the opposite of what I asserted in my review — I withdraw the memory concern for the single-build case. The strong cache is capped at 200 entries (MSBUILDPROJECTROOTELEMENTCACHESIZE), so the retained set is bounded.

One anomaly worth a look, not a blocker. When I ran repeated restore+build cycles inside one long-lived process (which is the warm-MSBuild-Server shape, not the cold dotnet build shape), the 100-project case inverted — 6270 ms with the cache kept vs 5296 ms cleared, reproducible across two runs at 3 and 7 iterations. Sizes 1 and 25 still won in that mode. I don't have a mechanism, and it may well be an artifact of my harness's GC behaviour rather than anything real. But since the warm server is precisely the configuration this change targets, it seems worth someone confirming that a server node repeatedly building a large repo doesn't regress. Everything in the table above is from single-shot processes and is unaffected.

Tests: two assertions made load-bearing

Pushed as a separate commit — no product code touched, so this is independent of how the deletion issue gets resolved.

1. ClearCachesAfterBuildStillClearsCacheThatDoesNotReloadFromDisk asserted only TryGet(...).ShouldBeNull(), which also passes if a build of that shape never populated the cache. Added a control run of the same submission without the flag asserting the entry is present, so the discard is what the test actually measures. (The control passes, so the original assertion was testing the right thing — it just wasn't guarded.)

2. ClearCachesAfterBuildKeptCacheStillSeesRewrittenImport probed the rewritten import with cache.TryGet(importPath).ShouldBeNull() before the second build. That probe isn't passive — TryGet → Get → ForgetEntryIfExists performs the revalidation and eviction itself, so the eviction the test attributed to the following build had already happened by the time the build ran. Reordered to assert the end-to-end build first, then assert the cache holds the rewritten content:

result.OverallResult.ShouldBe(BuildResultCode.Success);

ProjectRootElement reloadedImport = cache.TryGet(importPath);
reloadedImport.ShouldNotBeNull();
reloadedImport.Properties.ShouldContain(property => property.Name == "PropertyFromImport" && property.Value == "after");

The helper now takes BuildRequestDataFlags and is renamed RunSubmission, since it no longer always passes ClearCachesAfterBuild.

All 4 tests still pass (--filter-method "*ClearCachesAfterBuild*" → 4/4).

Not done, since it needs the product change: the delete-during-restore regression test.

_{Unrelated to this PR: build.cmd currently fails for me in Microsoft.Build.EndToEnd.Tests (net472) with MSB3277, a System.Threading.Tasks.Dataflow 10.0.0.9 vs 10.0.0.10 conflict. I worked around it by building the Bootstrap target directly.}

[ViktorHofer]

Re-measured with the real MSBuild Server (warm) — the anomaly does not reproduce

My previous numbers came from an in-process approximation of the entry node, and the machine was busy while they ran. Redone properly against a warm MSBuild Server, with a noise-resistant methodology. Retracting the possible-regression note from that comment: with the real server there is no inversion at any size, in either parallelism mode.

Method

Bootstrap built from this branch. MSBUILDUSESERVER=1, msbuild dirs.proj -restore -t:Build, where dirs.proj MSBuild-tasks into all projects from both the Restore and Build targets — so the restore submission populates the XML cache exactly as NuGet's own restore evaluation does, and the build submission is the one that either re-parses the closure or doesn't. Synthetic SDK-style net11.0 projects, so it's the real ~250-file SDK import closure.

Because the box has ambient load (~29% from browsers/Teams), a straight sequential A/B was uselessly noisy — my first attempt spread from 4.3 s to 8.2 s and produced a spurious inversion. So: A/B interleaved in alternating blocks, MSBuild Server killed and restarted on every switch (so MSBUILDDISABLEFEATURESFROMVERSION applies to a fresh server), one discarded warm-up run after each restart, 3 timed runs per block, 5 blocks → 15 warm-server samples per configuration. Reporting min and p25 alongside median, since min is the least noise-contaminated statistic.

Results (warm server, ms)

scenario	stat	wave enabled (this PR)	wave disabled (today)	delta
1 project, -m:1	min	304	426	−122 (−29%)
	p25	326	451	−125 (−28%)
	median	347	509	−162 (−32%)
100 projects, -m:1	min	4165	4326	−161 (−3.7%)
	p25	4214	4372	−158 (−3.6%)
	median	4301	4507	−206 (−4.6%)
100 projects, default -m	min	4134	4298	−164 (−3.8%)
	p25	4257	4489	−232 (−5.2%)
	median	4507	4647	−140 (−3.0%)

Every order statistic in every configuration favours the change, and at 100 projects the two distributions barely overlap. The optimization is real and it is a wall-clock win on the warm server, which is the configuration it targets.

Reading the numbers

The saving is a near-constant ~120-230 ms regardless of repo size, which is exactly what you'd expect: the SDK import closure is parsed once and shared by every project in the build, so what the old Clear() cost was one re-parse of that closure per build, not one per project. That constant is ~29% of a warm single-project build and ~4% of a 100-project one.

That makes the single-project case the headline: dotnet build on one project after an implicit restore goes from ~430 ms to ~300 ms of warm-server build time. Suggest putting that in the PR description.

One thing worth knowing for future measurements

With default parallelism, most project evaluation happens on worker nodes, and OutOfProcNode has always constructed its cache with autoReloadFromDisk: true and never cleared it — so worker nodes already behaved the way this PR makes the entry node behave. The change only affects what the entry node itself evaluates. That's why the default--m and -m:1 deltas are so similar in absolute terms (~160 ms either way): it's the entry node's single re-parse of the closure being saved in both cases.

Correction to my earlier comments

• The 100-project "inversion" was a harness artifact. It only appeared when I ran many restore+build cycles inside one process with accumulated GC state; it does not occur with the real server. Disregard it.
• The memory observation stands and if anything is reinforced: nothing suggests keeping the cache costs working set, and the strong cache remains capped at 200 entries.

[ViktorHofer]

Perf follow-up: MSBuild Server + -mt (multithreaded)

Per request, I re-ran the A/B with -mt enabled alongside the server, and added same-tree server-only baselines so the two topologies can be compared apples-to-apples.

Why -mt is the interesting case

Verified the process topology under MSBUILDUSESERVER=1 -mt:

MSBuild.exe /nodemode:8                                  <- server node, does ALL the work in-proc
MSBuild.exe /nodemode:2 /nodereuse:True /low:False ...   <- task host (TaskRouter.NeedsTaskHostInMultiThreadedMode)

No /nodemode:1 worker nodes — as expected from Scheduler.cs:1543-1546, where MT sets
maxInProcNodeCount = MaxNodeCount and availableNodesWithOutOfProcAffinity = 0.

That matters here: in MT mode every project evaluation happens in the server process, i.e. in exactly
the ProjectRootElementCache this PR stops discarding. In non-MT server builds most evaluation happens on
out-of-proc worker nodes, whose caches were already autoReloadFromDisk: true and never cleared
(OutOfProcNode.cs:173), so only the entry node's re-parse was ever at stake.

Results

Warm server. dotnet msbuild dirs.proj -restore -t:Build [-mt], all times ms.
"on" = change wave 18.11 enabled (PR behavior), "off" = MSBUILDDISABLEFEATURESFROMVERSION=18.11 (today's behavior).

scenario	n	on min/p25/med	off min/p25/med	delta min/p25/med	paired median	runs favoring PR
1 proj, server only	12	583 / 593 / 618	740 / 754 / 783	−21.2% / −21.4% / −21.1%	−162 ms	12/12
1 proj, server + -mt	12	651 / 656 / 683	795 / 802 / 821	−18.1% / −18.2% / −16.8%	−131 ms	12/12
100 proj, server only	12	6800 / 7294 / 8002	7532 / 7548 / 8379	−9.7% / −3.4% / −4.5%	−526 ms	8/12
100 proj, server + -mt	24	4544 / 4879 / 5293	4708 / 5047 / 5778	−3.5% / −3.3% / −8.4%	−301 ms	21/24

Every order statistic in every configuration favors the change. No inversions.

Reading of the numbers

The absolute saving is essentially unchanged by -mt — ~130-160 ms at one project, ~300-530 ms at one
hundred. It scales strongly sublinearly with project count, which fits the mechanism: what is saved is one
re-parse of the shared SDK import closure (~250 files, parsed once and shared by all projects), plus the
project files and nuget.g.props/.targets themselves. Not per-project work.

The -mt percentage at 100 projects is smaller only because -mt is itself much faster on this tree
(median 5293 ms vs 8002 ms with the change on, ~34% faster). Same milliseconds saved, larger denominator.

So -mt does not change the conclusion, and does not regress anything. The one thing it does change is who
benefits: under -mt the saving is realized on the node that does all the evaluation, so the win is
structural rather than incidental to the entry node.

Methodology (and a correction to my own first attempt)

This box has ~29% ambient load, so straight sequential A/B is not usable. Protocol:

• Interleaved A/B blocks, with the leading config alternating per block to cancel monotonic drift.
• Server killed on every config switch, so MSBUILDDISABLEFEATURESFROMVERSION is picked up by a fresh
  server (ChangeWaves caches statically at first use — reusing the server across configs silently
  measures the wrong thing).
• Paired analysis by (block, run index) in addition to pooled order statistics.

Correction: my first -mt 100-project run used a single discarded warm-up and reported +1.9% at run
index 1. That was an artifact — the server keeps getting faster over its first ~4 builds (tiered JIT), so
run 1 was still on the ramp. Decomposing by run index made it obvious:

run index within block	on median	off median	delta
1	7657	7513	+1.9% (still ramping)
2	6915	7357	−6.0%
3	4872	5047	−3.5%

Raising the discarded warm-up to 3 runs per block removed it. All numbers in the table above use 3 warm-ups.

This also supersedes the 100-project anomaly I flagged two comments ago — that one came from an
in-process harness looping many build cycles with accumulated GC state, and does not reproduce against a
real server in any configuration, with or without -mt.

[ViktorHofer]

Post-merge note, for the record: I isolated the evaluation-only delta. Measured by summing ProjectEvaluationStarted/Finished spans from binlogs, warm server + -mt, interleaved A/B with the server restarted per config switch:

tree	metric	wave on	wave off	delta
1 project	evaluation	185 ms	298 ms	−113 ms (−37.9%)
1 project	wall clock	756 ms	876 ms	−121 ms (−13.8%)
100 projects	evaluation (union)	4626 ms	5097 ms	−471 ms (−9.2%)
100 projects	wall clock	9335 ms	9847 ms	−512 ms (−5.2%)

The useful part is the agreement between the two rows in each pair: evaluation drops by essentially the same absolute amount as total build time (−113 vs −121 ms, −471 vs −512 ms). So the entire win is evaluation, and nothing else got faster or slower as a side effect. Evaluation count is identical across configs (5 vs 5, and 302 vs 302), so it's the same work done cheaper — one avoided re-parse of the shared SDK import closure — rather than work being skipped.

That also explains why the headline percentages shrink with repo size: the saving is a near-constant number of milliseconds against a growing denominator, not something that scales with project count. Worth keeping in mind if anyone quotes the −21% single-project figure as a general expectation.

Two measurement caveats for anyone reproducing this:

• The server needs ~4 builds to reach steady state (tiered JIT). A single discarded warm-up produced a spurious +1.9% at 100 projects, which is what my earlier flagged "anomaly" was; it doesn't reproduce at steady state in any configuration.
• Under -mt, sum of evaluation durations is the wrong metric (302 overlapping evaluations absorb CPU contention and it reads +2%); the union of evaluation intervals is the meaningful one.