Fix TraceEngine file contention deadlock in multithreaded mode

.github/agents/expert-reviewer.md

@@ -40,15 +40,17 @@ Apply **all** dimensions on every review, weighted by file location (see [Folder
 **Severity: BLOCKING**
 
 **Rules:**
-1. Gate breaking behavior changes behind a ChangeWave. See `../../documentation/wiki/ChangeWaves.md`.
-2. New warnings are breaking changes for `-WarnAsError` builds. Gate behind ChangeWave or emit as `Message`.
-3. Make behavioral changes opt-in by default.
-4. SDK target changes must preserve backward compatibility with existing project files.
-5. Never remove CLI switches or aliases — deprecate with warnings first.
+1. Breaking changes are basically forbidden. Customers expect any project that built before to keep building.
+2. In cases where we expect no breaking behavior but there is a slight chance it should be gated behind a ChangeWave. See `../../documentation/wiki/ChangeWaves.md`.
+3. New warnings are breaking changes because customers set `-WarnAsError` in their builds. Gate them behind a ChangeWave or emit them as `Message`.
+4. Make behavioral changes opt-in by default.
+5. SDK target changes must preserve backward compatibility with existing project files.
+6. Never remove CLI switches or aliases — deprecate with warnings first.
 
 **CHECK — Flag if:**
-- [ ] A behavioral change has no ChangeWave gate
-- [ ] New warnings emitted without considering `WarnAsError` impact
+- [ ] A change can be percieved by customers as breaking
+- [ ] Risky behavioral change has no ChangeWave gate
+- [ ] New warnings emitted
 - [ ] A property default value has changed
 - [ ] A CLI switch or alias removed rather than deprecated
 - [ ] Output format changes could break downstream consumers
@@ -62,7 +64,7 @@ Apply **all** dimensions on every review, weighted by file location (see [Folder
 See `../../documentation/wiki/ChangeWaves.md` and `../../documentation/wiki/ChangeWaves-Dev.md`.
 
 **Rules:**
-1. Use the correct ChangeWave version — must match the upcoming MSBuild release, not the current one.
+1. Use the correct ChangeWave version — must match the upcoming MSBuild release, not the current one
 2. Test both enabled and disabled paths — `MSBuildDisableFeaturesFromVersion` opt-out must work.
 3. Document the ChangeWave in error messages, specs, and the ChangeWaves tracking file.
 
@@ -87,7 +89,7 @@ Hot paths: `Evaluator.cs`, `Expander.cs`, file I/O operations.
 4. Profile before optimizing — claims require evidence.
 
 **CHECK — Flag if:**
-- [ ] LINQ in a loop on a hot path
+- [ ] LINQ
 - [ ] Strings allocated that could be reused or avoided
 - [ ] A value recomputed on every call when cacheable
 - [ ] `Dictionary` for <10 items, or `List` for frequent lookups of >100 items
@@ -123,7 +125,7 @@ See `../../documentation/assigning-msb-error-code.md`.
 **Rules:**
 1. Messages must be actionable — state what happened, why, and how to fix.
 2. Use `MSBxxxx` format error/warning codes, each unique across the codebase.
-3. Use `ResourceUtilities.FormatResourceStringStripCodeAndKeyword` for formatting.
+3. Use `ResourceUtilities` for formatting.
 4. Use correct severity levels — don't emit errors for non-fatal conditions.
 
 **CHECK — Flag if:**
@@ -243,13 +245,15 @@ See `../../documentation/wiki/Microsoft.Build.Framework.md`.
 2. Handle .NET Framework vs .NET Core differences explicitly.
 3. Handle UNC paths, long paths (`\\?\`), and symlinks.
 4. Build output must not differ based on build machine OS.
+5. Use Microsoft.IO Redist when targeting .NET Framework for better performance.
 
 **CHECK — Flag if:**
 - [ ] Hardcoded path separators
 - [ ] Windows-only APIs without cross-platform fallback
 - [ ] File path case sensitivity not considered
 - [ ] Code assumes `\r\n` newlines
 - [ ] `.NET Framework`-only API without `#if` guard
+- [ ] System.IO used instead of Microsoft.IO Redist for .NET Framework file operations
 
 ---
 
@@ -277,13 +281,14 @@ See `../../documentation/wiki/Microsoft.Build.Framework.md`.
 
 **Severity: BLOCKING**
 
-See `../../documentation/wiki/Nodes-Orchestration.md`.
+See `../../documentation/wiki/Nodes-Orchestration.md`, `../../documentation/specs/multithreading/multithreaded-msbuild.md`
 
 **Rules:**
 1. Shared mutable state must be thread-safe (`ConcurrentDictionary`, `Interlocked`, explicit locking).
 2. Synchronize access to state from multiple threads or nodes.
 3. Handle in-proc vs out-of-proc node differences — behavior must be correct in both.
 4. Consider IPC concurrency: message ordering, reentrancy, serialization.
+5. Check assumptions where only one thread accesses a code path.
 
 **CHECK — Flag if:**
 - [ ] Shared field read/written without synchronization
@@ -341,6 +346,7 @@ See `../../documentation/ProjectReference-Protocol.md`.
 2. Match surrounding code style — MSBuild conventions may differ from general .NET guidance.
 3. Handle nullability explicitly with annotations and `is null`/`is not null`.
 4. Track .NET version constraints with `// TODO` for unavailable APIs.
+5. new code should have nullable enabled even if some parts of codebase don't
 
 **CHECK — Flag if:**
 - [ ] `using` block where surrounding code uses `using` declarations
@@ -453,12 +459,14 @@ See `../../documentation/High-level-overview.md` and `../../documentation/Built-
 2. Verify behavior matches documented semantics.
 3. Validate fixes against original repro steps.
 4. Validate inputs early — fail fast with clear errors.
+5. Imagine exotic scenarios that could break assumptions. Like a red teamer trying to break MSBuild with weird project files or build environments.
 
 **CHECK — Flag if:**
 - [ ] New code path doesn't handle null/empty inputs
 - [ ] Bug fix doesn't include original repro as test
 - [ ] Boundary conditions not considered (off-by-one, max-length, empty)
 - [ ] Input validation missing at public API boundaries
+- [ ] Code relies on assumptions that exotic inputs could break
 
 ---
 
@@ -483,12 +491,12 @@ See `../../documentation/High-level-overview.md` and `../../documentation/Built-
 **Severity: BLOCKING**
 
 **Rules:**
-1. Security-relaxing parameters (e.g., `TrustServerCertificate`) must require explicit user opt-in.
+1. Never regress security
 2. Task type loading must unify to currently-running MSBuild assemblies.
 3. Consider path traversal, symlink following, and temp file safety.
 
 **CHECK — Flag if:**
-- [ ] Security-relaxing flag applied unconditionally
+- [ ] Security-relaxing assumption
 - [ ] Task assembly loading accepts untrusted paths without validation
 - [ ] File operations exploitable via symlinks or path traversal
 - [ ] Credentials or tokens logged/stored insecurely

.github/instructions/shared-code.instructions.md

@@ -29,7 +29,3 @@ Code in `src/Shared/` is linked (compiled) into multiple MSBuild assemblies. A b
 
 * Handle .NET Framework vs .NET Core differences with appropriate `#if` guards.
 * Environment variable access patterns differ across platforms — use the shared helpers.
-
-## Related Documentation
-
-* [Contributing Code](../../documentation/wiki/Contributing-Code.md)

.github/instructions/tasks.instructions.md

@@ -33,7 +33,7 @@ Built-in tasks ship with MSBuild and cannot be independently versioned.
 
 ## Multithreaded Task Migration
 
-* Tasks must not hold locks across yield points.
+* All built-in tasks implement `IMultiThreadableTask` with a default `TaskEnvironment` backed by `MultiProcessTaskEnvironmentDriver.Instance`.
 * Shared static state is a concurrency hazard in multi-process builds.
 
 ## Related Documentation

.github/skills/multithreaded-task-migration/SKILL.md

@@ -18,7 +18,7 @@ b. Implement `IMultiThreadableTask` **only if** the task needs `TaskEnvironment`
 [MSBuildMultiThreadableTask]
 public class MyTask : Task, IMultiThreadableTask
 {
-    public TaskEnvironment TaskEnvironment { get; set; }
+    public TaskEnvironment TaskEnvironment { get; set; } = new TaskEnvironment(MultiProcessTaskEnvironmentDriver.Instance);
     ...
 }
 ```
@@ -61,7 +61,7 @@ The [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/P
 
 ## Updating Unit Tests
 
-Every test creating a task instance must set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()`.
+Built-in MSBuild tasks now initialize `TaskEnvironment` with a `MultiProcessTaskEnvironmentDriver`-backed default. Tests creating instances of built-in tasks no longer need manual `TaskEnvironment` setup. For custom or third-party tasks that implement `IMultiThreadableTask` without a default initializer, set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()`.
 
 ## APIs to Avoid
 
@@ -276,7 +276,7 @@ Assertions: Execute() return value, [Output] exact string, error message content
 ## Sign-Off Checklist
 
 - [ ] `[MSBuildMultiThreadableTask]` on every concrete class (not just base — `Inherited=false`)
-- [ ] `IMultiThreadableTask` only on classes that use `TaskEnvironment` APIs
+- [ ] `IMultiThreadableTask` on classes that use `TaskEnvironment` APIs, with default initializer `= new TaskEnvironment(MultiProcessTaskEnvironmentDriver.Instance)`
 - [ ] Every `[Output]` property: exact string value matches pre-migration
 - [ ] Every `Log.LogError`/`LogWarning`: path in message matches pre-migration (use `OriginalValue`)
 - [ ] Every `GetAbsolutePath` call: null/empty exception behavior matches old code path
@@ -285,7 +285,7 @@ Assertions: Execute() return value, [Output] exact string, error message content
 - [ ] Every `??` or `?.` added: verified it doesn't swallow a previously-thrown exception
 - [ ] No `AbsolutePath` leaks into user-visible strings unintentionally
 - [ ] Helper methods traced for internal File API usage with non-absolutized paths
-- [ ] All tests set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()`
+- [ ] Tests for custom tasks set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()` (built-in tasks have a default)
 - [ ] Cross-framework: tested on both net472 and net10.0
 - [ ] Concurrent execution: two tasks with different project directories produce correct results
 - [ ] No forbidden APIs (`Environment.Exit`, `Console.*`, etc.)

azure-pipelines/vs-insertion-experimental.yml

@@ -24,20 +24,8 @@ parameters:
     displayName: 'Insertion Target Branch (select for manual insertion)'
     values:
       - main
-      - rel/d18.6
-      - rel/d18.5
-      - rel/d18.4
-      - rel/d18.3
-      - rel/d18.0
-      - rel/d17.14
-      - rel/d17.13
-      - rel/d17.12
-      - rel/d17.11
-      - rel/d17.8
-      - rel/d17.6
-      - rel/d17.3
-      - rel/d17.0
-      - rel/d16.11
+      - rel/insiders
+      - rel/stable
 
 variables:
   - name: TeamName
@@ -120,28 +108,13 @@ extends:
           inputs:
             targetType: inline
             script: |
-              # Extract VS version from branch name if it follows exp/vsXX.Y-somename pattern
-              $fullBranch = "$(resources.pipeline.MSBuildExpPerf.sourceBranch)"
               $parameterTargetBranch = "${{ parameters.TargetBranch }}"
-              $detectedTarget = "main" # Default target branch
-
-              # Try to match the pattern with regex
-              if ($fullBranch -match "exp/vs(\d+)\.(\d+).*") {
-                $major = $matches[1]
-                $minor = $matches[2]
-                $targetBranch = "rel/d$major.$minor"
-                Write-Host "Detected version pattern in branch: $major.$minor"
-                Write-Host "Setting target branch to $targetBranch"
-                $detectedTarget = $targetBranch
-              } else {
-                Write-Host "No version pattern detected in branch, using default target: main"
-              }
 
               # Determine which target branch to use based on build reason
               $finalTargetBranch = $parameterTargetBranch
               if ("$(Build.Reason)" -eq "ResourceTrigger" -or "$(Build.Reason)" -eq "PipelineCompletedTrigger") {
-                Write-Host "Build was triggered automatically, using detected target branch: $detectedTarget"
-                $finalTargetBranch = $detectedTarget
+                Write-Host "Build was triggered automatically, using default target branch: main"
+                $finalTargetBranch = "main"
               } else {
                 Write-Host "Build was triggered manually, using parameter target branch: $parameterTargetBranch"
               }

azure-pipelines/vs-insertion.yml

@@ -28,7 +28,7 @@ resources:
     branch: main # for daily main scheduled insertion
     trigger:
       branches:
-        include: # trigger as a followup to servicing CI
+        include: # trigger as a followup to CI on servicing branches
           - vs*
   repositories:
   - repository: 1ESPipelineTemplates
@@ -44,20 +44,8 @@ parameters:
     values:
       - auto
       - main
-      - rel/d18.6  
-      - rel/d18.5
-      - rel/d18.4
-      - rel/d18.3
-      - rel/d18.0
-      - rel/d17.14
-      - rel/d17.13
-      - rel/d17.12
-      - rel/d17.11
-      - rel/d17.10
-      - rel/d17.8
-      - rel/d17.6
-      - rel/d17.3
-      - rel/d17.0
+      - rel/insiders
+      - rel/stable
   - name: DropRetentionDays
     default: 183
     type: number
@@ -69,44 +57,11 @@ parameters:
 
 variables:
   # `auto` should work every time and selecting a branch in parameters is likely to fail due to incompatible versions in MSBuild and VS
-  - name: AutoInsertTargetBranch
-    ${{ if eq(variables['Build.SourceBranchName'], 'vs18.6') }}:
-      value: 'rel/d18.6'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs18.5') }}:
-      value: 'rel/d18.5'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs18.4') }}:
-      value: 'rel/d18.4'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs18.3') }}:
-      value: 'rel/d18.3'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs18.0') }}:
-      value: 'rel/d18.0'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs17.14') }}:
-      value: 'rel/d17.14'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs17.13') }}:
-      value: 'rel/d17.13'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs17.12') }}:
-      value: 'rel/d17.12'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs17.11') }}:
-      value: 'rel/d17.11'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs17.8') }}:
-      value: 'rel/d17.8'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs17.6') }}:
-      value: 'rel/d17.6'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs17.3') }}:
-      value: 'rel/d17.3'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs17.0') }}:
-      value: 'rel/d17.0'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'main') }}:
-      value: 'main'
-    ${{ elseif eq(variables['Build.SourceBranchName'], 'vs17.15') }}:
-      value: 'main'
-    ${{ else }}:
-      value: ''
   - name: InsertTargetBranch
     ${{ if not(eq(parameters.TargetBranch, 'auto')) }}:
       value: ${{ parameters.TargetBranch }}
     ${{ else }}:
-      value: $(AutoInsertTargetBranch)
+      value: 'main'
   - name:  TeamName
     value: msbuild
   - name: TeamEmail
@@ -227,37 +182,10 @@ extends:
               $MicrosoftNETStringToolsPackageVersion = $packageFile.BaseName.TrimStart("Microsoft.NET.StringTools")
               Write-Host "Setting MicrosoftNETStringToolsPackageVersion to '$MicrosoftNETStringToolsPackageVersion'"
               Write-Host "##vso[task.setvariable variable=MicrosoftNETStringToolsPackageVersion]$($MicrosoftNETStringToolsPackageVersion)"
-              $props = @(
-                      "VS.ExternalAPIs.MSBuild=$MSBuild_ExtApisPackageVersion",
-                      "Microsoft.Build=$MicrosoftNETStringToolsPackageVersion",
-                      "Microsoft.Build.Framework=$MicrosoftNETStringToolsPackageVersion",
-                      "Microsoft.Build.Tasks.Core=$MicrosoftNETStringToolsPackageVersion",
-                      "Microsoft.Build.Utilities.Core=$MicrosoftNETStringToolsPackageVersion",
-                      "Microsoft.NET.StringTools=$MicrosoftNETStringToolsPackageVersion"
-                  )
-              # servicing branches until 17.12 also include Microsoft.Build.Engine and Microsoft.Build.Conversion.Core
-              if ("$(InsertTargetBranch)" -in @("rel/d17.0", "rel/d17.3", "rel/d17.6", "rel/d17.8", "rel/d17.10", "rel/d17.11", "rel/d17.12"))
 
-              {
-                  $props += @(
-                      "Microsoft.Build.Conversion.Core=$MicrosoftNETStringToolsPackageVersion",
-                      "Microsoft.Build.Engine=$MicrosoftNETStringToolsPackageVersion"
-                  )
-              }
-              $propsValue = $props -join ";"
-
-              # For 18.0+, don't update PackageReferences so that VS packages like VSSDK
-              # can choose their own versioning and don't always depend on a super fresh MSBuild
-              if ("$(InsertTargetBranch)" -in @("rel/d17.10", "rel/d17.12", "rel/d17.14"))
-              {
-                  Write-Host "Setting InsertPackagePropsValues to '$propsValue'"
-                  Write-Host "##vso[task.setvariable variable=InsertPackagePropsValues]$($propsValue)"
-              }
-              else
-              {
-                  Write-Host "Unsetting InsertPackagePropsValues"
-                  Write-Host "##vso[task.setvariable variable=InsertPackagePropsValues]"
-              }
+              # InsertPackagePropsValues was needed for old servicing branches (rel/d17.x) but is not used for main
+              Write-Host "Unsetting InsertPackagePropsValues"
+              Write-Host "##vso[task.setvariable variable=InsertPackagePropsValues]"
 
               # autocomplete main
               $autocomplete = "false"

documentation/specs/multithreading/multithreaded-msbuild.md

@@ -138,6 +138,8 @@ The scheduler is already capable of juggling multiple projects, and there's alre
 
 The scheduler should  be responsible for creating the appropriate combination of nodes (in-proc, out-of-proc, and thread nodes) based on the execution mode (multi-proc or multithreaded, CLI or Visual Studio scenarios). It will then coordinate projects execution through the node abstraction. Below is the diagram for cli multi-threaded mode creating all the thread nodes in the entry process for simplicity--in final production these will be in an [MSBuild Server process](#msbuild-server-integration).
 
+In the current implementation, enabling multithreaded mode implies that all worker nodes are in-proc. Out-of-proc worker-node topologies for multithreaded execution remain future work.
+
 ```mermaid
 sequenceDiagram
 
@@ -234,7 +236,7 @@ The `MSBuildMultiThreadableTaskAttribute` is **non-inheritable** (`Inherited = f
 * Derived classes cannot accidentally inherit thread-safety assumptions from base classes
 * The routing decision is always explicit and visible in the task's source code
 
-Tasks may optionally implement `IMultiThreadableTask` to access `TaskEnvironment` APIs, but only the attribute determines routing behavior.
+Tasks may optionally implement `IMultiThreadableTask` to access `TaskEnvironment` APIs, but only the attribute determines routing behavior. If task implements `IMultiThreadableTask`, `TaskEnvironment` should be backed by `MultiProcessTaskEnvironmentDriver.Instance`, which acts as a fallback for explicit instantiation and task host scenarios.
 
 ## Tasks transition
 
@@ -246,6 +248,8 @@ To ease task authoring, we will provide a Roslyn analyzer that will check for kn
 
 We need to ensure the support for multithreaded mode in Visual Studio builds. Currently, the entry node for MSBuild runs entirely within the devenv process, but the majority of the build operation are run in the MSBuild worker processes, because project systems set `BuildParameters.DisableInProcNode=true`. In multithreaded mode, all of the task execution must continue to be out of process. To address this, unlike the CLI scenario, we will move all thread nodes to the out-of-process MSBuild process, keeping only the scheduler in devenv.
 
+This section describes the intended Visual Studio topology, not the current implementation invariant that multithreaded mode implies all worker nodes are in-proc.
+
 ```mermaid
 sequenceDiagram