Skip to content

Avoid constructor-time ownership for compiler client mutexes - #85675

Merged
jjonescz merged 3 commits into
dotnet:mainfrom
JanProvaznik:janprovaznik-compiler-client-mutex-workaround
Sep 21, 2026
Merged

jjonescz merged 3 commits into
dotnet:mainfrom
JanProvaznik:janprovaznik-compiler-client-mutex-workaround

Conversation

@JanProvaznik

@JanProvaznik JanProvaznik commented Sep 16, 2026 •

Copy link
Copy Markdown
Member

Summary

Related to dotnet/runtime#134043 (transferred from #85264). This is a compiler-client workaround, not a fix for the underlying runtime issue.

  • Open native client mutexes without initial ownership, then use the existing synchronous timed acquisition before checking/starting/connecting to the server. Keep Mono's eager file-lock behavior and constructor-failure fallback, and preserve constructor-time ownership for server-election/cache callers.
  • Record ownership when WaitOne reports abandonment, then rethrow for existing caller handling. Include both ApplicationException and InvalidOperationException in release diagnostics, preserving thread IDs and the original inner exception.
  • Cover explicit acquisition, abandoned ownership release, release diagnostics, timeout without server startup, and concurrent same-pipe requests (4 batches of 8). The test-thread helper rethrows captured exceptions through ExceptionDispatchInfo, preserving the original assertion and stack.

No global lock, await across mutex ownership, shared-compilation disablement, task isolation, or runtime changes.

Upstream validation

The initial upstream port passed 18/18 focused tests on each of net10.0 and net472, zero skips, on Windows with repository SDK 11.0.100-preview.6.26359.118 and RunAnalyzersDuringBuild=true: all BuildServerConnectionTests, both CompilerServerApiTest.Mutex* server-election tests, and the three existing cache-lock tests. Microsoft.Build.Tasks.CodeAnalysis built for net10.0/net472, and Microsoft.Build.Tasks.CodeAnalysis.Sdk for net472, with analyzers and zero warnings/errors.

What the five tests prove

Three independent source-level mutants were compiled successfully and exercised against both the originally published tests and the final exception-preserving helper. Compilation failures were not counted as red results.

Test Purpose and mutant Observed result
ClientMutexRequiresExplicitAcquisition Strategy guard: delegate the native client factory to the old eager OpenOrCreateMutex, retaining the new API shape. This does not independently reproduce the runtime race. Red: Assert.False(holdsMutex), expected false, actual true.
AbandonedClientMutexIsReleased Ownership regression: omit IsLocked = true from the abandonment catch but retain the rethrow. An extra raw handle keeps the named object alive. Red: the other thread's TryLock(0) assertion expected true, actual false. The final helper reports this original assertion directly rather than wrapping it in Assert.Null.
ClientMutexReleaseFailureIncludesThreadIds Platform-specific diagnostic regression: restore the ApplicationException-only filter. Red on Linux .NET 11.0.0-rc.1.26453.118: exact exception assertion expected System.Exception, actual System.InvalidOperationException with "Cannot release a lock that is not owned by the current thread." The same mutant passed on Windows, so Windows alone does not cover this regression.
ClientMutexTimeoutDoesNotStartServer Behavior safeguard: timeout returns CannotConnectResponse without attempting server creation. Passed with both the old-factory mutant and restored implementation; not claimed as a deterministic old-code regression.
UseExistingServerConcurrently Stress safeguard: 32 requests use the existing server without creation/fallback. Passed with both the old-factory mutant and restored implementation; not a guaranteed runtime-race reproducer.

After restoring the unchanged production DLL, all five final tests passed, zero skips, on Windows with the net10.0 payload running on .NET 11.0.0-preview.6.26359.118, Windows with the net472 payload running on .NET Framework 4.8.9345.0, and Ubuntu/WSL with the affected .NET 11.0.0-rc.1.26453.118 runtime. Linux used DOTNET_ROLL_FORWARD=Major and DOTNET_ROLL_FORWARD_TO_PRERELEASE=1 for the net10.0 payload.

The final test assemblies were recompiled into isolated output using the recorded analyzer-enabled compiler invocations. The original production source and all 2,033 original build-output files remained hash-identical. Changed-file formatting and diff checks passed; MSBuild invocations captured separate binary logs. The follow-up commit changes only the test helper/import, not production logic or the SDK-gated patch.

Matched SDK gate (before the upstream port)

Ubuntu 24.04 x64 under WSL; SDK 11.0.100-rc.1.26453.118, runtime 11.0.0-rc.1.26453.118. Both runs used the same 32 project references plus app, harness, and flags:

-mt -m:16 -nr:false -t:Rebuild -p:PublishAot=false -p:SelfContained=false -p:UseSharedCompilation=true

Stock failed on build 15: native initial acquisition returned EBUSY, then ServerNamedMutex.Dispose failed on the same native thread. The patched SDK passed all 50 builds. Replay of all 50 binlogs using the matching SDK counted 1,650 Csc tasks and 1,650 compiler-server completions, with zero command-line fallbacks, external TaskHost routing messages, or errors. No initial-acquisition EBUSY or ownership failure was recorded in the patched run.

