IPC: handle disconnects and protocol corruption gracefully#8602
Open
Evangelink wants to merge 1 commit into
Open
IPC: handle disconnects and protocol corruption gracefully#8602Evangelink wants to merge 1 commit into
Evangelink wants to merge 1 commit into
Conversation
Replace ApplicationStateGuard.Unreachable() throws in IPC header/payload reads with graceful disconnect handling. Tolerate short reads, treat mid-header EOF as graceful disconnect, validate currentMessageSize > 0, and catch IOException/ObjectDisposedException during write/flush/drain so a peer disconnect cannot crash the host or client process. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the Microsoft.Testing.Platform named-pipe IPC transport so disconnects and certain protocol-corruption scenarios don’t crash the host/client process (replacing prior “unreachable” paths with graceful exits).
Changes:
- Server: tolerate short/EOF header reads, reject non-positive message sizes, and handle write-side disconnects without FailFast.
- Client: add write-side disconnect handling, tolerate short response headers, and treat response deserialization failures as a generic IPC failure exit.
- Tests: add a regression test to ensure an invalid (0) message-size header doesn’t crash the host.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/IPC/IPCTests.cs | Adds regression coverage for invalid message-size header handling on the server. |
| src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs | Makes the server loop tolerant to mid-header EOF/short reads and write-side disconnects; logs and exits cleanly on certain corrupt headers. |
| src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.cs | Adds write-side disconnect handling, short-read header handling, and exits cleanly on corruption/deserialization errors. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 4
Comment on lines
+175
to
+178
| if (currentMessageSize <= 0) | ||
| { | ||
| // Protocol corruption: message size must be positive. Drop the connection. | ||
| await _logger.LogWarningAsync($"Pipe {PipeName.Name} received invalid message size {currentMessageSize}; closing connection.").ConfigureAwait(false); |
Comment on lines
+223
to
+227
| if (currentMessageSize <= 0) | ||
| { | ||
| // Protocol corruption: message size must be positive. | ||
| _environment.Exit((int)ExitCode.GenericFailure); | ||
| throw new InvalidOperationException($"Received invalid IPC message size {currentMessageSize}."); |
Comment on lines
230
to
232
| missingBytesToReadOfCurrentChunk = currentReadBytes - sizeof(int); | ||
| missingBytesToReadOfWholeMessage = currentMessageSize; | ||
| currentReadIndex = sizeof(int); |
Comment on lines
+242
to
+275
| var serverEnvironment = new SystemEnvironment(); | ||
| bool callbackInvoked = false; | ||
|
|
||
| NamedPipeServer server = new( | ||
| pipeNameDescription, | ||
| _ => | ||
| { | ||
| callbackInvoked = true; | ||
| return Task.FromResult<IResponse>(VoidResponse.CachedInstance); | ||
| }, | ||
| serverEnvironment, | ||
| new Mock<ILogger>().Object, | ||
| new SystemTask(), | ||
| _testContext.CancellationToken); | ||
|
|
||
| try | ||
| { | ||
| Task waitConnection = server.WaitConnectionAsync(_testContext.CancellationToken); | ||
|
|
||
| using (var raw = new System.IO.Pipes.NamedPipeClientStream(".", pipeNameDescription.Name, System.IO.Pipes.PipeDirection.InOut, System.IO.Pipes.PipeOptions.Asynchronous)) | ||
| { | ||
| await raw.ConnectAsync(_testContext.CancellationToken); | ||
| await waitConnection; | ||
|
|
||
| // Write a zero-length message size header (invalid per protocol). | ||
| byte[] invalidHeader = BitConverter.GetBytes(0); | ||
| await raw.WriteAsync(invalidHeader, 0, invalidHeader.Length, _testContext.CancellationToken); | ||
| await raw.FlushAsync(_testContext.CancellationToken); | ||
| } | ||
|
|
||
| // The server's internal loop should exit cleanly within the disposal timeout. If the loop | ||
| // crashed via FailFast the test process would be terminated instead of running to completion. | ||
| Assert.IsFalse(callbackInvoked, "Server callback must not run for an invalid message header."); | ||
| } |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| } | ||
|
|
||
| currentReadBytes += additionalBytes; | ||
| missingBytesToReadOfCurrentChunk = currentReadBytes; |
Member
Author
Test Coverage GapsThe PR introduces 8 new error paths but only tests 1 (server-side zero message size). Missing test scenarios: Client-side (4 untested paths):
Server-side (3 untested paths):
All 8 scenarios are concrete failing interleavings that the new code explicitly handles. Recommend adding these tests to prevent regressions.
|
Youssef1313
reviewed
May 26, 2026
| // If currentRequestSize is 0, we need to read the message size | ||
| if (currentMessageSize == 0) | ||
| { | ||
| // We need at least sizeof(int) bytes to parse the message-size header. A pipe read can |
Member
There was a problem hiding this comment.
This is adding a lot of additional code. Is there a concrete bug that makes it worth adding this?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens the Microsoft.Testing.Platform IPC layer (named-pipe transport between the test host and its in-process clients) so that a peer disconnect or a corrupt/short header byte no longer takes down the host or client process.
Audit items addressed
Part of the P0+P1 exception-handling audit (items #4–#10, IPC).
Changes
NamedPipeServer.csApplicationStateGuard.Unreachable()throws in the header read path with graceful disconnect handling (tolerate short reads, treat mid-header EOF as a normal disconnect).currentMessageSize <= 0→ log warning and return cleanly instead of crashing on a corrupt header.WriteAsync/FlushAsync/WaitForPipeDrainin try/catch (IOException/ObjectDisposedException) → setclientDisconnectedand exit the loop after resetting buffers, rather than tearing down the host.NamedPipeClient.csIOException/ObjectDisposedExceptionand routes through the existing_environment.Exit(GenericFailure)path used by the read-EOF handler.currentMessageSize <= 0→ exit on corruption.Deserializein try/catch (excludingOperationCanceledException) so protocol corruption exits cleanly instead of bubbling an undecorated deserialization exception.Test
NamedPipeServer_InvalidMessageSizeHeader_DoesNotCrashHostsends a zero-byte size header via a rawNamedPipeClientStreamand asserts the server stays alive instead of throwingApplicationStateGuard.Unreachable.Microsoft.Testing.Platform.UnitTestsIPC tests still pass on net9.0.Notes
GenericFailureexit code used for the read-EOF path on the client side.WaitConnectionAsyncFailFast is intentionally preserved; only loop-body IO faults get graceful handling.