dotnet / dotnet/msbuild

Fail fast on connect-to-node when launched child process has already exited

Open
#13,718 2 comments 0 reactions 0 assignees View on GitHub
triaged
Dominant language
C#
Stars
5.5k
Forks
1.5k
Avg merge
1d 8h
Merged PRs (30d)
141

Description

> Blocked on #13715 / #13716. This is a generalization follow-up — once the immediate `DOTNET_CLI_USE_MSBUILD_SERVER=true` regression is resolved, we should make every connect-to-node path fail fast when the launched child has already died.

## Problem

Every "connect to a child MSBuild process" path in the engine ultimately calls `NodeProviderOutOfProcBase.TryConnectToPipeStream`, which does a single blocking `NamedPipeClientStream.Connect(timeout)` ([`NodeProviderOutOfProcBase.cs:804`](https://github.com/dotnet/msbuild/blob/main/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs#L804)). The default timeouts are large:

- Worker node launch: `TimeoutForNewNodeCreation` = **30s** ([line 50](https://github.com/dotnet/msbuild/blob/main/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs#L50), used at [line 414](https://github.com/dotnet/msbuild/blob/main/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs#L414))
- MSBuild Server launch: **20s** (`MSBuildClient.ConnectToServer`)
- TaskHost connect: configurable

If the child process **fails to start or crashes before opening its named pipe** (e.g. apphost can't locate the runtime, missing permissions, missing dependency), the parent waits for the full timeout before reporting failure. The .NET 10.0.300 `DOTNET_CLI_USE_MSBUILD_SERVER=true` regression (#13715) caused exactly this: a 20s hang per build before falling back to in-proc.

The parent **already holds a `Process` handle** to the launched child in every launch path, so we can poll `HasExited` in parallel with the pipe connect and bail out within ~100ms when the child dies, instead of waiting the full timeout.

## Goal

Make `TryConnectToPipeStream` accept an optional `Process` to watch. While the pipe-connect call is waiting, periodically check whether the watched process has exited; if it has, abandon the connect immediately and return a structured failure that includes the exit code.

This must work uniformly on **.NET Framework and .NET (Core)** without `#if NETCOREAPP`, because `Microsoft.Build` multi-targets `$(FullFrameworkTFM);$(LatestDotNetCoreForMSBuild)` ([`Microsoft.Build.csproj:6`](https://github.com/dotnet/msbuild/blob/main/src/Build/Microsoft.Build.csproj#L6)). That rules out `WaitForExitAsync`, `ConnectAsync(timeout, ct)`, and any APIs introduced after .NET Standard 2.0.

## Required design

### Approach

Replace the single `nodeStream.Connect(timeout)` call with a **chunked poll loop** that splits the wait into small slices and checks `Process.HasExited` between slices. Catch the `TimeoutException` thrown by each short `Connect` slice; only convert it to a real failure once the overall timeout elapses or the watched process exits. This avoids needing async or cancellation tokens entirely.

### Sketch

In `NodeProviderOutOfProcBase.cs`:

```csharp
internal static bool TryConnectToPipeStream(
NamedPipeClientStream nodeStream,
string pipeName,
Handshake handshake,
int timeout,
out HandshakeResult result,
Process? watchProcess = null) // NEW optional param
{
const int PollIntervalMs = 100;

if (watchProcess is null || timeout <= PollIntervalMs)
{
// Preserve existing behavior on the non-watching/poll-only paths.
nodeStream.Connect(timeout);
}
else
{
Stopwatch sw = Stopwatch.StartNew();
while (true)
{
int remaining = timeout - (int)sw.ElapsedMilliseconds;
if (remaining <= 0)
{
// Let the final attempt throw TimeoutException as before so callers' catch sites still work.
nodeStream.Connect(0);
break;
}

try
{
nodeStream.Connect(Math.Min(PollIntervalMs, remaining));
break; // connected
}
catch (TimeoutException)
{
if (watchProcess.HasExited)
{
result = HandshakeResult.Failure(
HandshakeStatus.Timeout,
$"Child process {watchProcess.Id} exited with code {watchProcess.ExitCode} before the named pipe '{pipeName}' became available.");
return false;
}
// else loop and try the next slice
}
}
}

// …existing handshake code below this line is unchanged…
}
```

### Wiring callers

Plumb the optional `Process` through to every call site that *just launched* a child:

1. **`NodeProviderOutOfProcBase.StartNewNode`** ([lines 403–414](https://github.com/dotnet/msbuild/blob/main/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs#L403-L414)) — pass `msbuildProcess` to `TryConnectToProcess`/`TryConnectToPipeStream`. This is the highest-impact site (worker node launches; up to N of them in parallel, each costing 30s today on failure).
2. **`MSBuildClient.ConnectToServer`** — pass the `msbuildProcess` returned from `nodeLauncher.Start` into the connect call. Note: today the `Process` is `using`-scoped to `LaunchNode` and disposed before `ConnectToServer` runs; you'll need to keep the handle alive across both methods (store it in a field next to the existing `_launchedServerPid` and dispose it in `Dispose`/`Shutdown`).
3. **`NodeProviderOutOfProcTaskHost.AcquireAndSetUpHost`** ([lines 181, 186](https://github.com/dotnet/msbuild/blob/main/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs#L181)) — these go through `ShutdownAllNodes`/reuse paths that call into existing `Process` instances. Pass them through too.

The reuse path in `TryReuseAnyFromPossibleRunningNodes` ([line 350](https://github.com/dotnet/msbuild/blob/main/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs#L350)) uses `timeout=0`, so the new code path is naturally skipped — but pass the `Process` anyway for consistency and so future timeouts (if any) are correctly watched.

### `TryConnectToProcess` (the private wrapper at line 747)

Add a `Process? watchProcess` parameter and forward it to `TryConnectToPipeStream`. All in-tree callers can pass the `Process` object they already have.

## Out of scope / explicitly do NOT change

- Do **not** touch the `nodeStream.WriteIntForHandshake` / `TryReadEndOfHandshakeSignal` block. The pipe was successfully opened by then; if the child dies mid-handshake the existing pipe-broken path handles it.
- Do **not** change any timeout *values*. This change only shortens the *failure* path; healthy startup still gets the full budget.
- Do **not** introduce `async`, `Task`, `CancellationToken`, or `WaitForExitAsync`. They aren't needed and they'd require `#if NETCOREAPP`.
- Do **not** remove the diagnostic-trace emission added in #13716 — the `Process.ExitCode` from this fast-fail makes those diagnostics *more* useful, not less.

## Edge cases to handle

1. **PID reuse race.** After `Process.HasExited` returns true, the OS may eventually reassign the PID. Mitigation: we hold a `Process` instance, which keeps the OS handle alive on Windows (and on Unix the `Process` retains the wait handle), so `ExitCode` and `HasExited` are stable. Do not reach for `Process.GetProcessById(pid)` — always use the original `Process` instance the launcher returned.
2. **Race between `HasExited` and `Connect` succeeding.** If `Connect` succeeds in the same poll slice the process exits, prefer the successful connect (the loop `break`s on success before checking `HasExited` — the sketch above does this correctly).
3. **`Process` disposed too early.** `Process.HasExited` throws `InvalidOperationException` after `Dispose`. If the calling code disposes the `Process` before the connect attempt finishes, the loop should treat that the same as "exited" (catch `InvalidOperationException` and translate to a structured failure). Tests should cover this.
4. **`watchProcess` is null.** Always supported — preserves today's behavior. Used by reuse path (`PossibleRunningNodes`) and by callers that don't have a handle.
5. **Sub-poll-interval timeouts.** The `timeout <= PollIntervalMs` short-circuit keeps the hot reuse path (`timeout=0`) byte-identical to today's behavior.

## Tests to add

In `src/Build.UnitTests/BackEnd/`:

1. **Fast-fail test (the meat).** Launch a stub child process that exits immediately with a known non-zero code (use `RunnerUtilities` / `dotnet exec` of a tiny program, or `cmd /c exit 7` on Windows / `sh -c "exit 7"` elsewhere — the existing `ProcessTests` infrastructure has examples). Drive `TryConnectToPipeStream` with a 30s timeout against a never-opened pipe name and assert it returns `false` in well under 1s with the exit code surfaced in `HandshakeResult.ErrorMessage`.
2. **No regression for healthy connect.** Open a real `NamedPipeServerStream` in a background task after a 200ms delay; assert connect still succeeds and that `watchProcess` (a long-running stub) is not consulted incorrectly.
3. **Null-watchProcess parity.** Assert that with `watchProcess: null` the new code path is byte-equivalent to the old behavior on both timeout and success.
4. **Disposed `Process`.** Pass a `Process` that's been disposed; assert structured failure rather than an unhandled exception.
5. **Both target frameworks.** Make sure the new tests run under both `net472` and `net10.0` configurations of `Microsoft.Build.Engine.UnitTests`.

## Acceptance criteria

- `TryConnectToPipeStream` accepts an optional `Process` parameter.
- All four in-engine launch sites pass the launched `Process`.
- When the watched process exits before the pipe opens, the connect call returns a `HandshakeResult.Failure` containing the exit code within ~100ms of process death, regardless of the configured timeout.
- No `#if NETCOREAPP` / `#if NET5_0_OR_GREATER` blocks introduced.
- Healthy node startup wall-time is unchanged (within noise).
- All existing `Microsoft.Build.Engine.UnitTests` and `MSBuild.UnitTests` pass on Windows, Linux, and macOS.
- A new test demonstrates fast-fail in <1s when a stub child exits immediately.

## Notes for the implementer

- The single-blocking-call shape of `nodeStream.Connect(timeout)` is the primary reason this isn't already responsive. Don't try to thread an `INodeLauncher` callback for "process exited" — keep the change local to `TryConnectToPipeStream` plus call-site wiring.
- The `PollIntervalMs = 100` constant is a reasonable starting point. Don't make it configurable unless a test needs it.
- The new `watchProcess` parameter should be the **last** parameter and have a default of `null` so the change is source-compatible with any external callers using `TryConnectToPipeStream` as `internal` (it's `internal static` today, but reflection-based testing or `InternalsVisibleTo` consumers should not break).
- Update the XML doc on `TryConnectToPipeStream` in `NodeProviderOutOfProcBase.cs` to describe the new parameter.

## Related

- Blocked on #13715 / #13716 (the immediate `DOTNET_CLI_USE_MSBUILD_SERVER=true` regression fix). Once this lands, the per-launch timeout in `MSBuildClient` becomes effectively sub-second on the failure path, and the diagnostic message added in #13716 can be tightened to include the actual exit code rather than a "may have failed to start" hedge.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.