The task DLL was rebuilt from SDK-matching VMR sources at 3c8d132bfb012af4bbba80de22149a9f58e737f0, using stock SDK MSBuild assemblies and preserving the protocol commit hash. This was an unsigned development assessment overlay, not an officially source-built current-main SDK. Full SDK file hashes differ only for the task DLL and its added PDB; the compiler server, runtime, targets, and other files are unchanged. The production diff in this upstream port matches the gated patch exactly.

Limits: Both SDK runs used the same observation-only native library and first-chance hook, which may affect timing. Finite stress results do not establish absence of all races or a customer failure rate. Mono/macOS execution and performance impact were not assessed.

Microsoft Reviewers: Open in CodeFlow

Open native client mutexes without initial ownership and acquire them through the existing timed synchronous wait. Preserve Mono and server-election behavior, track abandoned ownership for disposal, and retain thread diagnostics for release failures.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service dotnet-policy-service Bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Sep 16, 2026
Rethrow captured test-thread exceptions through ExceptionDispatchInfo after the bounded join so failed ownership assertions retain their original type and stack.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@JanProvaznik
JanProvaznik marked this pull request as ready for review September 16, 2026 12:07
@JanProvaznik
JanProvaznik requested a review from a team as a code owner September 16, 2026 12:07
Copilot AI lite review requested due to automatic review settings September 16, 2026 12:07
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The mutex ownership and cross-runtime concurrency changes require final human review.

Pull request overview

This PR avoids constructor-time ownership races for compiler client mutexes while preserving existing server/cache behavior.

Changes:

  • Adds explicit client-mutex acquisition and abandonment tracking.
  • Expands release diagnostics for runtime exception differences.
  • Adds mutex lifecycle, timeout, and concurrency regression tests.
File summaries
File Summary
src/Compilers/Shared/BuildServerConnection.cs Implements non-owning client mutex acquisition and ownership-safe disposal.
src/Compilers/Server/VBCSCompilerTests/BuildServerConnectionTests.cs Adds mutex lifecycle and concurrent-request regression coverage.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

@jjonescz jjonescz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaredpar @dotnet/roslyn-compiler for a second review

Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
Copilot AI review requested due to automatic review settings September 17, 2026 15:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Cross-platform mutex behavior warrants final human review.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@jjonescz
jjonescz requested a review from a team September 18, 2026 07:25
@AlekseyTs
AlekseyTs requested a review from jaredpar September 18, 2026 15:42
@jjonescz
jjonescz merged commit 5e5a4f4 into dotnet:main Sep 21, 2026
22 checks passed
@dotnet-policy-service dotnet-policy-service Bot added this to the Next milestone Sep 21, 2026
karolz-ms added a commit to microsoft/aspire that referenced this pull request Sep 24, 2026
## Description

Project Resource V2 makes the dashboard available earlier, but
coordinated project builds can delay full application readiness. .NET
SDK 11 RC1 introduces the `-mt` MSBuild switch, which allows supported
project builds to use multithreaded task execution.

This change:

- Detects the `dotnet` SDK selected for each build context, including
`global.json` selection and prerelease versions.
- Enables `-mt` for coordinated traversal builds and project rebuild
commands when the selected SDK supports the target:
  - Project and generated traversal builds use `11.0.100-rc.1` or newer.
- File-based `.cs` builds use `11.0.100-rtm.26473.104` or newer; stable
`11.0.100` and later are included.
- Uses the stricter file-based build floor because earlier .NET 11
prereleases lack both safe `-mt` argument forwarding and the
compiler-client mutex mitigation. The required fixes landed in
dotnet/sdk#56207, dotnet/roslyn#85675, and dotnet/dotnet#9565.
- Caches version probes by SDK-selection context and refreshes the
result when `global.json` changes.
- Reuses the same provider for existing file-based app and .NET tool SDK
validation.
- Omits `-mt` when version detection fails, preserving the existing
build behavior.

Validation:

- `.\restore.cmd`
- `./restore.sh`
- Affected `Aspire.Hosting` and `Aspire.Hosting.Dotnet` build completed
with zero warnings and errors.
- 47 focused provider, version-policy, coordinated-build, rebuild,
file-app, and .NET-tool tests passed.
- 15 focused `Aspire.Hosting.Tests` file-app SDK policy and
environment-aware provider cases passed.
- 6 focused `Aspire.Hosting.Dotnet.Tests` file-only build, rebuild,
mixed-model coordination, and configured rebuild argument cases passed.
- A repository-local .NET 11 RC1 `.csproj` build completed successfully
with `-mt`.
- A full `Aspire.Hosting.Tests` run encountered two unrelated
`MSBuildTests` timeouts in `dotnet build --disable-build-servers`; both
timeouts reproduced when retried in isolation.

Fixes #19944

## Checklist

- Is this feature complete?
  - [x] Yes. Ready to ship.
  - [ ] No. Follow-up changes expected.
- Are you including unit tests for the changes and scenario tests if
relevant?
  - [x] Yes
  - [ ] No
- Did you add public API?
  - [ ] Yes
    - If yes, did you have an API Review for it?
      - [ ] Yes
      - [ ] No
- Did you add `<remarks />` and `<code />` elements on your triple slash
comments?
      - [ ] Yes
      - [ ] No
  - [x] No
- Does the change make any security assumptions or guarantees?
  - [ ] Yes
    - If yes, have you done a threat model and had a security review?
      - [ ] Yes
      - [ ] No
  - [x] No

---------

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants