Stop sending after a partial frame write

[Damian Horna (damianhorna)]

A test run can lose results and still report success.

When a socket write fails partway through a message, the runner and the testhost can lose track of where
one message ends and the next begins. From there the receiver is reading at the wrong offset. It either
deserializes garbage and aborts the run, or waits for a message that will never arrive. Neither side
reports that anything went wrong.

This is the Unexpected character encountered while parsing value abort at Path '', line 0, position 0.
#2879 was closed for want of a repro. There is one below, as a test.

It is not rare where the conditions line up. On one CI pipeline over three weeks, 141 of 379 pull request
builds aborted mid run, and 106 of those still reported success.

The green ones are the problem. An aborted run does not turn failing tests into passing tests, it stops
running them, and a test that never ran reports nothing. For 39 pull requests in that window there is both
a clean successful build and an aborted successful build of the same PR. In 38 of the 39, the aborted build
published coverage for about half as many lines. Same commit, same suite, both green.

What this PR does: once a Send has failed, stop using that channel. Latch the failure, close the
transport, and reject later sends with CommunicationException.

It cannot un-send bytes that already left. What it prevents is the next message being read as the body of a
message that was never finished.

How the two sides get out of sync

Two messages, sizes and prefix bytes taken from a real capture:

intended    a1 83 02 [33,185 byte payload]   ee ab 02 [38,382 byte payload]
                     ^ this write throws

actual      a1 83 02 ee ab 02 [38,382 byte payload]
            ^^^^^^^^ orphaned prefix: "the next 33,185 bytes are one message"

The first message's payload never left, but its 3 byte prefix did, and the channel stayed open. So the
receiver takes a1 83 02 at face value and consumes 33,185 bytes as that message's body: the second
message's prefix, plus the first 33,182 bytes of its payload.

ee ab is not valid UTF-8, so it decodes to U+FFFD, and 02 to U+0002. The result is not JSON. The
position is 0 because the corruption starts at the first byte of the frame, which is why the error names no
useful location.

The receiver cannot recover on its own. A length prefixed stream has no marker to resync on, so once the
boundary is lost, every later message is garbage.

Why the prefix goes out alone

This is the part that makes it look impossible. BinaryWriter.Write(string) writes the prefix and the
payload in one call, so how does one arrive without the other?

From a verbose /Diag capture:

1. A timer callback sends a 33,185 byte TestExecution.StatsChange. BinaryWriter puts the 3 byte prefix
   into the BufferedStream, then writes the payload.
2. The payload does not fit in what is left of the 16 KB buffer, so BufferedStream flushes the 3 buffered
   prefix bytes as their own write and passes the payload straight through. The prefix flush succeeds.
3. The payload write throws SocketException 10038. Send wraps it in CommunicationException,
   TestRunCache.OnCacheTimeHit swallows that, and the run continues.
4. 126 ms later, on another thread, the 38,382 byte message is written to the same channel and succeeds.

So it needs a buffer boundary, a mid message write failure, a caller that swallows it, and a second send.
That combination is why it presents as intermittent and why it resisted reproduction. The arithmetic
matches the log at every step.

The change

In LengthPrefixCommunicationChannel, a failed send records the failure, closes the transport, and every
later send on that channel throws.

The failure path is a named method, FaultChannel, rather than the body of a catch, because the part that
needs justifying deserves somewhere to put it.

Send also moved fully inside the existing _writeSyncObject lock, and Dispose now takes that lock with
Monitor.TryEnter instead of not taking it at all.

This does not stop the underlying write from failing. That is not vstest's to stop. It stops a failed write
from silently corrupting the next message.

What you will want to push back on

Three things, in the order I would raise them.

It closes a stream the channel does not own. Dispose deliberately leaves the stream open, and
DisposeShouldNotCloseTheStream pins that. This change does not touch either. What closes the stream is a
failed send, and that is a different situation, because only that one leaves the peer holding a prefix for a
body that will never arrive.

There is no way to tell the peer in band. The framing it is reading is the thing that broke, so a
"disregard that" message would just be consumed as the missing body. End of stream is the only signal left,
and the peer already handles it. Latching without closing stops the corruption but leaves the peer blocked
until some outer timeout fires.

On the ownership itself: both callers build this channel over a TcpClient stream, in SocketServer.cs and
SocketClient.cs, and both close that client in their own error path. SocketServer spells out what that
means in a comment: "Close the client and dispose the underlying stream". So the stream this closes was
already on its way down. What changes is that it closes when the framing breaks rather than whenever the
read side eventually notices.

