Make source-build-assets MSBuild tasks multi-threading safe - #1776
Closed
ViktorHofer wants to merge 2 commits into
Closed
Make source-build-assets MSBuild tasks multi-threading safe#1776ViktorHofer wants to merge 2 commits into
ViktorHofer wants to merge 2 commits into
Conversation
Migrates the 7 MSBuild tasks in Microsoft.DotNet.SourceBuild.Tasks.XPlat and PackageSourceGeneratorTask to the multithreaded task model, matching the migration already done for Arcade and the VMR eng/tools tasks. - Annotate each task with [MSBuildMultiThreadableTask], implement IMultiThreadableTask, and resolve every path through TaskEnvironment. - Enable Microsoft.Build.TaskAuthoring.Analyzer on the two task projects (excluded from source-only builds) with a scoped globalconfig, so future tasks and edits keep the multithreading contract. The analyzer is added as a Maestro-flowed dependency alongside the other Microsoft.Build packages, so it stays in lockstep with the MSBuild version it validates against. - Wrap file I/O the analyzer cannot see because it is not a BCL path API: NuGet PackageArchiveReader, NuspecReader and AssemblyName.GetAssemblyName. - In GenerateProject.BuildPackagingItems, resolve the enumeration root once and rename it to absoluteProjectDirectory. Enumeration and Path.GetRelativePath must share the same absolute base; mixing a resolved root with a raw one silently produces wrong relative paths rather than failing. No behavioral change when paths are already absolute: GetAbsolutePath returns rooted input unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Migrates seven MSBuild tasks to safe in-process multithreaded execution.
Changes:
- Adds multithreadable task annotations, environment injection, and explicit path resolution.
- Adds and configures the task-authoring analyzer.
- Adds the analyzer to Maestro-managed dependency versions.
Show a summary per file
| File | Description |
|---|---|
UpdateFrameworkList.cs |
Resolves framework-list paths safely. |
PackageSourceGeneratorTask.csproj |
Enables the task analyzer. |
NormalizeIL.cs |
Makes IL normalization multithreadable. |
GetPackageItems.cs |
Resolves package and assembly paths. |
GenerateProject.cs |
Updates project generation path handling. |
AddSbrpAttribute.cs |
Makes IL rewriting multithreadable. |
Version.Details.xml |
Adds the analyzer dependency. |
Version.Details.props |
Exposes the analyzer version property. |
UpdateJson.cs |
Resolves JSON paths safely. |
Microsoft.DotNet.SourceBuild.Tasks.XPlat.csproj |
Enables the task analyzer. |
AddSourceToNuGetConfig.cs |
Resolves NuGet configuration paths. |
MultiThreadableTaskAnalyzer.globalconfig |
Configures analyzer scope and diagnostics. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
TargetPath and ReferencePackagesRoot/TextOnlyPackagesRoot are independent task inputs, so they can differ in rootedness. Path.GetRelativePath expands each operand with Path.GetFullPath, so a relative operand was resolved against the process current directory. That happens to match the project directory today, but not under -mt, where it would silently emit a wrong ProjectReference path. Resolving only one side would have been worse than resolving neither, so make projectDirectory absolute and resolve the dependency path at the call site. The pairing is now correct for every combination of rooted and relative inputs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ViktorHofer
added a commit
to dotnet/msbuild
that referenced
this pull request
Aug 24, 2026
…ration (#14795) Encodes into the migration skill and reviewer agent the defect classes found while migrating ~150 tasks to `[MSBuildMultiThreadableTask]` across dotnet/arcade ([arcade#17381](dotnet/arcade#17381)), dotnet/source-build-assets ([#1776](dotnet/source-build-assets#1776)) and the dotnet/dotnet VMR. ### Why `Microsoft.Build.TaskAuthoring.Analyzer` was enabled in all three repos and every build was **0 warnings / 0 errors**. A manual audit afterwards still found **9 real defects**: | Why it was missed | Count | |---|---| | Code outside the analysis scope (unannotated base class) | 2 | | API not on the monitored list (`AssemblyName.GetAssemblyName`) | 3 | | Failure class not modeled at all (task-object race, memoized failure, nested task construction, path crossing a DI boundary) | 4 | Every one of these was findable by reading the code with the right checklist — so they belong in the skill. Analyzer gaps are filed separately against #14772 (#14791, #14792, #14793) plus #14794 for a runtime forcing function. ### Changes **SKILL.md** - **Sin 8 — swallowed exceptions hiding an unresolved path.** Where a `catch` means a *semantic answer* ("not an assembly") rather than "fail", an unresolved path produces a silently wrong result instead of an error. In the VMR's `CheckForPoison` this skipped both poison checks, turning a leaked binary into a false negative in source-build's leak-detection gate — with a green build. - **Analyzer-invisible path consumers.** `MSBuildTask0003` monitors 8 types; anything else taking a path string is equally unsafe and silent. `AssemblyName.GetAssemblyName` on a raw input was the most-repeated defect of the whole exercise. Notes the string-vs-stream overload distinction. - **Unsafe code in an unannotated base class.** The attribute is `Inherited = false`, so bases are unannotated — and under `scope = multithreadable_only` they are never analyzed, despite executing multithreaded. Real case: `AkaMSLinksBase` held `File.ReadAllText` on a raw task input while both derived tasks were annotated and clean. - **Engine-owned shared state: `RegisterTaskObject`.** Non-atomic read/write with no `static` field in sight, so it reads as thread-safe. Includes the benign/broken decision table — `LocateDotNet` is fine, `SingleError` was not. - **Paths crossing interface / DI boundaries**, where no `System.IO` type appears in the task at all. - **New "Verification" section** — a clean analyzer run is not a migration, with the miss breakdown above and why "it worked when I tested it" is weak evidence for a failure mode that is load- and schedule-dependent. - **New "Migrating Is Not Always the Right Answer" section** — leaving a task unannotated preserves today's TaskHost behavior exactly and is a supported outcome. A slower task beats a wrong one. - Sign-off checklist extended with the above; fixed a pre-existing broken anchor to Sin 2. **mt-migration-reviewer.agent.md** - Operating rule 7: never accept a clean analyzer run as a basis for approval. - Step 1 now walks the base chain, `catch` blocks, caches and DI boundaries. - Step 2 hazard table gains analyzer-invisible APIs, DI boundaries, semantic-answer catches and the `RegisterTaskObject` pair. Also **corrects an existing row**: `AssemblyName.GetAssemblyName` was listed only under Sin 2 message leakage, missing that the call is itself CWD-dependent. - Step 5: explicitly approve a justified decision *not* to migrate, so the agent does not create an incentive to maximize annotation count. Docs only — no product code, no tests affected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Given that all of those are invoked out-of-process via the TaskHostFactory, this change doesn't improve perf. What we should do though is moving the two tasks that are used here and in the VMR orchestrator into Arcade so that we can benefit from them being precompiled so that they can run in an multi-threaded mode in msbuild. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrates the 7 MSBuild tasks in
Microsoft.DotNet.SourceBuild.Tasks.XPlatandPackageSourceGeneratorTaskto MSBuild's multithreaded task model.This is the source-build-assets counterpart to dotnet/arcade#17381; the same migration has already been applied to Arcade and to the VMR's own
eng/toolstasks.Why
In MSBuild's multithreaded mode (
-mt), a task that is not declared multithread-safe is routed to an out-of-proc sidecar TaskHost. Every invocation then pays a cross-process marshalling round-trip, which erodes most of the benefit of the mode. Opting a task in requires two independent signals:[MSBuildMultiThreadableTask]IMultiThreadableTaskTaskEnvironmentinjectionBecause a multithreaded build has no per-thread current directory, tasks must resolve relative paths explicitly through
TaskEnvironment.GetAbsolutePathrather than relying onEnvironment.CurrentDirectory.What changed
[MSBuildMultiThreadableTask], implementedIMultiThreadableTask, and resolved every path throughTaskEnvironment.Microsoft.Build.TaskAuthoring.Analyzeron the two task projects (with a scopedglobalconfig) so future tasks and edits keep the contract. The analyzer is added as a Maestro-flowed dependency alongside the otherMicrosoft.Buildpackages, so it stays in lockstep with the MSBuild version it validates against. It is excluded from source-only builds.PackageArchiveReader,NuspecReader, andAssemblyName.GetAssemblyName.GenerateProject.BuildPackagingItems, resolved the enumeration root once and renamed it toabsoluteProjectDirectory.Notable fix
GenerateProject.BuildPackagingItemsenumerated files from a resolved absolute root but then computed relative paths against the raw, unresolved one. Enumeration andPath.GetRelativePathmust share the same base - mixing a resolved root with a raw one silently produces wrong relative paths rather than throwing. This class of bug is invisible to the analyzer, so the diff was additionally checked by hand for every site where a path feeds both I/O and string prefix math.Risk
Low.
GetAbsolutePathreturns rooted input completely unchanged (no normalization, trailing separators preserved), so wrapping is a provable no-op wherever the input is already absolute - which is the case for all MSBuild-supplied item paths today. Behavior only changes for relative inputs, which previously depended on the process-wide current directory.Note that
File.Exists/Directory.Existstolerate null/empty whileGetAbsolutePaththrows, so those call sites either rely on an existing short-circuit or add an explicitIsNullOrEmptyguard.Validation
Both task projects build with 0 warnings / 0 errors, including the newly enabled analyzer. The changes were additionally run through an independent invariant checker (verifying that every annotated task implements the interface and that no path is used for both I/O and prefix comparison in resolved/unresolved form), which reports no findings.
Part of dotnet/msbuild#13073.