Serialize BuildRequestConfiguration.RequestedTargets to fix solution metaproject MSB4057 in parallel builds

[ViktorHofer]

Unblocks dotnet/dotnet#7491

Summary

Fixes a bug where building a solution that requests a non-default target (e.g. Pack) fails with MSB4057: The target "Pack" does not exist in the project in parallel / multithreaded builds, while the same build succeeds serially.

This was observed building the dotnet/dotnet VMR (Arcade builds the solution in two phases with different __BuildPhase global properties; the SolutionBuild phase requests Build;Pack).

Root cause

BuildRequestConfiguration.RequestedTargets was a get-only auto-property that was never serialized in Translate. In parallel/MT builds the configuration is round-tripped through translation (even when the work appears to run in-proc on node 1), so the deserialized configuration reset RequestedTargets to an empty collection.

SolutionProjectGenerator only emits user-requested targets (such as Pack) into the generated .slnx.metaproj when they are present in config.RequestedTargets. With an empty set, Pack was omitted from the metaproject and the build failed with MSB4057. Serial builds never serialized the configuration, so they were unaffected.

Fix

Back RequestedTargets with a List<string> field and translate it in BuildRequestConfiguration.Translate.

Validation

Reproduced end-to-end against the dotnet/dotnet VMR (symreader). A/B with patched MSBuild deployed into the VMR SDK:

Serialization	targets reaching SolutionBuild generation	Result
disabled (before)	[]	metaproject lacks Pack -> MSB4057
enabled (this fix)	[Build;Pack]	metaproject contains Pack -> build succeeds

Added regression test TestTranslationPreservesRequestedTargets; full *TestTranslation* suite passes (56/56, net10.0 + net472).

[Copilot]

Pull request overview

Fixes a backend serialization bug where solution builds requesting non-default targets (for example Pack) can fail with MSB4057 in parallel/MT builds because BuildRequestConfiguration.RequestedTargets was not preserved across node-boundary translation.

Changes:

• Back RequestedTargets with a List<string> field and serialize it in BuildRequestConfiguration.Translate.
• Add a regression unit test ensuring RequestedTargets round-trips through translation.

Show a summary per file

File	Description
src/Build/BackEnd/Shared/BuildRequestConfiguration.cs	Persist requested target names across translation by backing with a field and translating it.
src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs	Add regression test to ensure RequestedTargets survives translation round-trip.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 4

[github-actions]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Design note — TranslateForFutureUse does not include _requestedTargets

TranslateForFutureUse (line 976) is a second serialization path used by ConfigCache to persist project-evaluation metadata across processes. It intentionally serializes a different, non-overlapping field set (_projectDefaultTargets, _projectInitialTargets, _projectTargets, etc.) and does not include _requestedTargets.

This is correct for the ConfigCache use-case: those deserialized configs are used for project-lookup metadata, not for executing build requests, so their RequestedTargets being empty ([]) is fine in that path.

However, it is worth noting that BuildManager.cs:1769 consumes config.RequestedTargets directly when generating the solution metaproject, and a config that arrived via the ConfigCache deserialization path would always present empty RequestedTargets. This is a pre-existing gap (it was never serialized by either path before this PR) rather than a regression, but a follow-up issue tracking the TranslateForFutureUse path — or an explicit comment in code — would help prevent future confusion about why the two paths diverge.

Generated by Expert Code Review (on open) for #14223 · 1.7K AIC · ⊞ 30.3K · ◷

[github-actions]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Code Review — PR #14223 · 24/24 dimensions checked ✅

Verdict: COMMENT — no blocking issues. The fix is correct, well-targeted, and the regression test directly validates the failing scenario.

---

Confirmed findings

Severity	Dimension	Finding
NIT	Idiomatic C# (dim 16)	new[] { "Build", "Pack" } used 3× in the new test — repo standard requires ["Build", "Pack"]. Inline comment posted on line 272.
Note	Correctness / Design (dims 10, 22)	TranslateForFutureUse (lines 976–987, the ConfigCache serialization path) does not serialize _requestedTargets. This is a pre-existing gap not introduced by this PR — RequestedTargets was never serialized there before either. Now that this field is recognized as load-bearing, it's worth a follow-up issue to evaluate whether the cache path also needs it. BuildManager uses config.RequestedTargets at several call sites; a cache-deserialized config would return [].

---

All other dimensions: ✅ LGTM