It is more than one change. Three things move: the latch, closing the transport, and the locking. That
is worth being precise about, because they are not three independent ideas.

The lock is not a separate improvement. On main the try/catch sits outside the lock, so without
widening it a concurrent send can slip in between another send's failure and the latch being set. That is
the exact race this fix exists to close.

The Dispose change fixes something already broken on main. Dispose there takes no lock at all, so it
can race an in flight write, and since BinaryWriter.Dispose flushes the BufferedStream, it blocks behind
that write. The test below measures 5 seconds on unmodified main. TryEnter is not compensating for the
wider lock, it fixes what was already there.

The latch alone is what fixes the reported corruption. If you would rather take that first and consider the
rest separately, say so and I will split it. Same if you want the latch without closing the transport, or
want it limited to IOException and SocketException.

It will fail runs that pass today. Yes. After a send throws there is no way to know whether the frame
was actually damaged, so this will also close channels that were fine, and where the failed send was the
last one, main would have finished the run and this will not.

What it will not do is turn working builds red. The 106 green builds above had already stopped testing
partway through, and the paired comparison puts the median one at roughly half the code of a clean build of
the same pull request. Those builds are not passing, they are declining to report. Whether you want that
made visible by default in a released product is yours to decide, but I would rather you decided it with
that number in front of you than without it.

How to verify it

PeerShouldNotDecodeAMessageThatWasNeverSent runs a real LengthPrefixCommunicationChannel on both ends.
It fails the first send after its prefix has been flushed, sends a second message, then asks the peer to
read one message. On unmodified main:

failed PeerShouldNotDecodeAMessageThatWasNeverSent (2ms)
  Assert.IsNull failed. 'value' expression: 'received'.
  The peer decoded a 16385 character frame that was never sent as a message,
  starting U+FFFD U+FFFD U+0001.

Same shape as production. The byte values differ only because the test uses SocketConstants.BufferSize + 1
rather than the sizes that happened to occur in the capture.

Five tests fail on unmodified main and pass with this change:

Test	What main does
PeerShouldNotDecodeAMessageThatWasNeverSent	the peer decodes a frame neither send produced
SendShouldCloseStreamAndRejectLaterMessagesAfterPartialFrame	the second Send succeeds instead of throwing
DisposeShouldNotFlushBufferedDataAfterSendFailure	Dispose performs a second write
ConcurrentSendShouldNotWriteAfterFirstSendFails	the queued Send writes after the first one failed
DisposeShouldNeitherWriteNorBlockWhileASendIsInFlight	Dispose blocks for 5 seconds behind the in flight write

SendAfterDisposeShouldStillWriteToTheStream passes before and after. It is there to catch any change to
post Dispose behavior.

I ran these against a worktree at unmodified origin/main with only the test file applied, to confirm they
fail for the stated reason rather than through test setup: 15 total, 5 failed. With the change: 15 total,
0 failed. The nine existing tests in the file pass unchanged.

If you would rather watch it over a real socket, there is a standalone program at
https://gist.github.com/damianhorna/7677d1050b2fcc93464f73f302594fe3 that runs with dotnet run and prints
the production byte values exactly. It reimplements the write path rather than referencing the assembly, so
treat it as an illustration. The tests are the load bearing evidence.

Other questions

Why fail the channel on any exception, rather than only on the ones that leave a partial frame?
Because BinaryWriter.Write(string) does not report how much it wrote. After it throws, the number of bytes
that reached the peer is unknown, so whether the frame is intact is also unknown.

Why Monitor.TryEnter in Dispose rather than lock? Because waiting is the harmful part. A plain
lock would keep the 5 second stall described above and add a shutdown hang whenever a socket write is
stuck. Taking the lock only when it is free gives the same protection against flushing mid frame, and
skipping the flush loses nothing, because Send flushes after every message it completes. The only thing
the buffer can still hold is part of a frame that failed, which is exactly what must not go out.

Why no _isDisposed guard on Send? I tried one and reverted it. Shutdown legitimately sends after
Dispose, because Dispose runs while timer driven sends are still in flight, so the guard silently
dropped real messages. SendAfterDisposeShouldStillWriteToTheStream pins the current behavior instead.

Why no new exception type? Send already threw CommunicationException on write failure. No caller
retries a failed Send or catches that type, so rejecting later sends the same way changes no handling.

