Make isolated graph builds fail deterministically on undeclared cross-project targets

[Copilot]

Fixes #14225

In -graph -isolate builds, a cross-project reference to a target that only has results because it ran as a DependsOnTargets dependency could be satisfied from a node-local (or in-proc shared) results cache when the referenced project happened to run on the same node, but miss and fail with MSB4252 when it ran on a different node. This made an incomplete ProjectReferenceTargets protocol scheduling-dependent — /m:1 passes, /m:8 intermittently fails.

Context

The presence of a result in the engine cache is the de-facto validation of the ProjectReferenceTargets protocol: the static graph pre-builds each node with its declared entry targets, so only those results land in the shared cache. However, dependency-produced target results also accumulate in a node's local cache and can satisfy an undeclared cross-project reference, but only under scheduling where parent and child share a node. The result of an isolated build should not depend on node scheduling.

Changes Made

Non-destructive gate: an isolated cross-project reference may only be satisfied from the results cache when the requested targets are among the referenced configuration's explicitly-requested (graph-declared) top-level targets. Otherwise it is treated as a cache miss, so the existing isolation constraint check fires and reports MSB4252 consistently.

• IResultsCache / ResultsCache / ResultsCacheWithOverride: added optional allowedTopLevelTargets parameter to SatisfyRequest. When set, all requested targets (or config default targets) must be contained in it (case-insensitive) or the request is reported unsatisfied.
• Scheduler: new GetIsolationAllowedTopLevelTargets helper threaded through TrySatisfyRequestFromCache and both cache-satisfaction sites (HandleRequestBlockedByNewRequests, ResolveRequestFromCacheAndResumeIfPossible).
• BuildRequestEngine.IssueBuildRequests: gated the node-local SatisfyRequest path the same way.
• Gate uses BuildRequestConfiguration.RequestedTargets and applies only under ProjectIsolationMode.True, for non-root, non-SkipStaticGraphIsolationConstraints requests, exempting self-references (a project building itself). Empty RequestedTargets falls back to existing behavior.

Testing

Manual repro from the issue (root/Parent/Child + fillers) now fails consistently with MSB4252 under both /m:1 and /m:8, instead of passing only under /m:1.

Notes

• Deliberately scoped to strict isolation (ProjectIsolationMode.True); MessageUponIsolationViolation and non-isolated builds retain existing behavior.
• The BuildRequestEngine.ReportConfigurationResponse cache path is intentionally not gated — its local cache is empty in the leak-relevant scenarios and its unresolved config's RequestedTargets is unreliable there.