• Backwards Compatibility: Pure bug fix — no new warnings, no API changes, no behavior change for serial builds.
• ChangeWave: Not required; this restores correct behavior that was silently broken.
• Performance: List<string> of 1–5 target names serialized per node-crossing config. Allocation is negligible; = [] field initializer is overwritten in every non-deserialization constructor path.
• Concurrency: _requestedTargets is written only during construction/deserialization, never mutated afterward. The copy-constructor alias (_requestedTargets = other._requestedTargets) is safe under that immutability contract.
• Wire-format compatibility: The new field is appended at the end of Translate. Versioned handshake prevents mixed-version nodes from communicating, so no protocol break.
• Test coverage: TestTranslationPreservesRequestedTargets covers the exact serialization round-trip described in the bug. The empty-target-list path is covered implicitly by the existing TestTranslation (config built without target names round-trips with _requestedTargets = []).
• API surface: RequestedTargets remains internal; _requestedTargets is private.
• Scope: Single-concern PR, minimal diff.

Generated by Expert Code Review (on open) for #14223 · 1.7K AIC · ⊞ 30.3K · ◷

[github-actions]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Review Summary

The fix is correct and minimal. RequestedTargets is now properly round-tripped through Translate, which is exactly what the IPC path required. Root-cause analysis, code change, and regression test all align cleanly.

#	Dimension	Verdict
3	Performance & Allocation	🟡 1 NIT
4	Test Coverage & Completeness	🟡 2 NITs
10	Design Before Implementation	🟡 1 pre-existing MODERATE (out of scope)
16	Idiomatic C# Patterns	🟡 1 NIT

✅ 20/24 dimensions clean — all findings are NIT or pre-existing gaps.

Checklist:

• Backwards compatibility: pure bug fix, no new warnings/errors
• ChangeWave: not needed — restores correct behaviour, no user-visible opt-in/opt-out needed
• Concurrency: _requestedTargets is effectively immutable after construction, no mutations found anywhere in the class; aliased copy-constructor reference is safe
• Wire format: new field appended at end of Translate; mixed-version nodes are prevented by version-locked handshake before any packet exchange
• TranslateForFutureUse: correct to omit _requestedTargets — that path is for project-evaluation metadata lookup, not build-request execution

Minor action items (all NIT):

• Replace new[] { "Build", "Pack" } with ["Build", "Pack"] (C# 14 collection expressions) — three occurrences in TestTranslationPreservesRequestedTargets
• Add deserializedConfig.RequestedTargets.ShouldBeEmpty() to the existing TestTranslation to explicitly verify the empty-list round-trip (since Assert.Equal uses InternalEquals which does not compare RequestedTargets)

Generated by Expert Code Review (on open) for #14223 · 1.7K AIC · ⊞ 30.3K

[ViktorHofer]

Thanks! I addressed the three collection-expression suggestions in 4b81ddb.

On the version-gating comment: NegotiatedPacketVersion gating is used for packets that can cross an MSBuild version boundary — specifically TaskHostConfiguration, which is sent to task-host processes that may be a different MSBuild version (e.g. the .NET Framework MSBuildTaskHost.exe). BuildRequestConfiguration is only translated between the scheduler and worker nodes, which are version-locked: a worker node whose version does not match the host fails the handshake and is not reused (a fresh node is spawned instead). Consistent with that, every existing field in BuildRequestConfiguration.Translate (e.g. _projectEvaluationId, _savedEnvironmentVariables) is translated unconditionally with no version gating, so the new field follows the established pattern for this packet type.

[ViktorHofer]

Thanks for the thorough review. Addressed the follow-ups:

• Empty RequestedTargets round-trip coverage — added deserializedConfig.RequestedTargets.ShouldBeEmpty(); to TestTranslation (e85a2ef), since InternalEquals intentionally ignores RequestedTargets and would not have caught an empty-list serialization regression.

• TranslateForFutureUse / ConfigCache path — agreed this is a pre-existing gap rather than a regression (the field was never serialized by either path before). For solution metaproject generation the config always comes from the BuildRequestData constructor, so RequestedTargets is populated there; I've kept this PR scoped to the node-translation fix and we can file a follow-up to evaluate whether the cache path needs it too.

• Field-initializer allocation nit — I'm keeping private List<string> _requestedTargets = []; as-is. Config construction is coarse-grained (per project request, not a per-item/per-property hot path), the discarded allocation is a single empty List<string> (no backing array), and the initializer guarantees the non-null IReadOnlyCollection contract for every current and future constructor. Scattering the initialization across constructors would trade negligible perf for a real null-safety risk, so the initializer is the safer choice here.

[ViktorHofer]

/backport to vs18.9

[github-actions]

Started backporting to vs18.9 (link to workflow run)