Why is the NotSupportedException filter now on _stream instead of _writer.BaseStream? Reading
BaseStream flushes the writer, and an exception filter runs while the exception is still in flight. On a
channel that has just failed mid frame, that flush is the last thing you want. The channel already holds
the stream, so it can check that instead. The log message was updated to match.

[Copilot]

🟡 Changes recommended

Send currently no-ops after Dispose, which can silently drop messages and hide teardown/disconnect issues in callers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens LengthPrefixCommunicationChannel so that a partial-frame write failure becomes terminal: it records the first send failure, closes the underlying stream to prevent subsequent frame-boundary corruption, and ensures later sends fail fast without touching the writer.

Changes:

• Track the first send failure and close the underlying stream on send exceptions to avoid corrupting subsequent frames.
• Serialize disposal with sends and avoid disposing the BinaryWriter after a send failure to prevent buffered re-flush of partial frame bytes.
• Add regression tests that simulate partial-frame writes and concurrent sends to validate the new terminal-failure behavior.

File summaries

File	Description
src/Microsoft.TestPlatform.CommunicationUtilities/LengthPrefixCommunicationChannel.cs	Makes send failures terminal by recording the first failure, closing the underlying stream, and coordinating disposal with concurrent sends.
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/LengthPrefixCommunicationChannelTests.cs	Adds regression coverage for partial-frame failures, disposal after send failure, and concurrent send behavior.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

There are concurrency/behavior issues around Dispose vs Send synchronization and a newly added test that asserts invalid Send-after-Dispose behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

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

[Damian Horna (damianhorna)]

This one I would push back on.

The test passes as written, and it also passes on unmodified main, where it is one of the ten green tests in this file. Two details combine to make a post-Dispose send reach the stream: BinaryWriter tracks no disposed state, so Write has nothing to reject, and the channel builds both reader and writer with leaveOpen set (new BinaryWriter(..., Encoding.UTF8, true)), so disposing them does not close the stream underneath. This PR does not touch that path.

It is a characterization test. It asserts no new guarantee and documents no contract. It records behaviour that shutdown currently leans on, since Dispose runs while timer-driven sends are still in flight. If someone later adds the disposed-state guard, this surfaces as a failing test rather than as a silent change to teardown.

Happy to rename it if Should reads too much like a promise.

[Copilot]

🟡 Changes recommended

Dispose is not serialized with Send (risking flush/retry during concurrent teardown) and one new unit test encodes a contradictory post-Dispose send contract.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/LengthPrefixCommunicationChannelTests.cs:138

• This test asserts that Send continues to work after LengthPrefixCommunicationChannel.Dispose(), but Dispose disposes the BinaryWriter (when no prior send failure). That makes subsequent Send calls invalid and, with the current Send implementation, would be treated as a send failure that can also dispose the underlying stream. Either the production code needs an explicit post-dispose behavior, or this test should be removed to avoid encoding an incorrect contract.

    [TestMethod]
    public async Task SendAfterDisposeShouldStillWriteToTheStream()
    {
        using var stream = new MemoryStream();
        var channel = new LengthPrefixCommunicationChannel(stream);
        channel.Dispose();

        await channel.Send(Dummydata);

        Assert.IsTrue(stream.CanWrite);
        SeekToBeginning(stream);
        using var reader = new BinaryReader(stream);
        Assert.AreEqual(Dummydata, reader.ReadString());
    }

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

[Copilot]

🟢 Approval recommended

The behavioral change is well-scoped to IPC framing correctness and is backed by targeted regression/concurrency tests, with only minor wording nits noted.

Review details

Suppressed comments (1)

src/Microsoft.TestPlatform.CommunicationUtilities/LengthPrefixCommunicationChannel.cs:143

• This comment says "Send flushes every message, so nothing is pending in either case", but the failure path is specifically about avoiding flushing buffered bytes after a failed send. Tweaking the wording would make the rationale clearer and avoid implying there can never be pending buffered data after a failure.

            // BinaryWriter.Dispose flushes its BufferedStream. That would re-emit bytes of an
            // incomplete frame after a failed send, and race the writer while a send is in
            // flight. Send flushes every message, so nothing is pending in either case.
            if (lockTaken && _sendFailure is null)

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The behavioral change is well-scoped to the IPC framing layer and is backed by focused unit tests that cover the new failure-closed and concurrency/disposal scenarios.

Review details

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

[Copilot]

🔵 Needs a closer look

