IPC: handle disconnects and protocol corruption gracefully

.github/agents/expert-reviewer.agent.md

@@ -52,13 +52,16 @@ Assess all 21 dimensions on every review. For each dimension, either provide rev
 3. Verify new code paths are reachable and tested.
 4. Look for off-by-one errors, wrong boundary conditions, logic inversions, missing cases in switches/pattern matches.
 5. For bug fixes, verify the fix addresses the root cause, not just a symptom.
+6. **Do NOT weaken `ApplicationStateGuard.Unreachable()`, `Debug.Assert`, or explicit invariant throws into "graceful" handling, fallbacks, or warnings without evidence.** These guards exist because the author proved the condition cannot occur given the surrounding protocol/invariant. Replacing them with `LogWarning` + `return` (or any silent recovery path) is a regression: it hides a real bug if the invariant ever breaks, and adds untested, unmaintainable error paths. If the guard is wrong, prove it with a concrete repro or failing test before relaxing it. If a corruption *is* possible from an untrusted boundary, the right severity is `LogError` and abort — never `LogWarning`, which signals a user-actionable condition.
 
 **CHECK — Flag if:**
 - [ ] Off-by-one or wrong boundary condition
 - [ ] Missing case in `switch` or pattern match
 - [ ] Logic inversion (`&&` vs `||`, `<` vs `<=`)
 - [ ] Fix patches a symptom when the root cause could be addressed
 - [ ] New code path unreachable or untested
+- [ ] `ApplicationStateGuard.Unreachable()` / `Debug.Assert` / invariant throw replaced by a "graceful" path, fallback, or warning without a documented repro
+- [ ] Internal-bug condition logged as `LogWarning` (user-actionable severity) instead of `LogError` + abort
 
 ---
 
@@ -210,12 +213,15 @@ The test platform loads arbitrary user code — it must not crash regardless of
 3. Enforce timeouts on user code execution.
 4. Guard against unbounded collection growth from user-controlled input.
 5. Consider `StackOverflowException` risk from deeply recursive user data sources.
+6. **Defensive coding belongs at trust boundaries — not on internal invariants.** Wrapping every internal call in `try/catch`, replacing `throw Unreachable()` with logging, or adding "tolerant" loops on protocols that are strictly synchronous between trusted in-process components is *not* hardening — it's bloat that hides bugs. Before adding "graceful" handling for an internal condition, identify the concrete external trigger (peer process crash, OS-level I/O failure, untrusted input). If there is no concrete trigger, the existing invariant guard is correct and must be preserved.
 
 **CHECK — Flag if:**
 - [ ] Missing `try/catch` around user-provided callback
 - [ ] Reflection `Invoke()` without exception handling
 - [ ] Missing timeout enforcement on user code
 - [ ] Unbounded growth from user-controlled input
+- [ ] "Defensive" handling added for an internal invariant with no documented external trigger
+- [ ] Try/catch wrapping a call between trusted in-process components for an exception that cannot occur given the protocol
 
 ---
 

src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.cs

@@ -154,6 +154,14 @@ public async Task<TResponse> RequestReplyAsync<TRequest, TResponse>(TRequest req
                     _namedPipeClientStream.WaitForPipeDrain();
                 }
             }
+            catch (Exception ex) when (ex is IOException or ObjectDisposedException)
+            {
+                // The server disconnected while we were writing the request. Mirror the read-EOF handling
+                // below: if we cannot deliver the request there's no way to recover, so exit abnormally
+                // instead of surfacing a raw IPC error to the caller.
+                _environment.Exit((int)ExitCode.GenericFailure);
+                throw;
+            }
             finally
             {
                 // Reset the buffers

src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs

@@ -141,6 +141,7 @@ private async Task InternalLoopAsync(CancellationToken cancellationToken)
             if (currentReadBytes == 0)
             {
                 // The client has disconnected
+                await _logger.LogDebugAsync($"Client disconnected from pipe '{PipeName.Name}', exiting read loop").ConfigureAwait(false);
                 return;
             }
 
@@ -250,6 +251,7 @@ private async Task InternalLoopAsync(CancellationToken cancellationToken)
 #endif
 
                 // Send the message
+                bool clientDisconnected = false;
                 try
                 {
 #if NET
@@ -263,13 +265,26 @@ private async Task InternalLoopAsync(CancellationToken cancellationToken)
                         _namedPipeServerStream.WaitForPipeDrain();
                     }
                 }
+                catch (Exception ex) when (ex is IOException or ObjectDisposedException)
+                {
+                    // The client disconnected while we were writing the reply. Treat it as a graceful disconnect
+                    // (symmetric with the read-side EOF handling above) so the server loop exits without crashing
+                    // the host.
+                    await _logger.LogDebugAsync($"Pipe {PipeName.Name} broken while writing reply; treating as client disconnect: {ex.Message}").ConfigureAwait(false);
+                    clientDisconnected = true;
+                }
                 finally
                 {
                     // Reset the buffers
                     _messageBuffer.Position = 0;
                     _serializationBuffer.Position = 0;
                 }
 
+                if (clientDisconnected)
+                {
+                    return;
+                }
+
                 // Reset the control variables
                 currentMessageSize = 0;
                 missingBytesToReadOfWholeMessage = 0;
@@ -304,6 +319,7 @@ public void Dispose()
             // To close gracefully we need to ensure that the client closed the stream in the InternalLoopAsync method (there is comment `// The client has disconnected`).
             if (!_loopTask.Wait(TimeoutHelper.DefaultHangTimeSpanTimeout))
             {
+                _logger.LogError($"NamedPipeServer.Dispose: '{nameof(InternalLoopAsync)}' for pipe '{PipeName.Name}' did not complete within {TimeoutHelper.DefaultHangTimeSpanTimeout}. WasConnected={WasConnected}, LoopTaskStatus={_loopTask.Status}.");
                 throw new InvalidOperationException(string.Format(
                     CultureInfo.InvariantCulture,
                     PlatformResources.InternalLoopAsyncDidNotExitSuccessfullyErrorMessage,
@@ -337,6 +353,7 @@ public async ValueTask DisposeAsync()
             }
             catch (TimeoutException)
             {
+                await _logger.LogErrorAsync($"NamedPipeServer.DisposeAsync: '{nameof(InternalLoopAsync)}' for pipe '{PipeName.Name}' did not complete within {TimeoutHelper.DefaultHangTimeSpanTimeout}. WasConnected={WasConnected}, LoopTaskStatus={_loopTask.Status}.").ConfigureAwait(false);
                 throw new InvalidOperationException(string.Format(
                     CultureInfo.InvariantCulture,
                     PlatformResources.InternalLoopAsyncDidNotExitSuccessfullyErrorMessage,