dotnet / dotnet/arcade

Remaining multi-threaded task migration blockers: ambient credentials, process-wide static state, and unresolved paths

Open
#17,388 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
729
Forks
397
Avg merge
3d 15m
Merged PRs (30d)
149

Description

Follow-up to #17378 and #17381.

#17381 annotates **104 of the 121** tasks that Arcade registers via `UsingTask` and that resolve to source in this repo. **18 remain unannotated**: those 17, plus `CreateAzureContainerIfNotExists`, which has no `UsingTask` registration of its own but is invoked through `BlobFeedAction`.

Every one carries a comment at its declaration recording why, so the decision is not silently reverted.

> Counts here supersede the earlier "127 of 136 / nine remain" figures. That accounting predated three things: six tasks were de-annotated during review once their ambient-state dependencies could not be established, `LaunchDebugger` and `UploadToAzure` were deleted as unreachable dead code, and the inventory was rebased onto `UsingTask` registrations rather than raw class counts. `PublishArtifactsInManifestV3`/`V4` in particular are *not* separately registered — they are subclasses of `PublishArtifactsInManifestBase` reached through the single registered `PublishArtifactsInManifest`.

### Why these were excluded from #17381

- **No build-time benefit.** Only 10 Arcade tasks appear in a runtime inner-repo binlog at all (7,143 invocations, ~82 s of TaskHost overhead in the `-mt` measurement that motivated this work). All 10 are already migrated. None of the 18 below executes during a repo build — they run in publish, sign and Helix-orchestration stages, one invocation at a time, off the parallel critical path. Annotating them cannot save build time.
- **Poor risk/reward.** These are 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 meaningfully, so a regression surfaces in a release pipeline rather than in CI. The migration already produced two rounds of CI regressions in tasks that *do* run in PR builds — and were caught precisely for that reason.
- **Reviewability.** #17381 is already ~190 files of mechanical change. Folding a refactor of the 2,082-line `PublishArtifactsInManifestBase` into it would make it unreviewable.

---

## Group 1 — Ambient credentials or pipeline environment (8)

`CreateAzureDevOpsFeed`, `CreateNewAzureContainer`, `CreateAzureContainerIfNotExists`, `PublishArtifactsInManifest`, `PublishBuildToMaestro`, `PublishSignedAssets`, `SendHelixJob`, `InstallDotNetTool`

These either build an `AzureCliCredential` when no PAT/AccountKey is supplied, or read pipeline variables straight from the process environment.

The largest instance is `PublishArtifactsInManifestBase`, which reads eight pipeline variables directly via `Environment.GetEnvironmentVariable` to choose a credential strategy (`AZURESUBSCRIPTION_CLIENT_ID`, `AZURESUBSCRIPTION_TENANT_ID`, `AZURESUBSCRIPTION_SERVICE_CONNECTION_ID`, `SYSTEM_ACCESSTOKEN`, `SYSTEM_OIDCREQUESTURI` at lines 1008-1012; `servicePrincipalId`, `idToken`, `tenantId` at lines 1018-1020). `SendHelixJob` is the same shape one layer down: `JobDefinition` reads `BUILD_REPOSITORY_NAME`, `BUILD_SOURCEBRANCH`, `SYSTEM_TEAMPROJECT` and `BUILD_REASON`. `InstallDotNetTool` reaches the environment through `ICommandFactory`.

The mechanical fix is the one already applied to `AzureDevOpsTask`: route through `TaskEnvironment.GetEnvironmentVariable`. Unlike `AzureDevOpsTask` there is no existing funnel helper, so one has to be introduced — and a credential-selection change in the publishing path deserves validation against a real pipeline rather than a PR build.

**Effort:** medium, mostly validation rather than code.

## Group 2 — Process-wide static state (4)

`GenPartialFacadeSource`, `NotSupportedAssemblyGenerator`, `SignCheckTask`, `SingleError`

**`GenPartialFacadeSource` / `NotSupportedAssemblyGenerator`** — their shared base `RoslynBuildTask.Execute` subscribes an *instance* method to `AssemblyLoadContext.Resolving`, which is process-wide state:

```csharp
AssemblyLoadContext currentContext = AssemblyLoadContext.GetLoadContext(Assembly.GetExecutingAssembly())!;
currentContext.Resolving += ResolverForRoslyn;
try { return ExecuteCore(); }
finally { currentContext.Resolving -= ResolverForRoslyn; }
```

`ResolverForRoslyn` closes over the instance's `RoslynAssembliesPath`. With two instances executing concurrently both handlers are attached, so a resolution triggered by task A can be serviced by task B's handler and satisfied from **B's** `RoslynAssembliesPath`. If the paths differ, that is exactly the "two different versions of the Roslyn assemblies from a different location" hazard the method's own comment exists to prevent. The fix is a genuine design change: register a single process-wide resolver once, asserting all callers agree on the path, or load into a dedicated `AssemblyLoadContext` per task.

**`SignCheckTask`** — builds a `SignatureVerificationManager` through `SignCheckRunner` whose static `_fileVerifiers` state reaches well beyond the task class.

`SignCheckTask` additionally has a **shared-core constraint** that none of the other tasks here have: `Microsoft.DotNet.SignCheckLibrary` is referenced by both the task *and* `Microsoft.DotNet.SignCheck`, which is `OutputType=Exe`. It in turn depends on `Microsoft.DotNet.MacOsPkg.Core`, shared with `Microsoft.DotNet.MacOsPkg.Cli`, so the constraint is transitive. Retyping those libraries' signatures to `AbsolutePath` would force `Microsoft.Build.Framework` into two console applications, and is semantically wrong there — a CLI has no project directory, and its correct base is `Environment.CurrentDirectory`. This task must therefore resolve at the task boundary and pass plain absolute strings down; see the note below.