This changes core IPC error-handling and stream lifecycle semantics (including closing externally-owned streams) in a high-impact component, warranting final human review despite the added tests.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core IPC transport send/dispose failure semantics in a high-impact area and should be validated by a human reviewer across expected runtime/host scenarios.

Review details

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

[Damian Horna (damianhorna)]

@microsoft-github-policy-service agree company="Microsoft"

[Damian Horna (damianhorna)]

Adding a runnable repro, since #2879 was closed asking for exactly this.

https://gist.github.com/damianhorna/7677d1050b2fcc93464f73f302594fe3, two files, no clone needed:

dotnet run

It builds the same write path LengthPrefixCommunicationChannel builds in its constructor, over a real loopback socket: BinaryWriter(BufferedStream(stream, 16384), Encoding.UTF8, leaveOpen: true), with Write then Flush per message. The only addition is a deterministic failure on the write that carries the payload, standing in for the SocketException that does it in the wild.

Output:

message 1: 33,185 bytes, prefix a1 83 02
message 2: 38,382 bytes, prefix ee ab 02

=== on main: the channel keeps going after a failed send ===
  send 1: failed, but 3 bytes of it are already on the wire
  send 2: accepted, 38388 bytes total on the wire
  receiver: read 33,184 chars starting U+FFFD U+0002 U+007B
            �{"Version":7,"MessageType":"TestExecution.Completed","Pa...
  receiver: JsonReaderException: Unexpected character encountered while parsing value: �. Path '', line 0, position 0.

=== with the fix: the failed send closes the channel ===
  send 1: failed, but 3 bytes of it are already on the wire
  send 2: rejected, Unable to send data over channel because a previous send failed.
  the receiver sees a closed connection, which it already handles

RESULT: reproduced on main, prevented by the fix.

Three details worth pointing at:

The prefix goes out on its own. BufferedStream holds the 3-byte length prefix, then the payload does not fit in the remaining buffer space, so the prefix is flushed as its own write and the payload is written straight through. When that second write fails, the prefix has already reached the peer. That is the whole bug: 3 bytes of a 33,188-byte frame, and nothing marks the channel unusable.

The next message is consumed as the previous one's body. The receiver reads length 33,185 and takes 33,185 bytes, which are message 2's prefix plus the first 33,182 bytes of its body. ee ab is not valid UTF-8, so it decodes to U+FFFD, and 02 to U+0002. Hence a JSON error at line 0, position 0 rather than anywhere interesting.

Message 2 has to be at least as large as message 1's declared length, or the receiver just blocks on bytes that never arrive. That is probably part of why this has been hard to pin down: the same fault shows up as a hang, a JSON error, or nothing at all, depending on what the next message happens to be.

The sizes above are from a captured failure, so the prefix bytes and the leading U+FFFD U+0002 match that trace exactly. It reproduces on every run.

The same thing is covered by unit tests against the real class, so treat the repro above as illustration
rather than as the evidence. Five of the six tests this PR adds fail on unmodified main:

failed SendShouldCloseStreamAndRejectLaterMessagesAfterPartialFrame (222ms)
failed PeerShouldNotDecodeAMessageThatWasNeverSent (2ms)
failed DisposeShouldNotFlushBufferedDataAfterSendFailure (1ms)
failed ConcurrentSendShouldNotWriteAfterFirstSendFails (55ms)
failed DisposeShouldNeitherWriteNorBlockWhileASendIsInFlight (5s 005ms)

PeerShouldNotDecodeAMessageThatWasNeverSent is the closest of those to the repro: a real
LengthPrefixCommunicationChannel on both ends, a first send that fails after its prefix has been flushed,
and a second send behind it. On main the receiving channel hands up a message nobody ever sent:

Assert.IsNull failed. The peer decoded a 16385 character frame that was never sent as a message,
starting U+FFFD U+FFFD U+0001.

The last failure in the list is not about this bug and is worth a separate look: on main, Dispose racing
an in-flight write blocks for the full duration of that write, because BinaryWriter.Dispose flushes the
BufferedStream. That is why this PR uses Monitor.TryEnter there rather than taking the lock.

[Copilot]

🟢 Approval recommended

The change addresses a concrete IPC framing failure mode with a clear fail-closed strategy and comprehensive unit tests covering the new behavior and key concurrency/dispose edge cases.

Review details

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

[Copilot]

🔵 Needs a closer look

It changes core IPC transport failure semantics (faulting/closing on send failure and altering dispose behavior) in a wire-critical component, so it warrants final human review despite strong unit test coverage.

Review details

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