[JanProvaznik]

Harden exceptions when connecting to server

[Copilot]

Pull request overview

This PR hardens MSBuild Server client behavior by broadening exception handling around server-state detection (mutex checks / launch mutex acquisition) so unexpected, non-critical failures don’t escape and instead trigger a structured fallback.

Changes:

• Wrap ServerNamedMutex.WasOpen in a non-critical exception filter and treat failures as “not open”, with tracing.
• Broaden MSBuildClient server-state exception handling from IOException to any non-critical Exception (still excluding PathTooLongException), mapping failures to UnknownServerState.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/Build/BackEnd/Node/ServerNamedMutex.cs	Adds exception handling + trace logging around Mutex.TryOpenExisting to avoid propagating non-critical failures.
src/Build/BackEnd/Client/MSBuildClient.cs	Broadens catch filters during server-state checks / launch-mutex acquisition to avoid crashing and return UnknownServerState.

[github-actions]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Review — Harden exceptions when connecting to server

The approach is sound: broadening from catch (IOException) to catch (Exception when !IsCritical) is the established MSBuild idiom (used in NodeEndpointOutOfProcBase, NodePipeServer, etc.) and correctly handles the Win32Exception("Connection timed out") root cause from #7993. One concrete correctness gap and two documentation/diagnostics improvements are worth addressing.

#	Dimension	Verdict
22	Correctness & Edge Cases	🟠 MODERATE
18	Documentation Accuracy	🟠 MODERATE
4	Test Coverage & Completeness	🟡 MAJOR
6/14	Logging & Diagnostics / Naming	🔵 NIT

✅ 20/24 dimensions clean.

• Correctness — WasOpen() catch (line 46) lacks && ex is not PathTooLongException, silently swallowing the exception that both MSBuildClient.cs catch blocks explicitly re-throw. See inline comment on ServerNamedMutex.cs:46.
• Documentation — Deleted comments were the only explanation for why Exception is caught instead of IOException and why PathTooLongException is excluded. A future contributor with no context will "fix" it back. See inline comment on MSBuildClient.cs:218.
• Test Coverage — The core fix (WasOpen returning false instead of throwing on non-IOException) has no regression test. Since Mutex.TryOpenExisting can't be injected today, a minimal approach is an integration-level test that verifies Execute() returns UnknownServerState (not a crash) when given a mutex name that causes a non-IOException. At minimum, the PathTooLongException propagation contract should be verified.
• Logging — TryLaunchServer()'s catch emits the same "Failed to obtain the current build server state" message as Execute()'s catch, making the two failure modes indistinguishable in a trace log. See inline comment on MSBuildClient.cs:468.

Generated by Expert Code Review (on open) for #14292 · 1.3K AIC · ⊞ 30.4K