**`SingleError`** — `BuildEngine4.GetRegisteredTaskObject` followed by `RegisterTaskObject` is not atomic, so two concurrent instances can both observe the sentinel as absent and both report. Smallest and most self-contained item in this issue.

**Effort:** low for `SingleError`; medium for `RoslynBuildTask` (64 lines, but needs a deliberate design decision); high for `SignCheckTask`, which needs both the static state and the shared-core problem solved.

## Group 3 — Unresolved paths flowing into helper chains (6)

`GenAPITask`, `PushToBuildStorage`, `SignToolTask`, `CreateLightCommandPackageDrop`, `CreateVisualStudioWorkload`, `CreateVisualStudioWorkloadSet`

These resolve paths against the process-wide current directory somewhere below the task class, so the fix is not an annotation but making the paths resolve correctly below the task — for these six, threading `AbsolutePath` through the helper chain, as was done for Packaging, GenFacades, Feed, NuGetRepack, SharedFramework.Sdk, PackageTesting and XliffTasks in #17381. That approach is safe for those assemblies specifically; see the note on shared cores below before applying it elsewhere. `MSBuildTask0005` is suppressed at these entry points.

- `GenAPITask` — `HostEnvironment` expands variables and probes with `Directory.Exists`/`File.Exists` on raw input.
- `PushToBuildStorage` — six `*LocalStorageDir` inputs plus artifact items that would all have to migrate together.
- `CreateLightCommandPackageDrop` — most of its execution sits in `CreateWixCommandPackageDropBase` (346 lines), which the validator also flags for inconsistent path resolution.
- `SignToolTask`, `CreateVisualStudioWorkload`, `CreateVisualStudioWorkloadSet` — helper chains spanning 29 and 62 files respectively.

**Effort:** high. Reasonable to leave indefinitely unless the sign/Wix packaging code is being touched for other reasons.

---

### Two ways to fix a path chain, and how to choose

#17381 threaded `AbsolutePath` through helper signatures in seven assemblies. That is **not** the general pattern, and copying it blindly will break repos that share task code with other hosts.

`AbsolutePath` lives in `Microsoft.Build.Framework` and can only be produced by `TaskEnvironment.GetAbsolutePath`. Putting it in a signature therefore imposes an MSBuild dependency on every caller. That is acceptable only when the assembly is MSBuild-only.

- **Thread `AbsolutePath`** when the helper is MSBuild-only — task base classes, and libraries that already reference `Microsoft.Build.*` and have no non-MSBuild consumer. All seven assemblies in #17381 qualify: their only consumers are task assemblies and test projects, and as `IsBuildTaskProject` packages they ship under `tools/` with `IncludeBuildOutput=false`, so the widened signatures are not reachable through `PackageReference`.
- **Resolve at the task boundary** whenever the code is shared with a CLI, a unit-test host, or anything else that does not run under MSBuild. The task is a host adapter and is the only component that knows the project directory; it resolves its inputs once, then passes plain absolute strings into a core that stays host-agnostic. The same core keeps working in a CLI, where relative paths correctly resolve against `Environment.CurrentDirectory`.

Boundary resolution reads best as a single normalization at the top of `Execute`, rather than wrapping each call site:

```csharp
public override bool Execute()
{
string runtimeFile = TaskEnvironment.GetAbsolutePath(RuntimeFile);
// everything downstream uses the local
```

Before wrapping any input, check whether it is `[Required]`. `GetAbsolutePath(null)` throws, whereas `File.Exists(null)` and `Directory.Exists(null)` return `false` — mechanically wrapping those two turns an optional unset property into a hard failure. That regression took out every source-build leg during #17381.

### Note on `IMultiThreadableTask`

Several of these tasks keep their `IMultiThreadableTask` implementation and `TaskEnvironment`-based path handling. That is deliberate and is not a half-migration: **routing is decided by the attribute alone**, so the interface only causes `TaskEnvironment` to be injected, which makes path resolution correct in either mode. The interface also cannot ever become a routing signal — `ToolTask` itself implements it, so that would opt in every `ToolTask`-derived task in the ecosystem (see dotnet/msbuild#14779). 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.

### Guarding against new unannotated tasks

Once dotnet/msbuild#14789 ships, its `MSBuildTask0012` fires on every concrete `ITask` lacking the attribute — the complete guard for this work. It can be enabled in `eng/MultiThreadableTaskAnalyzer.globalconfig` with `dotnet_diagnostic.MSBuildTask0012.severity = warning`, and the tasks above suppressed individually with `[SuppressMessage]`.

Do **not** enable it via `msbuild_task_analyzer.scope = require_multithreadable`: that also widens `MSBuildTask0001`-`0011` from `multithreadable_only` to all tasks, which floods exactly these 18 with the diagnostics Arcade deliberately silences.

### Suggested order

1. **`SingleError`** — smallest, self-contained, and a real race today.
2. **`RoslynBuildTask`** (`GenPartialFacadeSource`, `NotSupportedAssemblyGenerator`) — the only other item here that is an actual latent correctness bug rather than a migration blocker, since two `GenFacades` tasks *can* run concurrently in a repo build.
3. **Group 1** — alongside any other publishing change that already requires pipeline validation.
4. **Group 3** — opportunistically, or never.

cc @dotnet/dnceng

Contributor guide

Open the contributing guide

Research direction

Begin with the SingleError entry point and its BuildEngine4 GetRegisteredTaskObject/RegisterTaskObject sequence, then review RoslynBuildTask and its GenPartialFacadeSource and NotSupportedAssemblyGenerator callers. Treat the selected fix as complete only when the relevant race or migration blocker is addressed without violating the shared-core and CLI constraints; no named test target is provided, so validation must follow the affected host or pipeline.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
build-system, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.