Make MSBuild tasks safe for multi-threaded execution - #17381
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prepares Arcade MSBuild tasks for in-process multithreaded execution through task annotations, project-scoped environment handling, analyzer enforcement, and concurrency fixes.
Changes:
- Updates MSBuild dependencies and adds analyzer guardrails.
- Migrates tasks to
TaskEnvironmentand removes the obsolete shared task base. - Addresses shared caches, serializers, and other concurrency hazards while leaving unsafe task chains unannotated.
Show a summary per file
| File | Review result |
|---|---|
src/Microsoft.DotNet.XliffTasks/Tasks/XlfTask.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.XliffTasks/Tasks/UpdateXlf.cs |
Moderate: exception handling still compares against a potentially relative path. |
src/Microsoft.DotNet.XliffTasks/Tasks/TranslateSource.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.XliffTasks/Tasks/TransformTemplates.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.XliffTasks/Tasks/SortXlf.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.XliffTasks/Tasks/GatherXlf.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.XliffTasks/Tasks/GatherTranslatedSource.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.XliffTasks/Tasks/EnsureAllResourcesTranslated.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.XliffTasks/Model/Document.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.SwaggerGenerator/Microsoft.DotNet.SwaggerGenerator.MSBuild/GenerateSwaggerCode.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.SourceBuild/tasks/src/WriteBuildOutputProps.cs |
Moderate: relative asset directories are checked against the process directory. |
src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WriteUsageReports.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WritePackageUsageData.cs |
Moderate: RootDir is not normalized consistently. |
src/Microsoft.DotNet.SourceBuild/tasks/src/ReadNuGetPackageInfos.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.SignTool/src/SignToolTask.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.SignCheckTask/SignCheckTask.cs |
Critical: the shared verifier registry is not safe for concurrent or repeated construction. |
src/Microsoft.DotNet.SharedFramework.Sdk/src/ValidateFileVersions.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.SharedFramework.Sdk/src/GenerateSharedFrameworkDepsFile.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.SharedFramework.Sdk/src/GeneratePlatformManifestEntriesFromTemplate.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.SharedFramework.Sdk/src/GeneratePlatformManifestEntriesFromFileList.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.SharedFramework.Sdk/src/FileUtilities.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.SharedFramework.Sdk/src/CreateFrameworkListFile.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.SharedFramework.Sdk/Microsoft.DotNet.SharedFramework.Sdk.csproj |
Reviewed; no final comment. |
src/Microsoft.DotNet.PackageTesting/VerifyTypes.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.PackageTesting/VerifyClosure.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.PackageTesting/Microsoft.DotNet.PackageTesting.csproj |
Reviewed; no final comment. |
src/Microsoft.DotNet.PackageTesting/GetCompatiblePackageTargetFrameworks.cs |
Critical: static mutable framework collections are shared across executions. |
src/Microsoft.DotNet.NuGetRepack/tests/VersionUpdaterTests.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.NuGetRepack/tasks/src/UpdatePackageVersionTask.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.NuGetRepack/tasks/src/ReplacePackageParts.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.NuGetRepack/tasks/src/NuGetVersionUpdater.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/WaitForHelixJobCompletion.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/StopAzurePipelinesTestRun.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/StartAzurePipelinesTestRun.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/SendHelixJob.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/InstallDotNetTool.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/GetHelixWorkItems.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/FindDotNetCliPackage.cs |
Critical: the process-wide client can be overwritten or disposed during concurrent execution. |
src/Microsoft.DotNet.Helix/Sdk/DownloadFromResultsContainer.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/CreateXUnitWorkItems.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAppleWorkItems.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAndroidWorkItems.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/CreateMTPWorkItems.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/CreateFailedTestsForFailedWorkItems.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/CheckHelixJobStatus.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/CheckAzurePipelinesTestResults.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Helix/Sdk/CancelHelixJob.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.GenFacades/TypeParser.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.GenFacades/SourceGenerator.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.GenFacades/RoslynBuildTask.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.GenFacades/NotSupportedAssemblyGenerator.cs |
Critical: the shared process-wide assembly resolver is not thread-safe. |
src/Microsoft.DotNet.GenFacades/Microsoft.DotNet.GenFacades.csproj |
Reviewed; no final comment. |
src/Microsoft.DotNet.GenFacades/GenPartialFacadeSourceGenerator.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.GenFacades/GenPartialFacadeSource.cs |
Critical: the shared process-wide assembly resolver is not thread-safe. |
src/Microsoft.DotNet.GenFacades/ClearAssemblyReferenceVersions.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.GenAPI/Microsoft.DotNet.GenAPI.csproj |
Reviewed; no final comment. |
src/Microsoft.DotNet.GenAPI/GenAPITask.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Deployment.Tasks.Links/src/DeleteAkaMSLinks.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Deployment.Tasks.Links/src/CreateAkaMSLinks.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.CMake.Sdk/src/CreateCMakeFileApiQuery.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.CMake.Sdk/Microsoft.DotNet.CMake.Sdk.csproj |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Workloads/src/Wix/WixToolTaskBase.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Workloads/src/Wix/WixToolTask.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Workloads/src/Wix/HarvesterToolTask.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Workloads/src/Microsoft.DotNet.Build.Tasks.Workloads.csproj |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Workloads/src/CreateVisualStudioWorkloadSet.wix.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Workloads/src/CreateVisualStudioWorkload.wix.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.VisualStudio/Vsix/GetPkgDefAssemblyDependencyGuid.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.VisualStudio/Vsix/FinalizeInsertionVsixFile.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.VisualStudio/OptProf/GetRunSettingsSessionConfiguration.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.VisualStudio/OptProf/GenerateTrainingPropsFile.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.VisualStudio/OptProf/GenerateTrainingInputFiles.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.VisualStudio/OptProf/FindLatestDrop.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Templating/src/GenerateFileFromTemplate.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.TargetFramework/src/TargetFrameworkResolver.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.TargetFramework/src/Microsoft.DotNet.Build.Tasks.TargetFramework.csproj |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.TargetFramework/src/ChooseBestTargetFrameworksTask.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.TargetFramework/src/ChooseBestP2PTargetFrameworkTask.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/ValidateHarvestVersionIsLatestForReleaseTests.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/TestBuildEngine.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/RuntimeGraphTests.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/PackageIndexTests.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/Log.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/CreateTrimDependencyGroupsTests.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/VersionUtility.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidationTask.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidatePackage.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidateHarvestVersionIsLatestForRelease.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidateFrameworkPackage.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/UpdatePackageIndex.cs |
Critical: mutable cached PackageIndex instances are shared across executions. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/SplitReferences.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/SplitDependenciesBySupport.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/PromoteDependencies.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/PackageReport.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/PackageMetadata.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/PackageItem.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/PackageIndex.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetUtility.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetPack.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetAssetResolver.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/Microsoft.DotNet.Build.Tasks.Packaging.csproj |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/HarvestPackage.cs |
Moderate: relative package-folder offsets can produce invalid asset paths. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetSupportedPackagesFromPackageReports.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetRuntimeTargets.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetRuntimeJsonValues.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetPackageVersion.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetPackageFromModule.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetPackageDestination.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetPackageDescription.cs |
Critical: the static description cache is not synchronized. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetMinimumNETStandard.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetLayoutFiles.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetLastStablePackage.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetInboxFrameworks.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetAssemblyReferences.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetApplicableAssetsFromPackages.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetApplicableAssetsFromPackageReports.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GenerateRuntimeDependencies.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GeneratePackageReport.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GenerateNuSpec.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/FilterUnknownPackages.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/CreateTrimDependencyGroups.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ApplyPreReleaseSuffix.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ApplyMetaPackages.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ApplyBaseLine.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/StabilizeWixFileId.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMsiVersion.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateGuidFromName.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateCurrentVersion.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/ExecWithRetries.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateWixCommandPackageDropBase.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateRpmPackage.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateMD5SumsFile.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateLightCommandPackageDrop.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateDebPackage.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateControlFile.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateChangelogFile.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Installers/Microsoft.DotNet.Build.Tasks.Installers.csproj |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.FileCatalog/GenerateFileCatalog.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.FileCatalog/CatalogEntry.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.FileCatalog/CatalogBuilder.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.FileCatalog.Tests/CatalogTests.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/PushToBuildStorage.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishSignedAssets.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishBuildToMaestro.cs |
Critical: the environment proxy still reads process-global environment state. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestV4.cs |
Critical: shared credential detection still reads process-global environment state. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestV3.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifest.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/LaunchDebugger.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/CreateAzureDevOpsFeed.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/ConfigureInputFeed.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/common/UploadToAzure.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/common/CreateNewAzureContainer.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/common/CreateAzureContainerIfNotExists.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/common/AzureStorageUtils.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/BlobFeedAction.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/AzureStorageExtensions.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Build.Tasks.Feed/src/AzureStorageAssetPublisher.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/ValidateLicense.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/Unsign.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/SingleError.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/SetCorFlags.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/SaveItems.cs |
Moderate: the saved output still uses the original relative path. |
src/Microsoft.DotNet.Arcade.Sdk/src/LocateDotNet.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/InstallDotNetCore.cs |
Moderate: project loading and process start still use original relative paths. |
src/Microsoft.DotNet.Arcade.Sdk/src/GroupItemsBy.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/GetLicenseFilePath.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/GetAssemblyFullName.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/GenerateSourcePackageSourceLinkTargetsFile.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/GenerateResxSource.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/GenerateChecksums.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/ExtractNgenMethodList.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/DownloadFile.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/CompareVersions.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/CheckRequiredDotNetVersion.cs |
Reviewed; no final comment. |
src/Microsoft.DotNet.Arcade.Sdk/src/CalculateAssemblyAndFileVersions.cs |
Reviewed; no final comment. |
src/Common/Internal/BuildTask.cs |
Reviewed; no final comment. |
eng/Version.Details.xml |
Reviewed; no final comment. |
eng/Version.Details.props |
Reviewed; no final comment. |
eng/MultiThreadableTaskAnalyzer.globalconfig |
Reviewed; no final comment. |
eng/BuildTask.targets |
Reviewed; no final comment. |
Directory.Packages.props |
Reviewed; no final comment. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (16)
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateLightCommandPackageDrop.cs:10
- The derived task is annotated, but
CreateWixCommandPackageDropBasestill performs its coreDirectory/Fileoperations with rawpackageDropOutputFolder,OutputFolder,WixSrcFiles, andLocpaths. Relative inputs therefore continue to use the process current directory; the single resolvedWixProjectFilecopy here does not make the inherited execution safe. ThreadTaskEnvironmentthrough the base chain or leave this task unannotated.
src/Microsoft.DotNet.Build.Tasks.Installers/src/ExecWithRetries.cs:17 - The outer task is annotated, but each inner
Microsoft.Build.Tasks.Execis created without propagating the outerTaskEnvironment. Its working directory and child-process environment consequently fall back to process-global state, so relative commands/working directories can resolve incorrectly in a multi-threaded build. Add and assign the child task'sTaskEnvironment, or do not opt this wrapper in.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GeneratePackageReport.cs:24 - The new environment plumbing does not reach
RuntimeFile: both resolver constructors later pass the raw string toJsonRuntimeFormat.ReadRuntimeGraph. A relative runtime.json consequently remains process-current-directory dependent. ResolveRuntimeFilebefore constructing either resolver.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetApplicableAssetsFromPackages.cs:24 RuntimeFileis still passed unmodified toAggregateNuGetAssetResolver, whose constructor reads the file directly. Relative values therefore remain dependent on the process current directory despite this task being annotated; passTaskEnvironment.GetAbsolutePath(RuntimeFile)instead.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/HarvestPackage.cs:20RuntimeFileis still passed raw to bothNuGetresolver constructors, which read it directly. Relative runtime graph paths therefore bypass the newly injected project directory and can load/fail against the process current directory. Resolve this input before constructing the resolvers.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetPack.cs:17- This task is now marked multithreadable, but
BaseDirectoryis passed unchanged toPackageBuilder.PopulateFiles. If a caller supplies a relative base directory, NuGet resolves the package files against the process current directory rather thanTaskEnvironment.ProjectDirectory, so-mtcan pack the wrong files. ResolveBaseDirectorythroughTaskEnvironmentbefore passing it to the NuGet API.
src/Microsoft.DotNet.Deployment.Tasks.Links/src/CreateAkaMSLinks.cs:12 - The shared
AkaMSLinksBasehelper readsClientCertificatewithFile.ReadAllTextdirectly. For a relative certificate path, this annotated task still resolves against the process current directory rather than the project directory supplied byTaskEnvironment, so concurrent projects can read the wrong file or fail. AddTaskEnvironmentplumbing and resolve the certificate path before enabling the annotation.
src/Microsoft.DotNet.Deployment.Tasks.Links/src/DeleteAkaMSLinks.cs:11 - The shared
AkaMSLinksBasehelper readsClientCertificatewithFile.ReadAllTextdirectly. For a relative certificate path, this annotated task still resolves against the process current directory rather than the project directory supplied byTaskEnvironment, so concurrent projects can read the wrong file or fail. AddTaskEnvironmentplumbing and resolve the certificate path before enabling the annotation.
src/Microsoft.DotNet.Helix/Sdk/CheckAzurePipelinesTestResults.cs:16 - This task now runs concurrently with other
AzureDevOpsTaskinstances, but the base class uses one staticRandomforGetRetryDelay.Randominstances are not thread-safe, so concurrent retries race on the generator and can produce corrupted/non-random backoff values, defeating retry staggering. UseRandom.Sharedor an instance-local generator before opting these tasks into multithreaded execution.
src/Microsoft.DotNet.Helix/Sdk/CheckAzurePipelinesTestResults.cs:16 - This annotation also opts the
AzureDevOpsTaskbase implementation into in-process multi-threading, but that base readsBUILD_*,SYSTEM_*, and the access token viaEnvironment.GetEnvironmentVariable. Those values are process-global rather than the injected per-project task environment, so concurrent projects can observe the wrong build credentials/metadata. ThreadTaskEnvironmentthrough the base or leave these derived tasks TaskHost-routed.
src/Microsoft.DotNet.Helix/Sdk/CreateFailedTestsForFailedWorkItems.cs:15 - This task now runs concurrently with other
AzureDevOpsTaskinstances, but the base class uses one staticRandomforGetRetryDelay.Randominstances are not thread-safe, so concurrent retries race on the generator and can produce corrupted/non-random backoff values, defeating retry staggering. UseRandom.Sharedor an instance-local generator before opting these tasks into multithreaded execution.
src/Microsoft.DotNet.Helix/Sdk/InstallDotNetTool.cs:18 - This task is annotated while inheriting
MSBuildTaskBase, which has noTaskEnvironment;DestinationPath,ToolPath, andWorkingDirectoryare passed toFileSystem/Commandunchanged. Relative paths and the command's inherited environment therefore still depend on the process current directory/environment. Resolve these inputs and propagate the task environment through the helper interfaces, or keep the task out of in-process multi-threading.
src/Microsoft.DotNet.Helix/Sdk/StartAzurePipelinesTestRun.cs:13 - This task now runs concurrently with other
AzureDevOpsTaskinstances, but the base class uses one staticRandomforGetRetryDelay.Randominstances are not thread-safe, so concurrent retries race on the generator and can produce corrupted/non-random backoff values, defeating retry staggering. UseRandom.Sharedor an instance-local generator before opting these tasks into multithreaded execution.
src/Microsoft.DotNet.Helix/Sdk/StopAzurePipelinesTestRun.cs:13 - This task now runs concurrently with other
AzureDevOpsTaskinstances, but the base class uses one staticRandomforGetRetryDelay.Randominstances are not thread-safe, so concurrent retries race on the generator and can produce corrupted/non-random backoff values, defeating retry staggering. UseRandom.Sharedor an instance-local generator before opting these tasks into multithreaded execution.
src/Microsoft.DotNet.SourceBuild/tasks/src/ReadNuGetPackageInfos.cs:13 PackagePathsis a plain string array, andReadIdentitypasses each value directly toPackageArchiveReader. Relative package paths therefore still resolve against the process current directory despite this task being annotated. InjectTaskEnvironmentand resolve the paths before opening them (including callers of the static helper).
src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WritePackageUsageData.cs:24- The four package-file arrays are still passed directly to
ReadNuGetPackageInfos.ReadIdentity, which opens each string as-is. Relative nupkg paths therefore use the process current directory even though this task now has a per-projectTaskEnvironment; pass resolved paths into the helper.
- Files reviewed: 184/184 changed files
- Comments generated: 17
- Review effort level: Lite
fbebc51 to
eea6ad7
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (17)
src/Microsoft.DotNet.Arcade.Sdk/src/SaveItems.cs:53
- The directory is now resolved through
TaskEnvironment, butproject.Save(File)below still passes the raw path. With a relativeFilein an in-process multithreaded build, the directory can be created under the project directory while the generated project is written relative to the process current directory. Save using the same environment-resolved path.
Directory.CreateDirectory(TaskEnvironment.GetAbsolutePath(path));
src/Microsoft.DotNet.Arcade.Sdk/src/SingleError.cs:9
- The get-then-register sequence is not atomic. Two concurrently scheduled
SingleErrorinstances with the same text can both observe no sentinel and both log an error, defeating the task's single-error contract. Synchronize the check/registration or use an atomic build-scoped gate before opting this task into multithreaded execution.
[MSBuildMultiThreadableTask]
src/Microsoft.DotNet.Build.Tasks.Feed/src/LaunchDebugger.cs:11
Debugger.Launch()depends on process-wide debugger state and can prompt or attach the debugger for the entire MSBuild process. That is not safe for a task opted into in-process multithreaded execution; remove the multithreadable annotation from this diagnostic-only task.
[MSBuildMultiThreadableTask]
public class LaunchDebugger : Microsoft.Build.Utilities.Task
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifest.cs:28
- The concrete task is opted into multithreading, but its inherited
PublishArtifactsInManifestBasestill callsEnvironment.GetEnvironmentVariabledirectly when deciding whether Entra credentials are available. The injectedTaskEnvironmenton this class is therefore not used for that decision, so concurrent project executions still depend on process-global environment state. Move the check into the environment-aware plumbing in the base class, or remove the opt-in until that migration is complete.
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestV4.cs:21 - This annotation opts the task into in-process execution even though the inherited
PublishArtifactsInManifestBasereads Azure credential variables withEnvironment.GetEnvironmentVariableand has noTaskEnvironmentplumbing. That makes the task depend on process-global state, contrary to the multithreadable-task contract. Either thread the environment through the base class or leave this task unannotated.
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishBuildToMaestro.cs:35 - Several execution paths still call
_getEnvProxy.GetEnv, whose implementation usesEnvironment.GetEnvironmentVariabledirectly (for example, the Azure DevOps collection URI, project, and build identifiers). The task is now annotated and has aTaskEnvironment, but those reads bypass it and retain process-global behavior. Replace the proxy with the injected environment before opting this task into multithreading.
src/Microsoft.DotNet.Build.Tasks.Feed/src/PushToBuildStorage.cs:48 - This task is now routed as multithreadable, but it has no
TaskEnvironment; the injected file system receives paths such aspackage.OriginalFile, the local storage directories, andAssetManifestPathwithout project-relative normalization. Any relative input still uses the process current directory. Normalize the task's path inputs before model/file-system operations or leave it TaskHost-routed.
src/Microsoft.DotNet.Build.Tasks.Feed/src/common/UploadToAzure.cs:20 - This task is now opted into concurrent in-process execution, but
TokenSourceis static andCancel()cancels that shared source. Cancelling one task instance therefore cancels every concurrently runningUploadToAzureinvocation, causing unrelated uploads to stop. Make the cancellation source/token instance-owned before enabling multithreaded execution.
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateMD5SumsFile.cs:42 - The file is opened via
TaskEnvironment.GetAbsolutePath, but the emitted relative name still subtracts the rawRootDirectorylength from the rawItemSpec. When these inputs are project-relative, the checksum file contains a malformed path (or throws) even though hashing succeeded. Compute the relative name from the same normalized file and root paths.
src/Microsoft.DotNet.Build.Tasks.Installers/src/ExecWithRetries.cs:18 - This wrapper opts into in-process multithreading but delegates to
Microsoft.Build.Tasks.Execwithout injecting a per-project environment or resolvingWorkingDirectory. A relative working directory therefore still uses the process current directory, and the child process inherits ambient process state. Keep this task TaskHost-routed (or replace the wrapper with aToolTaskimplementation usingTaskEnvironment).
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GeneratePackageReport.cs:15 - This task now runs in-process and calls
FrameworkUtilities, whose sharedFrameworkReduceris initialized through an unsynchronizedif (s_reducer == null) s_reducer = new FrameworkReducer()lazy path. Concurrent first use can race on that static state, so this opt-in does not meet the thread-safety requirement. Make the helper useLazy<FrameworkReducer>/synchronized initialization (and audit the other newly annotated callers) before enabling this task.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/HarvestPackage.cs:619 - This helper has the same mismatch: it enumerates from
TaskEnvironment.GetAbsolutePath(packageFolder)but stripspackageFolder.Lengthfrom the resulting absolute path. Relative package folders therefore yield incorrect item paths. Keep the normalized folder in the relative-path calculation.
src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAndroidWorkItems.cs:18 - The annotated task passes APK metadata through
XHarnessTaskBase.GetNameAndPathand rawIFileSystemoperations, but neither the task nor the base has aTaskEnvironment. A relative APK path (and the generated payload path) is consequently resolved against the process current directory rather than the project directory. Normalize these paths before the helper/file-system calls or remove the annotation.
src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAppleWorkItems.cs:18 - The annotated task passes app-bundle and temporary paths through
XHarnessTaskBase/rawIFileSystemoperations without aTaskEnvironment. Relative inputs are therefore still resolved against the process current directory in multithreaded builds. Normalize all paths before this helper chain or remove the annotation.
src/Microsoft.DotNet.Helix/Sdk/FindDotNetCliPackage.cs:21 - The task is now eligible to run concurrently, but its HTTP client is stored in the static mutable
_clientfield and reassigned for every task instance. Concurrent invocations can overwrite each other's client while requests are in flight, so one task can use another task's handler/configuration. Make the client instance-owned (and dispose it appropriately), or do not opt this task into multithreaded execution.
src/Microsoft.DotNet.Helix/Sdk/InstallDotNetTool.cs:18 - This task is annotated, but its destination, dotnet executable, working directory, and mutex/file-system paths are passed through raw
IFileSystem/command APIs with noTaskEnvironment. Relative inputs therefore still resolve against the process current directory in multithreaded builds. Inject and apply the task environment to every path, or do not opt this task in.
src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WritePackageUsageData.cs:146 - The files returned here are absolute because
GetAbsolutePath(RootDir)is used, butGetPathRelativeToRootstill compares them with the unnormalizedRootDir. IfRootDiris relative, every discovered asset path fails theStartsWithcheck and the task throws instead of producing usage data. NormalizeRootDironce withTaskEnvironmentbefore both the scan and relative-path calculation.
- Files reviewed: 185/185 changed files
- Comments generated: 15
- Review effort level: Lite
Copilot's reviewer found two bug classes on dotnet/arcade#17381 that my own audit missed, plus the two races I had already fixed independently. 1. Paths resolved at one site but used raw at another. These do not throw; they silently produce wrong results, which is why the null-guard audit did not surface them: - HarvestPackage enumerated the resolved absolute folder but computed package-relative paths with Substring(rawPath.Length + 1), so a relative PackagesFolders entry misclassified every harvested asset. Fixed at the source: LocatePackageFolder now returns an absolute path, so every downstream use agrees. - WritePackageUsageData compared absolute enumeration results against a raw RootDir in both GetPathRelativeToRoot and the ProjectDirectories validation, throwing for every discovered file when RootDir is relative. Added AbsoluteRootDir, which preserves any trailing separator because callers depend on the stripped result staying relative. - UpdateXlf's exception filter compared FileNotFoundException.FileName against the unresolved path, turning a friendly BuildErrorException into an unhandled exception. - SaveItems created the resolved directory but saved to the raw path; InstallDotNetCore checked the resolved VersionsPropsPath but loaded the raw one and launched the raw DotNetInstallScript; WriteBuildOutputProps filtered AdditionalAssetDirs with an unresolved Directory.Exists. 2. Tasks annotated despite process-global state inherited from a base class. The analyzer does not walk into base types, so these all passed a clean analyzer run. De-annotated with TODOs pointing at arcade#17378: - The five Feed publishing tasks (PublishArtifactsInManifestBase reads AZURESUBSCRIPTION_*, SYSTEM_ACCESSTOKEN and workload-identity variables from the process environment; PublishBuildToMaestro goes through _getEnvProxy). - Four AzureDevOpsTask derivatives (same environment reads plus a shared static Random for retry jitter). - Two RoslynBuildTask derivatives (process-wide AssemblyLoadContext.Resolving). - SignCheckTask (static _fileVerifiers populated per manager construction), FindDotNetCliPackage (static HttpClient overwritten per execution), CreateLightCommandPackageDrop (base still uses raw paths). FinalizeInsertionVsixFile and ReadNuGetPackageInfos were annotated but never implemented IMultiThreadableTask, so TaskEnvironment was never injected and every resolution silently used Fallback. Both are now fully migrated instead. Also keyed the LocateDotNet and CheckRequiredDotNetVersion build-scoped caches by repository and SDK version; they are registered per build, not per project, so a second repository could reuse the first one's result. UpdatePackageIndex is deliberately left annotated: it passes a single AbsolutePath to PackageIndex.Load, which binds to the uncached overload rather than the cached IEnumerable one, so it never shares a mutable index. Annotated tasks: 119 of 136. Arcade builds clean (0 warnings, 0 errors); PackageTesting (9), Packaging (48) and Feed (214) tests pass. The one Arcade.Sdk.Tests failure is the pre-existing NU1009 SwixBuild conflict, which this change does not touch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
eea6ad7 to
6ae5f84
Compare
MSBuild 18.x can execute tasks on multiple threads inside a single node (`-mt`), but only for tasks that opt in via [MSBuildMultiThreadableTask]. Everything else is routed to an out-of-proc sidecar TaskHost, which is far more expensive than the in-proc path. Measuring a runtime inner-repo build in the VMR with `-mt -nodeReuse:true` against a baseline showed only a 3.6% wall-clock improvement, because none of Arcade's tasks were annotated: TaskHost task invocations went from 849 to 14,125 and total task time went *up* by 98 seconds, cancelling out most of the gain from threading. This annotates 133 of Arcade's 136 concrete tasks and makes the supporting code multi-threading safe: - Update the MSBuild package dependencies from 17.12.50 to 18.8.2, which is the first version exposing both [MSBuildMultiThreadableTask]/IMultiThreadableTask and ToolTask.TaskEnvironment. - Reference Microsoft.Build.TaskAuthoring.Analyzer from eng/BuildTask.targets so new violations are caught at build time. eng/MultiThreadableTaskAnalyzer.globalconfig scopes it to tasks that have already opted in, and disables the API-shape suggestions (MSBuildTask0006-0008, 0011) which would be binary-breaking for task parameters set from targets across the ecosystem. - Replace ambient process state with the injected TaskEnvironment: relative paths now resolve against the project directory instead of the process-wide current directory, environment variables are read per-project, and child processes are started from TaskEnvironment.GetProcessStartInfo(). - Thread AbsolutePath through the shared path helpers in Packaging, GenFacades, Feed, NuGetRepack, SharedFramework.Sdk, PackageTesting and XliffTasks. - Fix a real data race: TargetFrameworkResolver.CreateOrGet used an unsynchronized static Dictionary cache. It is now a ConcurrentDictionary. - Stop sharing a static Newtonsoft JsonSerializer instance in PackageIndex. - Delete the internal BuildTask/ILog base class. It only wrapped Microsoft.Build.Utilities.Task and TaskLoggingHelper, and standing between tasks and the MSBuild base class meant every derived task needed its own TaskEnvironment plumbing. Tasks now derive from Microsoft.Build.Utilities.Task directly and use Log/TaskLoggingHelper. SignToolTask, CreateVisualStudioWorkload and CreateVisualStudioWorkloadSet are deliberately left unannotated. Their helper chains (BatchSignUtil, ZipData, Configuration, VerifySignatures, WorkloadPackageBase, MsiBase, SwixProject, ...) still resolve paths against the process-wide current directory, and converting them is a much larger change. Each carries a TODO pointing at the tracking issue. Both run once per build, so keeping them TaskHost-routed costs effectively nothing. Contributes to dotnet#17378 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
6ae5f84 to
244ce41
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishBuildToMaestro.cs:39
IMultiThreadableTaskis documented as a thread-safe routing signal, even though the current router bug ignores interface-only implementations. Once fixed, this task would run in-process while_getEnvProxystill reads ambient process environment. Remove the interface until environment access is migrated.
- Files reviewed: 186/186 changed files
- Comments generated: 14
- Review effort level: Balanced
- GetPackageDescription: stop memoizing failed description loads. GetOrAdd cached the null returned after an IOException, so every later invocation on the node silently got null without retrying or re-logging. - GetAssemblyFullName, ExecWithRetries: implement IMultiThreadableTask and resolve paths through TaskEnvironment. ExecWithRetries forwards its TaskEnvironment into the nested Exec instance, which MSBuild does not inject into because the task constructs it directly. - InstallDotNetTool, PushToBuildStorage, SingleError, LaunchDebugger: remove the MSBuildMultiThreadableTask attribute. These are not yet safe to run in-process, so they must keep routing to a TaskHost until they are migrated properly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
src/Microsoft.DotNet.Arcade.Sdk/src/LocateDotNet.cs:83
- This still splits
PATHwith the Windows-only;separator. On Unix,PATHis colon-delimited, so the entire value is treated as one directory anddotnetcannot be found. UsePath.PathSeparatorfor cross-platform lookup.
var dotNetDir = paths.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries).FirstOrDefault(p => File.Exists(TaskEnvironment.GetAbsolutePath(Path.Combine(p, fileName))));
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidateHarvestVersionIsLatestForRelease.cs:57
- Correct the malformed success message.
src/Microsoft.DotNet.Build.Tasks.Feed/src/LaunchDebugger.cs:13 - The PR description still claims 133 of 136 tasks are annotated and lists only three deliberate exclusions, but the updated diff now leaves additional tasks unannotated (including this one,
SingleError,SignCheckTask,InstallDotNetTool, GenFacades tasks, and several Feed tasks). Update the stated count and exclusion/validation scope so reviewers and performance consumers are not relying on the superseded inventory.
// Not opted into multithreading, and deliberately never will be: Debugger.Launch attaches a
// debugger to the whole process. In a shared multithreaded node that would affect every project
// building on that node, and concurrent invocations would race to attach. Keeping this
// diagnostic task in the out-of-proc TaskHost confines the attach to a single sidecar.
- Files reviewed: 187/187 changed files
- Comments generated: 6
- Review effort level: Balanced
…DotNetVersion Both tasks do a non-atomic GetRegisteredTaskObject/RegisterTaskObject pair, which under multithreaded execution lets two threads miss and populate the cache. Both computations are pure, so the result is unchanged; the only visible effect is that CheckRequiredDotNetVersion can log a failing check's error twice. Recording this so the pattern is not mistaken for the SingleError case, where the same race defeated the task's entire purpose and the attribute had to be removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WritePackageUsageData.cs:21
- This opt-in still leaves the four package-file inputs on ambient path resolution:
ReadNuGetPackageInfos.ReadIdentityopens each supplied string directly, unlike the runtime JSON paths migrated below. If any item identity is relative, a shared-node build can read from the process working directory instead of this project. Resolve every package path withTaskEnvironment.GetAbsolutePathbefore callingReadIdentity, or keep this task TaskHost-routed until that migration is complete.
- Files reviewed: 187/187 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…ncies All six are P3 with no measured invocations in the hot-path profile, so in-process routing buys no measured build-time saving while taking on real correctness risk. Each keeps its `IMultiThreadableTask` implementation and `TaskEnvironment`-based path handling -- correct either way, and the groundwork for a later migration -- but loses the routing attribute, and carries a comment recording why so it is not silently re-added. - GenAPITask: `HostEnvironment` resolves the raw `LibPath`/`Assembly` values via `Environment.ExpandEnvironmentVariables` plus `Directory.Exists`/`File.Exists` (HostEnvironment.cs:719-740). The task resolves `OutputPath` and `HeaderFile` through `TaskEnvironment` but hands these two through raw -- resolved at one site, raw at another. Fixing it needs expand-then-resolve inside Cci, because resolving a path that still contains %VAR% would itself be wrong. - SendHelixJob: `JobDefinition` reads BUILD_REPOSITORY_NAME, BUILD_SOURCEBRANCH, SYSTEM_TEAMPROJECT and BUILD_REASON straight from `Environment` (JobDefinition.cs:209-223,402-423). - CreateAzureDevOpsFeed, CreateNewAzureContainer, CreateAzureContainerIfNotExists: build an `AzureCliCredential` when no PAT/AccountKey is supplied, which launches `az` from the ambient process environment. This is the same class already used to exclude PublishArtifactsInManifest*; the criterion was applied incompletely. - UploadToAzure: cancellation state is `static`, so `Cancel()` on one instance cancels the token handed to every other concurrent and future instance, and the uploads use an unlinked timeout token rather than the task token. Also avoided writing the literal attribute name in these comments, so an audit grep for the attribute does not match a class that deliberately lacks it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: be19d794-fe4f-43fe-9e6e-f6e6b13ec6e7
There was a problem hiding this comment.
Review details
Suppressed comments (5)
Previously missed (5) — in code that hasn't changed since the last review.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/HarvestPackage.cs:17
- This opt-in is not thread-safe yet:
HarvestSupportedFrameworkspassesRuntimeFileunchanged toAggregateNuGetAssetResolverandNuGetAssetResolver, whose constructors callJsonRuntimeFormat.ReadRuntimeGraph(runtimeFile). A relative value (the existing tests use"runtime.json") therefore still resolves against the shared process working directory and can load another project's graph or fail under-mt. Resolve the value throughTaskEnvironmentand preferably change both resolver constructors to acceptAbsolutePath; until then, keep this task TaskHost-routed.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GeneratePackageReport.cs:15 - This task still sends the raw
RuntimeFiletoAggregateNuGetAssetResolverandNuGetAssetResolver; both constructors open it viaJsonRuntimeFormat.ReadRuntimeGraph(string). Relative task inputs consequently resolve against the shared node's process directory rather than this project's directory, so concurrent projects can read the wrong runtime graph. Resolve this path throughTaskEnvironment(ideally by retyping the shared resolver API toAbsolutePath) before opting in.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetApplicableAssetsFromPackages.cs:17 - The new opt-in leaves
RuntimeFileunresolved when constructingAggregateNuGetAssetResolver; that helper opens the string directly withJsonRuntimeFormat.ReadRuntimeGraph. With a relative runtime graph, this task therefore depends on the shared process current directory and can read another project's file under-mt. Thread anAbsolutePathinto the resolver or leave this task unannotated.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetPack.cs:18 BaseDirectoryis still passed unchanged to NuGet'sPackageBuilder.PopulateFiles, which performs filesystem enumeration relative to that base. When callers provide a relativeBaseDirectory, the annotated task resolves package inputs against the shared process working directory instead of the project directory. AbsolutizeBaseDirectorybeforePopulateFiles, or keep this task TaskHost-routed until that path is migrated.
src/Microsoft.DotNet.SignCheckTask/SignCheckTask.cs:17- The PR description still says 133 of 136 tasks are annotated and lists only three exclusions, but the current revision documents many additional concrete exclusions (including this task,
SingleError,SendHelixJob,InstallDotNetTool, both GenFacades tasks, several Feed tasks, and others). This materially changes the claimed migration coverage and expected TaskHost reduction; update the inventory/count and the “Deliberately not migrated” section to match the code.
- Files reviewed: 187/187 changed files
- Comments generated: 2
- Review effort level: Balanced
…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>
LaunchDebugger has no UsingTask registration and no reference anywhere in the repo, so it is unreachable dead code. It was one of the tasks left unannotated for the multithreading migration, since Debugger.Launch attaches a debugger to the whole process and a shared multithreaded node would attach for every project building on it. Deleting it removes the exclusion instead of documenting it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: be19d794-fe4f-43fe-9e6e-f6e6b13ec6e7
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Microsoft.DotNet.Build.Tasks.Feed/src/LaunchDebugger.cs:1
- The PR description says
LaunchDebuggeris one of the tasks deliberately left unannotated, but this change deletes the public task class instead. Any consumer that referencesMicrosoft.DotNet.Build.Tasks.Feed.LaunchDebuggerwill now fail to load it. Restore the unannotated task (as described), or explicitly document and version this as a breaking removal.
- Files reviewed: 187/187 changed files
- Comments generated: 11
- Review effort level: Balanced
UploadToAzure has no UsingTask registration and no reference anywhere in the repo, so it is unreachable dead code. Uploads go through AzureStorageAssetPublisher and BlobFeedAction instead. It was one of the tasks left unannotated for the multithreading migration: its CancellationTokenSource is static, so Cancel() on one instance cancelled the token handed to every other concurrent and future instance, and the uploads used an unlinked timeout token rather than the task token. Deleting it removes that bug along with the exclusion. Its base AzureConnectionStringBuildTask and the AzureStorageUtils helpers stay in use by CreateAzureContainer and the publishers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: be19d794-fe4f-43fe-9e6e-f6e6b13ec6e7
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidateHarvestVersionIsLatestForRelease.cs:57
- Correct the misspelling in this success message: “erreleasea” should be “release.”
- Files reviewed: 187/187 changed files
- Comments generated: 2
- Review effort level: Balanced
Copilot review flagged eleven annotated tasks that pass a raw, user-settable path into a helper that opens it with plain File/Directory/NuGet APIs. In a shared -mt node those bind to the process current directory rather than the invoking project. The inputs are not absolute by construction. RuntimeGraph and RuntimeIdentifierGraph come from $(RuntimeIdentifierGraphPath), which is a public settable property, and RuntimeFile comes from $(RuntimeIdGraphDefinitionFile), which is explicitly guarded with Condition="'$(...)' == ''" so it is designed to be overridden. Only the Bundled* SDK defaults are absolute by construction. Resolved through the injected TaskEnvironment: - ChooseBestTargetFrameworksTask, ChooseBestP2PTargetFrameworkTask: RuntimeGraph. These had the attribute but did not implement IMultiThreadableTask, so they had no way to resolve anything; both now do. TargetFrameworkResolver also caches by the path string, so a relative value additionally let two projects share one resolver built from different files. - GeneratePackageReport, GetApplicableAssetsFromPackages, HarvestPackage: RuntimeFile, before constructing the NuGet asset resolvers. - GenerateSharedFrameworkDepsFile: RuntimeIdentifierGraph. It is optional, so an unset value is passed through unchanged to keep the existing ReadRuntimeGraph error instead of throwing from GetAbsolutePath. - NuGetPack: BaseDirectory before PopulateFiles, and the nuspec-relative fallback. - GetCompatiblePackageTargetFrameworks: PackagePaths. Also had the attribute without the interface. - WritePackageUsageData: the four package-file arrays feeding ReadIdentity. - CreateXHarnessAndroidWorkItems, CreateXHarnessAppleWorkItems: ApkPath and AppBundlePath, plus the Apple TmpDir. Relative paths here are the documented usage in tools/xharness-runner/Readme.md, so this was the most likely of the set to bite. TaskEnvironment is added to the shared XHarnessTaskBase. The XHarness tests already covered relative paths, which is why five of them caught the change. They now resolve their MockFileSystem registrations through the same TaskEnvironment the task uses, so the relative-path coverage is kept and the resolution itself is exercised. Full non-incremental Release build: 0 errors, 0 warnings. Helix.Sdk 236, Packaging 48, PackageTesting 9, SourceBuild 1: all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: be19d794-fe4f-43fe-9e6e-f6e6b13ec6e7
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Microsoft.DotNet.Arcade.Sdk/src/CheckRequiredDotNetVersion.cs:74
- Absolute paths are case-sensitive on Unix. This comparison can treat
/repo/A/global.jsonand/repo/a/global.jsonas the same cache entry; when the timestamps and requested SDK match, the second repository can reuse the first repository's validation result even if itsglobal.jsonhas different requirements. Use an ordinal comparison (a harmless extra cache miss on Windows) or an OS-aware path comparer.
string.Equals(globalJsonPath.Value, cachedResult.GlobalJsonPath, StringComparison.OrdinalIgnoreCase) &&
src/Microsoft.DotNet.Arcade.Sdk/src/LocateDotNet.cs:66
- Absolute paths are case-sensitive on Unix. This comparison aliases repositories whose paths differ only by case, so matching timestamps and
PATHvalues can cause one repository to reuse the other repository's resolved SDK path even when theirglobal.jsonfiles request different SDKs. Use an ordinal comparison (a harmless extra cache miss on Windows) or an OS-aware path comparer.
string.Equals(globalJsonPath.Value, cachedResult.GlobalJsonPath, StringComparison.OrdinalIgnoreCase) &&
- Files reviewed: 190/190 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…asks WixToolTask and HarvesterToolTask are not registered via UsingTask; MsiBase constructs them directly. MSBuild therefore never routes them and never injects a TaskEnvironment, so WixToolTaskBase resolves against TaskEnvironment.Fallback and the annotation had no effect other than marking part of the CreateVisualStudioWorkload helper chain as migrated, which contradicts the TODO comments on both entry-point tasks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: be19d794-fe4f-43fe-9e6e-f6e6b13ec6e7
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Microsoft.DotNet.Arcade.Sdk/src/CheckRequiredDotNetVersion.cs:74
OrdinalIgnoreCasecan conflate distinct repositories on case-sensitive filesystems. For example,/repo/A/global.jsonand/repo/a/global.jsonwith the same timestamp and requested SDK version cause the second invocation to reuse the first file's result. Use an ordinal comparison; on Windows, a casing-only difference would then only cause a harmless cache miss.
string.Equals(globalJsonPath.Value, cachedResult.GlobalJsonPath, StringComparison.OrdinalIgnoreCase) &&
src/Microsoft.DotNet.Arcade.Sdk/src/LocateDotNet.cs:66
OrdinalIgnoreCasecan conflate differentglobal.jsonfiles on case-sensitive filesystems. If paths differing only by case have matching timestamps and PATH values, this task can return the dotnet executable selected for the other repository. Use an ordinal comparison; a casing-only path on Windows would merely miss the cache.
string.Equals(globalJsonPath.Value, cachedResult.GlobalJsonPath, StringComparison.OrdinalIgnoreCase) &&
- Files reviewed: 190/190 changed files
- Comments generated: 1
- Review effort level: Balanced
Why
MSBuild 18.x can run tasks on multiple threads inside a single node (
-mt), but only for tasks that opt in via[MSBuildMultiThreadableTask]. Everything else is routed to an out-of-proc sidecar TaskHost, which is much more expensive than the in-proc path.Measured on a runtime inner-repo build in the VMR —
-mt -nodeReuse:true -msagainst an otherwise identical baseline, with no Arcade task annotated:-mt+ server + node reuseOnly a 3.6% wall-clock gain: task time went up by 98 s and effective parallelism went down, cancelling most of the benefit of threading. Arcade tasks run in essentially every project in every repo, so they are the single biggest lever here.
Those numbers are the unmigrated state. Across a full VMR build, where every repo pays this cost on every project, the expected saving from this change is in the 5-15 minute range.
What
Annotates 104 of the 121 tasks that Arcade registers via
UsingTaskand that resolve to source in this repo, and makes the supporting code multi-threading safe.17.12.50->18.8.2. First version exposing both[MSBuildMultiThreadableTask]/IMultiThreadableTaskandToolTask.TaskEnvironment. The routing check is guarded byBuildParameters.MultiThreaded, so the attribute is inert on older hosts.Microsoft.Build.TaskAuthoring.Analyzerfromeng/BuildTask.targets, scoped byeng/MultiThreadableTaskAnalyzer.globalconfigto tasks that have already opted in (msbuild_task_analyzer.scope = multithreadable_only). That keeps it as a regression guard without drowning the build in diagnostics. The API-shape suggestions (MSBuildTask0006-0008,0011) are off: several would be binary-breaking for task parameters set from targets across the ecosystem.TaskEnvironment. Relative paths resolve against the project directory instead of the process-wide current directory, environment variables are read per-project, and child processes start fromTaskEnvironment.GetProcessStartInfo().AbsolutePaththrough the shared path helpers in Packaging, GenFacades, Feed, NuGetRepack, SharedFramework.Sdk, PackageTesting and XliffTasks. Retyping a helper's path parameter is what actually resolves the transitiveMSBuildTask0005chains.BuildTask/ILogbase class. It only wrappedMicrosoft.Build.Utilities.TaskandTaskLoggingHelper, and standing between tasks and the MSBuild base class meant every derived task needed its ownTaskEnvironmentplumbing anyway. This is the bulk of the line count.LaunchDebuggerandUploadToAzuretasks. Neither has aUsingTaskregistration or a reference anywhere in the repo or the widerdotnetorg, so both were unreachable, and both would otherwise have needed a permanent exclusion.Debugger.Launch()attaches to the whole process, so a shared multithreaded node would attach for every project building on it.UploadToAzure'sCancellationTokenSourceis static, soCancel()on one instance cancelled the token handed to every other concurrent and future instance — a real bug that goes away with the file. Uploads already go throughAzureStorageAssetPublisherandBlobFeedAction; the shared baseAzureConnectionStringBuildTaskand theAzureStorageUtilshelpers stay in use.Concurrency bugs found and fixed
The analyzer checks path/env/process API usage, not thread-safety of shared mutable state, so a clean analyzer run is not evidence a task is thread-safe. These needed a manual sweep, and all are latent bugs today under any concurrent access — not just under
-mt:TargetFrameworkResolverFindBestItemGroupunder concurrentGetNearest. Hit ~15 runtime projects at once.GetPackageDescriptionstatic Dictionary; additionally memoized thenullreturned after a loggedIOException, so every later invocation on the node silently gotnullwithout retrying.GetCompatiblePackageTargetFrameworksstatic List/Dictionarymutated fromExecute; also accumulated duplicates across invocations on a reused node.PackageIndexJsonSerializer, which is not thread-safe.FindDotNetCliPackagestatic HttpClientoverwritten by every execution; a concurrent instance could issue requests through another's disposed handler.AkaMSLinksBaseFile.ReadAllText(ClientCertificate)on a raw input path, in the shared base of two annotated tasks.AzureDevOpsTaskstatic Randomfor retry jitter ->Random.Shared.Two regressions the migration itself introduced, both fixed and worth flagging to anyone doing this work:
GetAbsolutePath(null)throws, whileFile.Exists(null)/Directory.Exists(null)returnfalse. Mechanically wrapping those two calls turns an optional, unset task property into a hard failure — it took out every source-build leg. They are the only two BCL path APIs with that tolerance, so the regression surface is exactly those; all 292 added call sites were cross-referenced against[Required]and 6 fixed. Note this surfaced on legs running without-mt:TaskEnvironmentis injected in normal builds too.IMultiThreadableTask(FinalizeInsertionVsixFile,ReadNuGetPackageInfos,ChooseBestTargetFrameworksTask,ChooseBestP2PTargetFrameworkTask,GetCompatiblePackageTargetFrameworks), soTaskEnvironmentwas never injected and every resolution silently usedFallback. The two signals are independent — the attribute controls routing, the interface controls injection — and nothing fails loudly when only one is present. The interface cannot be made the routing signal, sinceToolTaskimplements it and that would opt in everyToolTasksubclass in the ecosystem.A late review pass found 11 tasks passing a raw path into a helper that opens it with plain
File/Directory/NuGet APIs, below the analyzer's visibility. The properties involved ($(RuntimeIdentifierGraphPath),$(RuntimeIdGraphDefinitionFile),ApkPath,AppBundlePath,BaseDirectory,PackagePaths) are all user-settable —RuntimeIdGraphDefinitionFileis even declared withCondition="'$(RuntimeIdGraphDefinitionFile)' == ''"— so their absolute defaults are not something a task may rely on. All now resolve throughTaskEnvironment. InChooseBest*this also fixed theTargetFrameworkResolvercache, which was keyed on the raw path string and so could hand two projects with different working directories the same resolver built from a different file.A follow-up sweep for the same shape also removed the attribute from
WixToolTaskandHarvesterToolTask. Neither isUsingTask-registered —MsiBaseconstructs them directly — so MSBuild never routes them and never injects aTaskEnvironment, leavingWixToolTaskBaseonTaskEnvironment.Fallback. The annotation was inert but marked part of theCreateVisualStudioWorkloadchain as migrated, contradicting the TODO comments on both entry points. Registered-task counts are unaffected.Deliberately not migrated
18 tasks are left unannotated: the 17 registered ones plus
CreateAzureContainerIfNotExists, which is notUsingTask-registered but is called throughBlobFeedAction. Every one carries a comment at its declaration recording why, so the decision is not silently reverted.All are P3 with no measured invocations in the hot-path profile — every task appearing in a runtime inner-repo binlog (7,143 invocations, ~82 s of TaskHost overhead in the measurement above) is already migrated, so annotating the rest cannot save build time. They are also the tasks where a mistake is most expensive and least testable: publishing to Maestro and blob storage, code signing, Wix packaging. No PR build exercises them, so a regression would surface in a release pipeline instead of CI.
Ambient credentials or pipeline environment (8). Build an
AzureCliCredentialwhen no PAT/AccountKey is supplied, or read pipeline variables straight from the process environment:CreateAzureDevOpsFeed,CreateNewAzureContainer,CreateAzureContainerIfNotExists,PublishArtifactsInManifest,PublishBuildToMaestro,PublishSignedAssets,SendHelixJob(JobDefinitionreadsBUILD_REPOSITORY_NAME,BUILD_SOURCEBRANCH,SYSTEM_TEAMPROJECT,BUILD_REASON),InstallDotNetTool(viaICommandFactory).Process-wide static state (4).
SignCheckTask(static_fileVerifiersinSignatureVerificationManager),GenPartialFacadeSourceandNotSupportedAssemblyGenerator(RoslynBuildTasksubscribes an instance handler to the process-wideAssemblyLoadContext.Resolving, so A's resolution can be serviced by B's handler — precisely the cross-location Roslyn load the method's own comment exists to prevent, and a latent bug today),SingleError(GetRegisteredTaskObjectfollowed byRegisterTaskObjectis not atomic).Unresolved paths flowing into helper chains (6).
GenAPITask(HostEnvironmentexpands variables and probes withDirectory.Exists/File.Existson raw input),PushToBuildStorage(six*LocalStorageDirinputs plus artifact items that would all have to migrate together),SignToolTask,CreateLightCommandPackageDrop,CreateVisualStudioWorkload,CreateVisualStudioWorkloadSet(helper chains spanning 29 and 62 files that still resolve against the process-wide current directory;MSBuildTask0005is suppressed at those entry points).Several of these keep their
IMultiThreadableTaskimplementation andTaskEnvironment-based path handling. That is deliberate and is not a half-migration: routing is decided by the attribute alone, so the interface only causesTaskEnvironmentto be injected, which makes path resolution correct in either mode. The interface also cannot ever become a routing signal —ToolTaskitself implements it, so that would opt in everyToolTask-derived task in the ecosystem. Removing it from a not-yet-safe task would revert that task's paths to the process current directory while leaving it exactly as unsafe.Path.GetTempPath()is suppressed at four sites where the temp root is only ever the parent of a freshly generated GUID/random name and is therefore never shared between concurrent tasks.TaskEnvironmenthas no temp-directory member today.Validation
dotnet build Arcade.slnx -c Release --no-incrementalwith CI defaults (TreatWarningsAsErrorson): 0 errors, 0 warnings, both in this repo and in the VMR.--no-incrementalis required for an authoritative count. Analyzer liveness was separately confirmed by injecting a deliberateEnvironment.GetEnvironmentVariablecall into an annotated task and checking it was reported asMSBuildTask0002.CentralPackageManagementTests.ImplicitPackageReferences_ShouldNotConflictWithPackageVersionEntries(NU1009 onMicroBuild.Plugins.SwixBuild, untouched here), twoWorkloads.Testsneeding a full VSMSBuild.exe, andArcade.Validation.Tests.RepoTestswhich time out building test repos locally.Contributes to #17378; the remaining tasks are tracked in #17388.
Filed against MSBuild while doing this work: #14779 and #14790 (
TaskRouterdocumentation contradicts the attribute-only implementation), #14780 (MSBuildTask0003code fix produces non-compiling code), and #14783/#14784/#14785 (analyzer gaps this migration surfaced — most importantly that it does not walk into base types).