[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.

[AlesProkop]

@copilot fix the failing checks

[Copilot]

@copilot fix the failing checks

Fixed. The failing checks were caused by a compile error in Scheduler.cs — the new GetIsolationAllowedTopLevelTargets helper had accidentally overwritten the signature of the existing CheckIfCacheMissOnReferencedProjectIsAllowedAndErrorIfNot method, breaking compilation of Microsoft.Build across all CI jobs. I restored the method signature and also added a missing <param> doc tag on ResultsCache.SatisfyRequest to clear CS1573 (an error under warnings-as-errors). Both Microsoft.Build and Microsoft.Build.Engine.UnitTests now build with 0 warnings / 0 errors.

[Copilot]

Pull request overview

This PR makes -graph -isolate builds fail deterministically when a project invokes an undeclared cross-project target, eliminating scheduling-dependent behavior where node-local cached dependency results could accidentally satisfy an invalid ProjectReferenceTargets protocol reference.

Changes:

• Adds an isolation-aware gate to results-cache satisfaction so only graph-declared top-level targets can be reused across projects in strict isolation (ChangeWave 18.10 gated).
• Threads the “allowed top-level targets” restriction through scheduler and build-request-engine cache satisfaction paths.
• Adds regression/unit tests and documents the new ChangeWave behavior.

Reviewed changes

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

Show a summary per file

File	Description
src/Framework/ChangeWaves.cs	Adds ChangeWave 18.10 to support opt-out for the new deterministic isolation behavior.
src/Build/BackEnd/Shared/BuildRequestConfiguration.cs	Computes “allowed top-level targets” for strict isolation cache satisfaction.
src/Build/BackEnd/Components/Scheduler/Scheduler.cs	Passes isolation “allowed targets” through scheduler cache satisfaction to enforce deterministic failures.
src/Build/BackEnd/Components/Caching/IResultsCache.cs	Extends SatisfyRequest API with optional allowedTopLevelTargets gate.
src/Build/BackEnd/Components/Caching/ResultsCache.cs	Implements the isolation gating check inside cache satisfaction.
src/Build/BackEnd/Components/Caching/ResultsCacheWithOverride.cs	Threads the new allowedTopLevelTargets argument through override cache implementation.
src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs	Applies the same isolation gate to node-local cache satisfaction in request issuance.
src/Build.UnitTests/Graph/IsolateProjects_Tests.cs	Adds regression tests covering deterministic MSB4252 behavior + ChangeWave opt-out behavior.
documentation/wiki/ChangeWaves.md	Documents ChangeWave 18.10 and the opt-out knob for this behavioral change.

[AlesProkop]

Expert review — PR #14280 · 9 files, +222/−12

Methodology: portable core + dotnet/msbuild expert-reviewer · 24/24 dimensions evaluated · Mode: Deep
Considered existing conversation (policy bot + "fix the failing checks" exchange) — no open review threads to reconcile. Source validated at PR head ce225e9.

Verdict: 🟡 COMMENT — address findings — 0 blocking, 0 major, 3 moderate, 2 nit · 20 of 24 evaluated dimensions clean

The fix is well-reasoned and correct in the scenarios I traced. A forced cache miss under strict isolation routes to CheckIfCacheMissOnReferencedProjectIsAllowedAndErrorIfNot, which emits MSB4252; the gate's exemption conditions line up with that method's allow-conditions, so a gated miss never degrades into a silent rebuild. RequestedTargets is the graph‑computed union of entry targets per node (ProjectGraph.GetTargetLists → one BuildRequestData per node), so declared references are not falsely rejected. Both cache-satisfaction sites (scheduler + BuildRequestEngine node-local) are gated, and the change is correctly opt-in behind ChangeWave 18.10.

---

⚠️ Needs attention (5)

🟡 MODERATE · Design Before Implementation — the gate's exemption set is duplicated from CheckIfCacheMissOnReferencedProjectIsAllowedAndErrorIfNot and must stay in lockstep, or a future edit silently converts an MSB4252 into an isolation‑violating rebuild. · src/Build/BackEnd/Shared/BuildRequestConfiguration.cs:186-206

• Scenario: GetIsolationAllowedTopLevelTargets restricts iff (Wave18_10 && isolation==True && non‑root && !SkipStaticGraphIsolationConstraints && !SkipNonexistentTargets && !self‑ref). CheckIfCacheMissOnReferencedProjectIsAllowedAndErrorIfNot (Scheduler.cs:2056) allows iff (isolation==False || root || SkipStaticGraphIsolationConstraints || SkipNonexistentTargetsIfExistentTargetsHaveResults || self‑ref). Today these are consistent, so a gated miss always yields the error. If someone later adds a new exemption to the check but not to the gate, the gate would force a miss for a case the check now allows → the reference gets rebuilt instead of failing, violating isolation without any diagnostic.
• Why it matters: the two methods encode the same invariant in two places; drift is silent and hard to catch (no test would fail loudly).
• Confidence: Medium · traced both condition sets; currently correct, this is a latent maintainability hazard.
• Fix: add a comment in each method pointing at the other and stating the "must stay in sync" invariant, or factor the shared predicate (isolation-restriction-applies) into one helper both call.

🟡 MODERATE · Test Coverage & Completeness — no test exercises a real -graph -isolate build or a multi‑parent (diamond) graph, so the "RequestedTargets == union of declared targets" assumption is unverified. · src/Build.UnitTests/Graph/IsolateProjects_Tests.cs:229-309

• Scenario: the new tests seed the cache with a single sequential BuildProjectFile(reference, ["EntryTarget"]) (matching existing convention in this file), never a GraphBuildRequestData at MaxNodeCount > 1. A diamond (project A references C for T1, project B references C for T2) would prove that C's config records the union [T1,T2] and that neither declared reference is falsely rejected.
• Why it matters: the whole fix rationale is graph‑scheduling determinism; the correctness of the gate rests on RequestedTargets being the full declared set, which is only produced by the graph union path — untested here.
• Confidence: Medium · verified the union is computed in ProjectGraph.GetTargetLists, but no automated test guards it for this feature.
• Fix: add a positive multi-parent graph test (declared target on a shared reference still succeeds under strict isolation) and, ideally, one end-to-end BuildRequest(GraphBuildRequestData) case.

🟡 MODERATE · Error Message Quality — the newly-deterministic MSB4252 gives no inline hint about the MSBUILDDISABLEFEATURESFROMVERSION=18.10 opt-out. · src/Build/Resources/Strings.resx (CacheMissesNotAllowedInIsolatedGraphBuilds)

• Scenario: a build that passed under /m:1 now fails with MSB4252 for a target (e.g. SpecialTarget) that did run as a dependency. The message is the generic isolation error; the only place the opt-out is documented is ChangeWaves.md.
• Why it matters: users hitting a new failure have no in-context path to the escape hatch or the "declare it via ProjectReferenceTargets" root cause.
• Confidence: Low · the message is shared by all isolation cache misses, so amending it has broader blast radius; documenting in ChangeWaves.md is the established convention, so this may be an acceptable trade-off.
• Fix: consider a dedicated message (or an appended note) for the change-wave-gated path only, mentioning both the opt-out and ProjectReferenceTargets; otherwise confirm the ChangeWaves.md entry is sufficient.

⚪ NIT · String Comparison Correctness — ContainsIgnoreCase compares target names with StringComparison.OrdinalIgnoreCase; repo convention (dimension 7, rule 1) prefers MSBuildNameIgnoreCaseComparer for target-name comparisons. · src/Build/BackEnd/Components/Caching/ResultsCache.cs:326-338

• Fix: use MSBuildNameIgnoreCaseComparer.Default.Equals (functionally equivalent, matches convention), and consider replacing the hand-rolled ContainsIgnoreCase loop with a set/Contains using that comparer if the allowed set is ever large. (Confidence: High · functionally correct today; convention/consistency only.)

⚪ NIT · Test Coverage & Completeness (hygiene) — UndeclaredTargetSatisfiedFromDependencyResultFailsDeterministically relies on Wave18_10 being enabled by default but, unlike its companion, does not call ChangeWaves.ResetStateForTests() to guard against polluted global change-wave state. · src/Build.UnitTests/Graph/IsolateProjects_Tests.cs:229-239

• Fix: reset change-wave state at the start of the deterministic-failure test too, so it can't be perturbed by a parallel test class that set MSBUILDDISABLEFEATURESFROMVERSION.

---

✅ Clean (20)

✅ Backwards Compatibility — opt-in via ChangeWave 18.10; new SatisfyRequest param is optional (= null); only ProjectIsolationMode.True is affected, MessageUponIsolationViolation/non-isolated unchanged.
✅ ChangeWave Discipline — Wave18_10 is the next release after 18_9, added to AllWaves; both enabled and disabled paths are tested; documented in ChangeWaves.md.
✅ Performance & Allocation — gate returns the existing RequestedTargets collection (no new alloc); the target check is a small linear scan run only per cross-project reference request, not a hot path.
✅ Logging & Diagnostics Rigor — no logging changes; existing TraceScheduler trace on cache satisfaction retained.
✅ API Surface Discipline — all touched members are internal; the interface change is a defaulted optional parameter.
✅ MSBuild Target Authoring Conventions — no product targets authored; only inline test project XML.
✅ Design (structure) — helper centralized on BuildRequestConfiguration and reused by both the scheduler and the engine; see MODERATE above for the cross-method invariant.
✅ Cross-Platform Correctness — project-path equality uses OrdinalIgnoreCase consistent with the existing self-reference check in the same area; the test accounts for the macOS temp-path discrepancy.
✅ Code Simplification — control flow is straightforward and early-returns cleanly.
✅ Concurrency & Thread Safety — the gate reads request.Targets/RequestedTargets (effectively immutable after config creation) before the existing _resultsByConfiguration lock; no new shared mutable state.
✅ Naming Precision — GetIsolationAllowedTopLevelTargets / allowedTopLevelTargets clearly convey intent.
✅ SDK Integration Boundaries — no SDK/common-targets changes.
✅ Idiomatic C# Patterns — matches the surrounding #nullable disable files (no ?), while ResultsCacheWithOverride.cs correctly uses ? under its nullable-enabled context.
✅ File I/O & Path Handling — no file I/O introduced.
✅ Documentation Accuracy — thorough XML <summary>/<remarks> on the new members plus the ChangeWaves.md entry; rationale (the "why") is well captured.
✅ Build Infrastructure Care — no build/CI/dependency-manifest changes.
✅ Scope & PR Discipline — single concern; net change is 9 files.
✅ Evaluation Model Integrity — no evaluation/expander changes.
✅ Correctness & Edge Cases — verified: forced miss → MSB4252; empty/self-reference/SkipNonexistentTargets fallbacks preserve prior behavior; override-cache exports are already target-filtered so the gate is a safe no-op there.
✅ Dependency Management / Security Awareness — no dependency, NuGet feed, or security-surface changes.

---

☑️ Fix checklist

• 🟡 Design — cross-reference (or unify) the gate's exemptions with CheckIfCacheMissOnReferencedProjectIsAllowedAndErrorIfNot and document the sync invariant (MODERATE)
• 🟡 Test Coverage — add a multi-parent/diamond graph positive test (and ideally a real GraphBuildRequestData case) (MODERATE)
• 🟡 Error Message — decide whether MSB4252's new deterministic path should hint the 18.10 opt-out / ProjectReferenceTargets, or confirm ChangeWaves.md suffices (MODERATE)
• ⚪ String Comparison — use MSBuildNameIgnoreCaseComparer for the target-name comparison (NIT)
• ⚪ Test hygiene — reset change-wave state in the deterministic-failure test (NIT)

_{🤖 Generated with the ultimate-reviewer skill (portable core + dotnet/msbuild expert-reviewer). Findings are advisory; a human should confirm before acting.}

[AlesProkop]

Expert review update — PR #14280 @ 4408138

I re-reviewed the current head after the latest fixes. I found one issue that should be addressed before merge.

Major: persisted input/output caches bypass the new strict-isolation allow-list

BuildRequestConfiguration.GetIsolationAllowedTopLevelTargets now relies on requestConfig.RequestedTargets to decide which top-level targets may be satisfied from cache in strict isolated graph builds. If that collection is empty, the method returns null, and ResultsCache.SatisfyRequest skips the allow-list check entirely.

That is fine for live node packets, because Translate includes _requestedTargets. It is not fine for the persisted cache format: ConfigCache serializes configurations through BuildRequestConfiguration.TranslateForFutureUse, and that method still omits _requestedTargets.

Relevant lines:

• src/Build/BackEnd/Shared/BuildRequestConfiguration.cs:211 returns null when RequestedTargets is empty.
• src/Build/BackEnd/Shared/BuildRequestConfiguration.cs:1018 serializes _requestedTargets for normal node transfer.
• src/Build/BackEnd/Shared/BuildRequestConfiguration.cs:1027 starts TranslateForFutureUse, which does not serialize _requestedTargets.
• src/Build/BackEnd/Components/Caching/ResultsCache.cs:177 only enforces the allow-list when allowedTopLevelTargets != null.
• src/Build/BackEnd/BuildManager/CacheSerialization.cs:76 trims target results only for MessageUponIsolationViolation, not strict ProjectIsolationMode.True.

Concrete scenario: build a referenced project in strict isolation with an output cache, where the graph-declared entry target is EntryTarget and EntryTarget DependsOnTargets="SpecialTarget". The output cache can contain both EntryTarget and the dependency-produced SpecialTarget, but the persisted configuration loses the fact that only EntryTarget was requested. A later strict isolated build that consumes this as an input cache and asks the referenced project for undeclared SpecialTarget gets allowedTopLevelTargets == null, so the new deterministic miss/MSB4252 path is bypassed and the undeclared target can be satisfied from the input cache.

Recommendation: persist _requestedTargets in TranslateForFutureUse as part of the output/input cache format, or explicitly treat missing requested-target data as restrictive for strict isolation rather than as “no restriction”. Please add a regression test that writes a strict-mode output cache for EntryTarget -> SpecialTarget, then consumes it from a root project that calls undeclared SpecialTarget and expects MSB4252.

I did not find other blocking issues in the 9-file PR diff. I did not run the test suite for this review.

[AlesProkop]

Expert review update — PR #14280 @ 64146a7

I re-reviewed the current head after the persisted-cache fix. No remaining actionable findings from the 10-file PR diff.

The previous major issue is addressed: BuildRequestConfiguration.TranslateForFutureUse now persists _requestedTargets, so the strict-isolation allow-list used by GetIsolationAllowedTopLevelTargets survives the input/output cache serialization path instead of coming back empty and disabling the deterministic MSB4252 gate.

Relevant fixed coverage:

• src/Build/BackEnd/Shared/BuildRequestConfiguration.cs:1039-1044 now serializes _requestedTargets in the future-use cache format.
• src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs:716 adds TestRequestedTargetsPreservedAcrossTranslateForFutureUse.
• src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs:743 and :752 assert the requested target is preserved before and after the future-use round trip.

Validation I ran:

• TestRequestedTargetsPreservedAcrossTranslateForFutureUse passed locally through the VS Code test runner.
• git diff --check 9ffbeaad0469d33a951897865c5c7541bbd85354...HEAD is clean.

Residual risk: the new test directly covers the serialization member that caused the cache bypass, and the existing graph test covers the in-memory cache leak path. I still do not see a full end-to-end test that writes a strict-mode output cache and consumes it later as an input cache, but I do not consider that a blocking gap now that the serialization path itself is covered.